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.

Sign up for GitHub

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

Jump to bottom

Migrate format_args!() lints to to ast::FormatArgs #10233

Closed
24 tasks done
m-ou-se opened this issue Jan 26, 2023 · 13 comments · Fixed by #10561
Closed
24 tasks done

Migrate format_args!() lints to to ast::FormatArgs #10233

m-ou-se opened this issue Jan 26, 2023 · 13 comments · Fixed by #10561

Comments

@m-ou-se
Copy link
Member

m-ou-se commented Jan 26, 2023 •
edited
Loading

Now that rust-lang/rust#106745 is merged, Clippy has access to the original parsed/processed format_args!() invocation. This hopefully means that Clippy no longer needs to do anything that depends on the exact expansion of format_args!() and implementation details of fmt::Arguments (which will undergo changes in the (near!) future).

Details:

As of rust-lang/rust#106745, the format_args!() builtin macro expands to a special ast::FormatArgs AST node, which contains nothing that is specific to the standard library's fmt::Arguments implementation. During AST lowering, this node is expanded into HIR that is specific to how fmt::Arguments is implemented, and will change in the (near) future.)

Clippy's format_args lints are currently done in late passes that use the HIR. They should be changed to use the information from ast::FormatArgs instead. (Ideally by making them early passes, but that might not always be possible.)

This is part of rust-lang/rust#99012

Lints to migrate:
@m-ou-se
Copy link
Member Author

m-ou-se commented Jan 26, 2023

cc @Alexendoo @nyurik

@Alexendoo
Copy link
Member

Nice to see it merged, I'll take a look some time in the next few days

@nyurik
Copy link
Contributor

nyurik commented Jan 26, 2023

Congratulations on merging!!! It only took 2 (?) weeks - kinda fast :) I would be very happy to participate, but don't have as good of the AST/HIR understanding as @Alexendoo, but will be happy to review and participate as much as possible.

This was referenced Jan 27, 2023
Open
Merged
Merged
@m-ou-se
Copy link
Member Author

m-ou-se commented Feb 1, 2023

I've added a to do list above with the list of lints to migrate.

I'll see if I can migrate some of them soon, because this issue is now blocking rust-lang/rust#106824

@Alexendoo
Copy link
Member

I've got write.rs mostly done, which is

PRINT_WITH_NEWLINE,
PRINTLN_EMPTY_STRING,
PRINT_STDOUT,
PRINT_STDERR,
USE_DEBUG,
PRINT_LITERAL,
WRITE_WITH_NEWLINE,
WRITELN_EMPTY_STRING,
WRITE_LITERAL,

I'll open a PR for that soon-ish, just figuring out the nicest way of carrying over the AST parts in a way that's easily shared with other modules

@m-ou-se
Copy link
Member Author

m-ou-se commented Feb 1, 2023

Nice!

@Alexendoo Alexendoo mentioned this issue Feb 1, 2023
Merged
@Alexendoo
Copy link
Member

Opened ☝️

@m-ou-se m-ou-se mentioned this issue Feb 1, 2023
Merged
bors added a commit that referenced this issue Feb 2, 2023
@bors
Auto merge of #10276 - m-ou-se:manual-assert, r=Alexendoo
006a4cc 
Don't depend on FormatArgsExpn in ManualAssert.

Part of #10233

changelog: none
@m-ou-se
Copy link
Member Author

m-ou-se commented Feb 22, 2023

Is there anything I can do to move this forward? I think most (all) of the remaining lints are blocked on #10275?

bors added a commit that referenced this issue Mar 6, 2023
@bors
Auto merge of #10275 - Alexendoo:format-args-ast, r=flip1995
717577f 
Migrate `write.rs` to `rustc_ast::FormatArgs`

changelog: none

Part 1 of #10233

The additions to `clippy_utils` are the main novelty of this PR, there's no removals yet since other parts still rely on `FormatArgsExpn`

The changes to `write.rs` itself are relatively straightforward this time around, as there's no lints in it that rely on type checking format params

r? `@flip1995`
bors added a commit that referenced this issue Mar 10, 2023
@bors
Auto merge of #10275 - Alexendoo:format-args-ast, r=flip1995
3c06e0b 
Migrate `write.rs` to `rustc_ast::FormatArgs`

changelog: none

Part 1 of #10233

The additions to `clippy_utils` are the main novelty of this PR, there's no removals yet since other parts still rely on `FormatArgsExpn`

The changes to `write.rs` itself are relatively straightforward this time around, as there's no lints in it that rely on type checking format params

r? `@flip1995`
@Alexendoo
Copy link
Member

I'll take a look at the FormatArgs ones next

@nyurik
Copy link
Contributor

nyurik commented Mar 10, 2023

@Alexendoo are you address all of format-args users in rust-clippy? Anything I can help with?

@Alexendoo
Copy link
Member

For the ones that don't need access to any hir Exprs, or something else missing, feel free to migrate them

I'll keep going until it's all migrated, hopefully after FormatArgs and Write they'll be fairly straight forward

@Alexendoo Alexendoo mentioned this issue Mar 11, 2023
Merged
@Alexendoo
Copy link
Member

Opened: #10484

bors added a commit that referenced this issue Mar 28, 2023
@bors
Auto merge of #10484 - Alexendoo:format-args-ast-2, r=Manishearth
84e42fb 
Migrate `format_args.rs` to `rustc_ast::FormatArgs`

changelog: none

Part of #10233

Empty precision specifiers are no longer linted as the span for that isn't present in [`FormatOptions`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_ast/format/struct.FormatOptions.html)

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

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

r? `@flip1995`
@Alexendoo Alexendoo mentioned this issue Mar 28, 2023
Merged
@Alexendoo
Copy link
Member

There we are, #10561 takes care of the last bunch and was a bit more straight forward than the others

@bors bors closed this as completed in 58fb801 Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Replace remaining usage of FormatArgsExpn Alexendoo/rust-clippy
3 participants
@m-ou-se @nyurik @Alexendoo