Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate format_args.rs to rustc_ast::FormatArgs #10484

Merged
merged 1 commit into from
Mar 28, 2023

Conversation

Alexendoo
Copy link
Member

changelog: none

Part of #10233

Empty precision specifiers are no longer linted as the span for that isn't present in FormatOptions

format!("{:.}", ...)

That could be fixed later with some hackery or a change upstream

r? @flip1995

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 11, 2023
}

false
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a slightly simplified version of

/// Gets the spans of the commas inbetween the format string and explicit args, not including
/// any trailing comma
///
/// ```ignore
/// format!("{} {}", a, b)
/// // ^ ^
/// ```
///
/// Ensures that the format string and values aren't coming from a proc macro that sets the
/// output span to that of its input
fn comma_spans(cx: &LateContext<'_>, explicit_values: &[&Expr<'_>], fmt_span: Span) -> Option<Vec<Span>> {
// `format!("{} {} {c}", "one", "two", c = "three")`
// ^^^^^ ^^^^^ ^^^^^^^
let value_spans = explicit_values
.iter()
.map(|val| hygiene::walk_chain(val.span, fmt_span.ctxt()));
// `format!("{} {} {c}", "one", "two", c = "three")`
// ^^ ^^ ^^^^^^
let between_spans = once(fmt_span)
.chain(value_spans)
.tuple_windows()
.map(|(start, end)| start.between(end));
let mut comma_spans = Vec::new();
for between_span in between_spans {
let mut offset = 0;
let mut seen_comma = false;
for token in tokenize(&snippet_opt(cx, between_span)?) {
match token.kind {
TokenKind::LineComment { .. } | TokenKind::BlockComment { .. } | TokenKind::Whitespace => {},
TokenKind::Comma if !seen_comma => {
seen_comma = true;
let base = between_span.data();
comma_spans.push(Span::new(
base.lo + BytePos(offset),
base.lo + BytePos(offset + 1),
base.ctxt,
base.parent,
));
},
// named arguments, `start_val, name = end_val`
// ^^^^^^^^^ between_span
TokenKind::Ident | TokenKind::Eq if seen_comma => {},
// An unexpected token usually indicates the format string or a value came from a proc macro output
// that sets the span of its output to an input, e.g. `println!(some_proc_macro!("input"), ..)` that
// emits a string literal with the span set to that of `"input"`
_ => return None,
}
offset += token.len;
}
if !seen_comma {
return None;
}
}
Some(comma_spans)
}

Comment on lines -777 to -788
error: variables can be used directly in the `format!` string
--> $DIR/uninlined_format_args.rs:125:20
|
LL | println!("{}", format!("{}", local_i32));
| ^^^^^^^^^^^^^^^^^^^^^^^^
|
help: change this to
|
LL - println!("{}", format!("{}", local_i32));
LL + println!("{}", format!("{local_i32}"));
|

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bug with root_macro_call_first_node, not sure why but it doesn't pick up that format!() as a macro call

@@ -9,6 +9,7 @@ keywords = ["clippy", "lint", "plugin"]
edition = "2021"

[dependencies]
arrayvec = { version = "0.7", default-features = false }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clippy_utils depends on this already so it's not a new dependency

@m-ou-se
Copy link
Member

m-ou-se commented Mar 11, 2023

Empty precision specifiers are no longer linted as the span for that isn't present in FormatOptions

format!("{:.}", ...)

We should probably just move that warning to rustc, since an empty . specifier is always useless.

@nyurik
Copy link
Contributor

nyurik commented Mar 13, 2023

I tried to apply a similar change to explicit_write.rs , and root_macro_call_first_node wasn't a drop-in replacement for FormatArgsExpn::parse. Most likely i am doing something totally wrong. Something like this partially works, but is probably wrong as well:

if let ExprKind::MethodCall(unwrap_fun, write_call, [], _) = expr.kind
    && unwrap_fun.ident.name == sym::unwrap
    // match call to write_fmt
    && let ExprKind::MethodCall(write_fun, write_recv, [write_arg], _) = look_in_block(cx, &write_call.kind)
    && let ExprKind::Call(_, write_arg2) = look_in_block(cx, &write_arg.kind)
    && write_fun.ident.name == sym!(write_fmt)
    // match calls to std::io::stdout() / std::io::stderr ()
    && let Some(dest_name) = if match_function_call(cx, write_recv, &paths::STDOUT).is_some() {
        Some("stdout")
    } else if match_function_call(cx, write_recv, &paths::STDERR).is_some() {
        Some("stderr")
    } else {
        None
    }
    && let Some(format_args) = root_macro_call_first_node(cx, write_call)
    && is_format_macro(cx, format_args.def_id)
    // && let Some(format_args2) = FormatArgsExpn::find_nested(cx, expr, format_args.expn)

@Alexendoo
Copy link
Member Author

The replacement would be find_format_args. The root_macro_call_first_node would be detecting the write[ln] macro call there, not format_args

You wouldn't need is_format_macro as it's already checking the macro kind below that, but if you're using root_macro_call_first_node it could replace the two is_expn_of checks with a match cx.tcx.get_diagnostic_name(macro_call.def_id) { .. }

No longer lints empty precisions `{:.}` as the spans aren't available
collect_ast_format_args(expr.span, args);
}
}
}

/// Detects if the format string or an argument has its span set by a proc macro to something inside
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: this is neat

@Manishearth
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 28, 2023

📌 Commit 3259b48 has been approved by Manishearth

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Mar 28, 2023

⌛ Testing commit 3259b48 with merge 84e42fb...

@bors
Copy link
Contributor

bors commented Mar 28, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Manishearth
Pushing 84e42fb to master...

@bors bors merged commit 84e42fb into rust-lang:master Mar 28, 2023
@Alexendoo Alexendoo deleted the format-args-ast-2 branch March 28, 2023 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants