Skip to content

Migrate write.rs to a late pass#8518

Merged
bors merged 1 commit intorust-lang:masterfrom
Alexendoo:write-late-pass
Sep 14, 2022
Merged

Migrate write.rs to a late pass#8518
bors merged 1 commit intorust-lang:masterfrom
Alexendoo:write-late-pass

Conversation

@Alexendoo
Copy link
Member

@Alexendoo Alexendoo commented Mar 10, 2022 •
edited
Loading

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
Copy link

rust-highfive commented Mar 10, 2022

r? @flip1995

(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 Mar 10, 2022
@bors
Copy link
Contributor

bors commented Mar 14, 2022

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

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

flip1995 commented Mar 17, 2022

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
Copy link
Contributor

bors commented Mar 21, 2022

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

@Jarcho
Copy link
Contributor

Jarcho commented Mar 22, 2022

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
Copy link
Member Author

Alexendoo commented Mar 22, 2022

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

@xFrednet xFrednet removed the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Mar 22, 2022
@Alexendoo Alexendoo force-pushed the write-late-pass branch 4 times, most recently from 8b92deb to 7e6a1c4 Compare March 23, 2022 21:31
@Alexendoo
Copy link
Member Author

Alexendoo commented Mar 23, 2022

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

Alexendoo
Alexendoo commented Mar 24, 2022
View reviewed changes
@bors
Copy link
Contributor

bors commented Mar 30, 2022

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

@Alexendoo Alexendoo mentioned this pull request Apr 24, 2022
Closed
@flip1995 flip1995 mentioned this pull request Apr 28, 2022
Closed
@bors
Copy link
Contributor

bors commented May 5, 2022

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

@bors
Copy link
Contributor

bors commented May 9, 2022

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

@bors
Copy link
Contributor

bors commented Jun 30, 2022

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

@bors
Copy link
Contributor

bors commented Jul 6, 2022

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

@nyurik nyurik mentioned this pull request Jul 22, 2022
Closed
@nyurik
Copy link
Contributor

nyurik commented Jul 24, 2022 •
edited
Loading

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

@nyurik nyurik mentioned this pull request Jul 28, 2022
Merged
@bors
Copy link
Contributor

bors commented Aug 11, 2022

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

flip1995
flip1995 reviewed Aug 15, 2022
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

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.

@Alexendoo Alexendoo mentioned this pull request Aug 18, 2022
Merged
@Alexendoo
Copy link
Member Author

Alexendoo commented Aug 18, 2022

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

flip1995 commented Aug 19, 2022

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 👍

bors added a commit that referenced this pull request Aug 19, 2022
@bors
Auto merge of #9349 - Alexendoo:format-args-expn, r=flip1995
3e594de 
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`
@flip1995 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
Copy link
Member Author

Alexendoo commented Aug 21, 2022

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

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

Alexendoo commented Aug 30, 2022

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
Copy link
Contributor

bors commented Sep 1, 2022

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

@nyurik
Copy link
Contributor

nyurik commented Sep 1, 2022

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
Copy link
Contributor

bors commented Sep 8, 2022

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

@flip1995
Copy link
Member

flip1995 commented Sep 14, 2022

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
Copy link
Contributor

bors commented Sep 14, 2022

📌 Commit 6fc6d87 has been approved by flip1995

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 14, 2022

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

@bors
Copy link
Contributor

bors commented Sep 14, 2022

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

@bors bors merged commit 2ddbc86 into rust-lang:master Sep 14, 2022
@bors bors mentioned this pull request Sep 14, 2022
Merged
@Alexendoo Alexendoo deleted the write-late-pass branch September 15, 2022 11:55
@dswij dswij mentioned this pull request Nov 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@flip1995 flip1995 flip1995 left review comments

Assignees

@flip1995 flip1995

Labels

S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)

Projects

None yet

Milestone

No milestone

7 participants

@Alexendoo @rust-highfive @bors @flip1995 @Jarcho @nyurik @xFrednet