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

FormatArgsExpn: Find comma spans and ignore weird proc macro spans #9586

Merged
merged 1 commit into from
Oct 5, 2022

Conversation

Alexendoo
Copy link
Member

Fixes the following cases:

A missing , 1 from the expect_fun_call suggestion:

Some(()).expect(&format!("{x} {}", 1));
warning: use of `expect` followed by a function call
 --> t.rs:7:14
  |
7 |     Some(()).expect(&format!("{x} {}", 1));
  |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| panic!("{x} {}"))`

The suggestion removing from the comma in the comment rather than the one after the format string:

println!(
    "{}",
    // a comment, with a comma in it
    x
);
warning: variables can be used directly in the `format!` string
  --> t.rs:9:5
   |
9  | /     println!(
10 | |         "{}",
11 | |         // a comment, with a comma in it
12 | |         x
13 | |     );
   | |_____^
   |
help: change this to
   |
10 ~         "{x}",
11 ~         // a comment
   |

It also no longer accepts expansions where a format string or argument has a "weird" proc macro span, that is one where the literal/expression it outputs has the span of one of its inputs. Kind of like a format_args specific clippy_utils::is_from_proc_macro, e.g. format!(indoc! {" ... "})

changelog: [expect_fun_call]: Fix suggestion for format! using captured variables
changelog: [print_literal], [write_literal], [uninlined_format_args]: Fix suggestion when following a comment including a comma

@rust-highfive
Copy link

r? @Manishearth

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 3, 2022
@bors
Copy link
Contributor

bors commented Oct 4, 2022

☔ The latest upstream changes (presumably #9547) made this pull request unmergeable. Please resolve the merge conflicts.

@nyurik
Copy link
Contributor

nyurik commented Oct 4, 2022

Overall looks good to me! One thing you may want to reuse - I trimmed down the indoc! macro in the unit tests for #9548 -- the simple_indoc -- would it make sense to merge that unit test into your code here?

@Alexendoo
Copy link
Member Author

Alexendoo commented Oct 4, 2022

with_span! covers the case where a proc macro sets the output span to one of its inputs

///
/// 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>> {
Copy link
Member

Choose a reason for hiding this comment

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

nit: fmt_args_comma_spans?

Copy link
Member

Choose a reason for hiding this comment

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

or perhaps as a static fn on the format args expansion helper type

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

Changed

@Manishearth
Copy link
Member

r=me, one nit

@Manishearth
Copy link
Member

@bors r+

(Thanks for the additional review help, @nyurik!)

@bors
Copy link
Contributor

bors commented Oct 5, 2022

📌 Commit 9226066 has been approved by Manishearth

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 5, 2022

⌛ Testing commit 9226066 with merge 887ba0c...

@bors
Copy link
Contributor

bors commented Oct 5, 2022

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

1 similar comment
@bors
Copy link
Contributor

bors commented Oct 5, 2022

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

@bors bors merged commit 887ba0c into rust-lang:master Oct 5, 2022
@Alexendoo Alexendoo deleted the format-args-commas branch October 5, 2022 17:10
/// `format!("{x} {} {y}", 1, z + 2)`.
value_args: Vec<&'tcx Expr<'tcx>>,
explicit_values: Vec<&'tcx Expr<'tcx>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

we may want to clarify this comment -- the above format!(...) doesn't look like valid code.

Copy link
Member Author

@Alexendoo Alexendoo Oct 6, 2022

Choose a reason for hiding this comment

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

Ah whoops, the {y} should be {}. I also forgot to remove the FIXME if you fancy doing a cleanup PR, otherwise I'll get around to it

@nyurik nyurik mentioned this pull request Oct 6, 2022
bors added a commit that referenced this pull request Oct 6, 2022
lint: fix a few comments

minor cleanup per `@Alexendoo` [comment](#9586 (comment))

changelog: none
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.

5 participants