Migrate write.rs to a late pass #8518

Alexendoo · 2022-03-10T19:26:10Z

changelog: Migrates write.rs from a pre expansion pass to a late pass
changelog: [positional_named_format_parameters] is renamed in favour of the rustc lint named_arguments_used_positionally

Macros are now identified by diagnostic items, so will no longer lint user defined macros named, e.g. a custom print!
print_literal/write_literal no longer lint no longer lint literals that come from macro expansions, e.g. env!("FOO")
print_with_newline/write_with_newline no longer lint strings with any internal \r or \ns

~~A false negative, print_literal/write_literal don't lint format strings that produce FormatSpecs, e.g. ones containing pretty print/width/align specifiers~~

Suggestion changes:

~~print_literal/write_literal no longer have suggestions, as the spans for the {}s were not easily obtainable~~
print_with_newline/write_with_newline has a better suggestion for a sole literal newline, but no longer has suggestions for len > 1 strings that end in a literal newline
~~use_debug spans are less precise, now point to the whole format string~~

The diff for write.rs is pretty unwieldy, other than for the declare_clippy_lint!s I think you'd be better off viewing it as a brand new file rather than looking at the diff, as it's mostly written from scratch

cc #6610, fixes #5721, fixes #7195, fixes #8615

rust-highfive · 2022-03-10T19:26:13Z

r? @flip1995

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

bors · 2022-03-14T10:53:32Z

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

flip1995 · 2022-03-17T13:48:30Z

Thanks for putting in the work! This loses many useful suggestions. Retaining those is currently not really possible as a LatePass. I nominated this issue for discussion in the next Clippy meeting, if we're ready for this tradeoff.

bors · 2022-03-21T21:01:10Z

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

Jarcho · 2022-03-22T12:54:26Z

print_literal/write_literal no longer have suggestions, as the spans for the {}s were not easily obtainable

print_with_newline/write_with_newline has a better suggestion for a sole literal newline, but no longer has suggestions for len > 1 strings that end in a literal newline

use_debug spans are less precise, now point to the whole format string

You should be able to walk the chain of SyntaxContexts to find the entry point and then re-lex the the text to find the format string. Definitely work, but it should be doable.

Alexendoo · 2022-03-22T22:46:26Z

Discussed in https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Meeting.202022-03-22

Alexendoo · 2022-03-23T21:44:49Z

I think this is ready to review now that I've finished upsetting the dogfood test 😄

The FormatSpecs false negatives are gone, _literal suggestions have returned, and use_debug points at the {:?}s once more

tests/ui/write_literal_2.stderr

bors · 2022-03-30T17:13:10Z

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

bors · 2022-05-05T13:31:26Z

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

bors · 2022-05-09T18:00:35Z

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

bors · 2022-06-30T08:51:53Z

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

bors · 2022-07-06T09:39:26Z

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

nyurik · 2022-07-24T03:05:17Z

I wrote inline format arguments lint #9233 that can convert print!("{}", var) into print!("{var}"). I had to change the FormatArgsArg to keep track of the argument_span -- the span of the argument before the : (if it exists). This is not sufficient though, because i will also need to track the optional precision and width spans, and we will also need to handle is_aliased correctly to include the usage by width and precision. Apparently there is also some related work being done in rustc - see rust-lang/rust#99655

bors · 2022-08-11T18:06:59Z

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

flip1995

Nice rewrite, the code looks really clean and I enjoyed reading through it.

I just left a few comments of "please add a comment here", nothing major.

Sorry again for taking so long.

clippy_lints/src/lib.rs

clippy_lints/src/write.rs

Alexendoo · 2022-08-18T17:42:03Z

Thank you for the review!

Sorry to drop another large PR on you, but as #9040 got merged I think this is blocked on #9349 😅

Unless we land this keeping the pre expansion pass around just for that lint, but I'm in no hurry

flip1995 · 2022-08-19T10:37:02Z

Ugh, thanks for the heads up. I'll try to get to all of this until early Sunday afternoon CEST, when my workaction ends and my vacation starts 👍

Refactor `FormatArgsExpn` It now for each format argument `{..}` has: - The `Expr` it points to, and how it does so (named/named inline/numbered/implicit) - The parsed `FormatSpec` (format trait/fill/align/etc., the precision/width and any value they point to) - Many spans The caller no longer needs to pair up arguments to their value, or separately interpret the `specs` `Expr`s when it isn't `None` The gist is that it combines the result of [`rustc_parse_format::Parser`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_parse_format/struct.Parser.html) with the macro expansion itself This unfortunately makes the code a bit longer, however we need to use both as neither have all the information we're after. `rustc_parse_format` doesn't have the information to resolve named arguments to their values. The macro expansion doesn't contain whether the positions are implicit/numbered/named, or the spans for format arguments Wanted by #9233 and #8518 to be able to port the changes from #9040 Also fixes #8643, previously the format args seem to have been paired up with the wrong values somehow changelog: [`format_in_format_args`]: Fix false positive due to misattributed arguments r? `@flip1995` cc `@nyurik`

Alexendoo · 2022-08-21T20:25:49Z

Ran into some span issues - rust-lang/rust#100851

Alexendoo · 2022-08-30T13:00:29Z

Rebased it onto master, there weren't many substantial changes to the existing lints but I marked clippy::positional_named_format_parameters as renamed to named_arguments_used_positionally, as it seems to have been improved to cover all the other cases the clippy lint was catching

bors · 2022-09-01T04:25:47Z

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

nyurik · 2022-09-01T17:03:45Z

Seems like this is getting really really close to merging! 🤞 I tried rebasing #9233 on top of this, but with all the constant changes and forced-pushes it is a bit too difficult to keep it up to date, so patiently waiting until the merge :). Based on @flip1995 comment, seems like there is nothing blocking it now 🎉

bors · 2022-09-08T19:56:03Z

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

flip1995 · 2022-09-14T15:58:16Z

Thanks! Sorry it took so long... $day_job is currently taking up all my time.

Let's get this merged.

@bors r+ p=1

bors · 2022-09-14T15:58:17Z

📌 Commit 6fc6d87 has been approved by flip1995

It is now in the queue for this repository.

bors · 2022-09-14T15:58:24Z

⌛ Testing commit 6fc6d87 with merge 2ddbc86...

bors · 2022-09-14T16:10:05Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 2ddbc86 to master...

rust-highfive assigned flip1995 Mar 10, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 10, 2022

Alexendoo force-pushed the write-late-pass branch from 40301d3 to c638cce Compare March 14, 2022 13:20

flip1995 added the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Mar 17, 2022

Alexendoo force-pushed the write-late-pass branch from c638cce to a3553f1 Compare March 22, 2022 12:01

xFrednet removed the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Mar 22, 2022

Alexendoo force-pushed the write-late-pass branch 4 times, most recently from 8b92deb to 7e6a1c4 Compare March 23, 2022 21:31

Alexendoo commented Mar 24, 2022

View reviewed changes

tests/ui/write_literal_2.stderr Outdated Show resolved Hide resolved

Alexendoo force-pushed the write-late-pass branch from 97c8cfb to 2f51fd3 Compare March 30, 2022 18:11

Alexendoo mentioned this pull request Apr 24, 2022

false negative write_with_newline: format string that starts with \n #8741

Closed

flip1995 mentioned this pull request Apr 28, 2022

Clippy ignores crate wide warn/deny #6610

Closed

Alexendoo force-pushed the write-late-pass branch from 2f51fd3 to 6750424 Compare May 5, 2022 17:41

Alexendoo force-pushed the write-late-pass branch from 6750424 to b2722b2 Compare May 10, 2022 15:29

Alexendoo force-pushed the write-late-pass branch from b2722b2 to 4797c3c Compare June 30, 2022 10:52

nyurik mentioned this pull request Jul 22, 2022

New lint: suggest using "implicit named arguments" #8368

Closed

nyurik mentioned this pull request Jul 28, 2022

Add new lint [positional_named_format_parameters] #9040

Merged

flip1995 reviewed Aug 15, 2022

View reviewed changes

Alexendoo mentioned this pull request Aug 18, 2022

Refactor FormatArgsExpn #9349

Merged

flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Aug 21, 2022

Alexendoo force-pushed the write-late-pass branch 3 times, most recently from 03c7bde to 8276e68 Compare August 30, 2022 12:47

Alexendoo force-pushed the write-late-pass branch from 8276e68 to 75a8444 Compare September 1, 2022 15:32

Alexendoo force-pushed the write-late-pass branch from 75a8444 to 4dd22c7 Compare September 8, 2022 20:10

Migrate write.rs to a late pass

6fc6d87

Alexendoo force-pushed the write-late-pass branch from 4dd22c7 to 6fc6d87 Compare September 8, 2022 20:18

bors merged commit 2ddbc86 into rust-lang:master Sep 14, 2022

bors mentioned this pull request Sep 14, 2022

bool_to_int_with_if inverse case patch #9476

Merged

Alexendoo deleted the write-late-pass branch September 15, 2022 11:55

dswij mentioned this pull request Nov 1, 2022

Changelog for Rust 1.65 #9761

Merged

Uh oh!

Migrate write.rs to a late pass #8518

Migrate write.rs to a late pass #8518

Uh oh!

Conversation

Alexendoo commented Mar 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Mar 10, 2022

Uh oh!

bors commented Mar 14, 2022

Uh oh!

flip1995 commented Mar 17, 2022

Uh oh!

bors commented Mar 21, 2022

Uh oh!

Jarcho commented Mar 22, 2022

Uh oh!

Alexendoo commented Mar 22, 2022

Uh oh!

Alexendoo commented Mar 23, 2022

Uh oh!

Uh oh!

bors commented Mar 30, 2022

Uh oh!

bors commented May 5, 2022

Uh oh!

bors commented May 9, 2022

Uh oh!

bors commented Jun 30, 2022

Uh oh!

bors commented Jul 6, 2022

Uh oh!

nyurik commented Jul 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Aug 11, 2022

Uh oh!

flip1995 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Alexendoo commented Aug 18, 2022

Uh oh!

flip1995 commented Aug 19, 2022

Uh oh!

Alexendoo commented Aug 21, 2022

Uh oh!

Alexendoo commented Aug 30, 2022

Uh oh!

bors commented Sep 1, 2022

Uh oh!

nyurik commented Sep 1, 2022

Uh oh!

bors commented Sep 8, 2022

Uh oh!

flip1995 commented Sep 14, 2022

Uh oh!

bors commented Sep 14, 2022

Uh oh!

bors commented Sep 14, 2022

Uh oh!

bors commented Sep 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Alexendoo commented Mar 10, 2022 •

edited

Loading

nyurik commented Jul 24, 2022 •

edited

Loading