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

Apply clippy::uninlined_format_args fixes #102804

Closed
wants to merge 1 commit into from

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented Oct 8, 2022

This is the result of running clippy::uninlined_format_args lint. Currently the lint is in pedantic, but there are plans/hopes to move it to style.

How can I add this specific lint to the current build/CI tools, or is this not possible? It would be ideal not to make the lint as the default style and then end up with hundreds of changes in an unrelated PR due to CI requirements.

Locally, I ran this to generate the changes. Not ideal because it has produced a number of compilation errors.

rustup run nightly cargo clippy --fix --no-deps --allow-dirty -- -A clippy::all -W clippy::uninlined_format_args

@rust-highfive
Copy link
Collaborator

r? @jyn514

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

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

  • These commits modify submodules.
  • These commits modify compiler targets. (See the Target Tier Policy.)

@rustbot rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-rustdoc-json Area: Rustdoc JSON backend A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 8, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 8, 2022
@rustbot
Copy link
Collaborator

rustbot commented Oct 8, 2022

The Miri subtree was changed

cc @rust-lang/miri

rustc_macros::diagnostics was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

Changes rustc_apfloat. rustc_apfloat is currently in limbo and you almost certainly don't want to change it (see #55993).

cc @eddyb

Some changes occurred in src/tools/cargo

cc @ehuss

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@jyn514
Copy link
Member

jyn514 commented Oct 8, 2022

This is not a good idea. It creates a lot of churn, has dubious benefit, and we have a policy of not enforcing clippy lints; if they're important enough to be gated in CI, the lints should be part of rustc proper.

How can I add this specific lint to the current build/CI tools, or is this not possible

There isn't a way; x.py clippy isn't officially supported.

@jyn514 jyn514 closed this Oct 8, 2022
@nyurik nyurik deleted the inline-fmt-arg branch October 8, 2022 16:35
@nyurik
Copy link
Contributor Author

nyurik commented Oct 8, 2022

@jyn514 , I didn't know about the policy, thanks for the explanation :)

@est31
Copy link
Member

est31 commented Oct 8, 2022

I think in the past, pull requests to fix clippy lints have been accepted, at least to compiler/. library/ has a much stricter policy to not allow clippy fixes. IDK what the policy is for src/bootstrap. I do think that this PR is improving code, just as #100589 has improved code, not from the "fix clippy" side of things but because inline format args feel nicer to me. That being said, this PR should not be merged in the current form: subtrees should only be modified if there is a pressing reason to modify them from rustc's side, e.g. build failures, and submodules should be modified even more rarely. So those two categories should be excluded, in addition to changes to library/. Instead you can make changes to the places where they are maintained canonically, if you want. Subtrees are clippy, miri, as well as the non-llvm codegen backends (correct me if I'm wrong). This would increase your chances of being merged I think :).

Also btw x.py clippy uses clippy from beta, so this lint that currently only exists on nightly won't appear there.

Last, there is conflict potential when it comes to error due to the ongoing migration to translatable error messages. In many ways I think the new API can be improved (structs are bad design IMO), but there are still PRs open to migrate away from format!() usage to fluent. Thus I think it would be better to exclude error messages from possible future PRs. Maybe even wait with compiler/ changes until the migration is over, idk.

@jyn514
Copy link
Member

jyn514 commented Oct 8, 2022

I think in the past, pull requests to fix clippy lints have been accepted, at least to compiler/.

Yes, but they've been accepted on their own merit, not because they fix a lint. I don't think these changes stand on their own; if nothing else the churn is a massive pain to review, and it makes git blame less useful in the future.

Put another way, I think it's fine to discover issues because of a lint, but not to decide they're an issue in the first place because of a lint.

@nyurik
Copy link
Contributor Author

nyurik commented Oct 9, 2022

@jyn514 I think "code is easier to read" is a benefit in of itself. Granted that it should be trivial to review, and this PR is clearly too messy to merge (my bad). Assuming it is easier to read after the cleanup, I think it would be good to start a slow process of cleaning up the code, one easy-to-review chunk at a time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-rustdoc-json Area: Rustdoc JSON backend A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants