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

Refactor FormatArgsExpn #9349

Merged
merged 1 commit into from
Aug 19, 2022
Merged

Refactor FormatArgsExpn #9349

merged 1 commit into from
Aug 19, 2022

Conversation

Alexendoo
Copy link
Member

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 Exprs when it isn't None

The gist is that it combines the result of rustc_parse_format::Parser 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

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

Maybe this makes #8857 unnecessary?

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.

This all is really specific to how the format_arg! macro expands. But that also was the previous code, so I don't see this as a blocker.

However, I noticed that the pattern is to construct those things from rpf information. Can you give an estimation how likely it is to add this information to rpf and give Clippy access to it? This feels like we're doing work that is/can already (be) done in rustc.

Those are no blockers. Just a few comments how the code could maybe be improved.

clippy_utils/src/source.rs Outdated Show resolved Hide resolved
clippy_lints/src/format_args.rs Show resolved Hide resolved
clippy_lints/src/format.rs Outdated Show resolved Hide resolved
clippy_utils/src/lib.rs Show resolved Hide resolved
clippy_utils/src/source.rs Outdated Show resolved Hide resolved
clippy_utils/src/macros.rs Show resolved Hide resolved
clippy_utils/src/macros.rs Outdated Show resolved Hide resolved
clippy_utils/src/macros.rs Show resolved Hide resolved
clippy_utils/src/macros.rs Outdated Show resolved Hide resolved
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.

Only 2 minor comments, then we can move on with the other PR 🚀

clippy_utils/src/macros.rs Outdated Show resolved Hide resolved
clippy_utils/src/macros.rs Outdated Show resolved Hide resolved
@Alexendoo
Copy link
Member Author

However, I noticed that the pattern is to construct those things from rpf information. Can you give an estimation how likely it is to add this information to rpf and give Clippy access to it? This feels like we're doing work that is/can already (be) done in rustc.

I don't think rustc_parse_format could gain the full capabilities since it's intended to compile on stable. Resolving the names would need access to the bits (TokenTree?) that come after the format string

There is talk of making format_args its own node though, so maybe in the future we can delete all of this 😄

@Alexendoo Alexendoo force-pushed the format-args-expn branch 2 times, most recently from 80cbf42 to cd8eda5 Compare August 19, 2022 15:28
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.

Thanks, just another detail. After that r=me.

clippy_utils/src/macros.rs Show resolved Hide resolved
@flip1995
Copy link
Member

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented Aug 19, 2022

📌 Commit 4f049f5 has been approved by flip1995

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Aug 19, 2022

⌛ Testing commit 4f049f5 with merge 3e594de...

@bors
Copy link
Contributor

bors commented Aug 19, 2022

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

@bors bors merged commit 3e594de into rust-lang:master Aug 19, 2022
@Alexendoo Alexendoo deleted the format-args-expn branch August 19, 2022 16:16
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.

format_in_format_args lint suggests code that results in alternative behaviour
5 participants