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

Temporarily downgrade uninlined_format_args (to pedantic or nursery) #10087

Closed
Tracked by #79
Rudxain opened this issue Dec 15, 2022 · 30 comments · Fixed by #10265
Closed
Tracked by #79

Temporarily downgrade uninlined_format_args (to pedantic or nursery) #10087

Rudxain opened this issue Dec 15, 2022 · 30 comments · Fixed by #10265

Comments

@Rudxain
Copy link
Contributor

Rudxain commented Dec 15, 2022

Description

Since format_args_capture is still not supported by rust-analyzer, suggesting to inline vars is actually a bad idea, because it makes refactoring harder (devs must manually rename vars).

I suggest we move it to the restriction group while that issue is still open.

Version

No response

Additional Labels

No response

@kraktus
Copy link
Contributor

kraktus commented Dec 16, 2022

It seems strange to degrade it for every clippy users, when it’s a RA-issue. The logic would be that they set up a workaround on their side.

@Rudxain
Copy link
Contributor Author

Rudxain commented Dec 16, 2022

The logic would be that they set up a workaround on their side

Fair enough. But it seems that may take a while, possibly months, perhaps 2 years at most

@xFrednet
Copy link
Member

Macros are not the easiest when it comes to static analysis. Moving the lint to restriction is too much IMO, but we could maybe move to pedantic. This was a lint, where the correct lint level was hard to decide. There was a Zulip discussion about this: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/.60uninlined_format_args.60.20category/near/306306974

I think it'd make sense to keep the lint as pedantic until ra supports this feature better. I'll ask what the others think :)

@xFrednet xFrednet added the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Dec 17, 2022
@nyurik
Copy link
Contributor

nyurik commented Dec 22, 2022

I would rather not change the group - the lint is already reduced from its maximum capacity (it doesn't inline the mixed cases unless there is a setting enabling it). I would love to help (time permitting) for it to be resolved on RA side .. maybe? Might be an interesting experiment

@nyurik
Copy link
Contributor

nyurik commented Jan 16, 2023

Per zulip discussion, we decided not to do this.

@Rudxain
Copy link
Contributor Author

Rudxain commented Jan 16, 2023

Per zulip discussion, we decided not to do this.

Ok, I'll close the issue for now

@Rudxain Rudxain closed this as not planned Won't fix, can't repro, duplicate, stale Jan 16, 2023
@Alexendoo Alexendoo removed the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Jan 24, 2023
@jeff-hiner
Copy link

The latest Rust update was incredibly disruptive to my team, as we had to go through 27 different crates in our workspace to turn this off. rust-analyzer can't introspect format strings and tokenize these variable names, which completely breaks our debug flow-- I can't ctrl-f for log line output, then ctrl-click the offending variable.

While this is arguably a shortcoming in rust-analyzer, that doesn't help those of us staying on stable to try to minimize disruptions to our build and debugging workflows. Please reconsider moving this to pedantic.

@Manishearth
Copy link
Member

Manishearth commented Jan 26, 2023

Hmm, the reason we originally decided against this was that:

  • the lint had changed/improved since it was originally a problem
  • some of the people initially involved had changed their minds

It's clearly enough of a problem for people still; we should revisit this.

In general Clippy's policy is that needing to put allows in your code is not a huge issue, but I think rust-analyzer is a big enough factor here that we should try to be consistent with it.

cc @xFrednet @nyurik @Alexendoo as people who were involved in that decision

@nyurik
Copy link
Contributor

nyurik commented Jan 26, 2023

My understanding, which might be incorrect, is that while Rust-analyzer cannot handle inlined arguments yet, it is not directly related to clippy -- i.e. someone can still write code with inlined args, or fix existing code with cargo clippy --fix. Moreover, the rust-analyzer code itself has had all arguments inlined, so assuming the team is dogfooding their own code, it seem to work ok for them.

@jeff-hiner you said you had to change all your crates - did you essentially just add a #![allow(clippy::uninlined_format_args)] to each one? Is it because rust analyzer automatically applies clippy fixes, or some other issue?

One thing I obviously cannot measure is the "positive" vs "negative" - i.e. the number of those affected by this issue vs those who benefit from it, but are silent. Rust analyzer is a big player in the community, so we clearly should pay attention to it.

One other thing -- if we move it back to pedantic/restricted, I feel it would serve very limited purpose because by the time it rolls out to the users, 99% cases would have been fixed, so it won't have much affect. At the same time, having it on will encourage this issue to be fixed faster in all the relevant tools (I would love to help with that btw).

@CAD97
Copy link
Contributor

CAD97 commented Jan 26, 2023

I believe this comes down to the "correct" solution being something along the lines of rust-lang/cargo#5034; being able to set lint levels at a workspace level rather than having to set it in multiple individual crates.

In the medium term, downgrading to pedantic could be a reasonable mitigation... though since it doesn't really justify a point release on its own, it's likely the earliest such a downlevel would ship would be with 1.68. After the lint's been on for six weeks, most projects will have either disabled the lint or adopted it, so the benefit of the downlevel mitigation is significantly reduced.

If the "had" is due to blocking CI on warnings,

a. A CI warning gate should be on a pinned toolchain version, so that CI doesn't fail for reasons unrelated to local changes.
b. The CI can use -Aclippy::uninlined_format_args to globally allow the lint and not block CI on it. (It would remain on locally.)

If r-a lacking support for inlined format arguments is a significant deal that we should be worrying about, then you'd probably also want to enable a restriction lint to forbid them; only using them sometimes is the worst of both worlds.

@jeff-hiner
Copy link

@nyurik In our case we gate successful builds on both zero warnings and passing cargo clippy checks as part of our CI/CD (because what it enforces generally makes the code better!) So when we update our CI Rust version (which we do generally pin) we generally have to apply some fixes to get it to build cleanly again. This churn is expected, and several times a review of the lint fixes has exposed real issues.

For this specific lint, applying the fix caused the aforementioned issues with rust-analyzer so we needed to opt out for the entire workspace. Definitely rust-lang/cargo#5034 made this a lot more painful than it needed to be. We wound up adding rustflags to both ./cargo/config and our CI scripts to cover everything. But it's a bit awkward as we already have rustflags set for other platform-specific issues. It's also uncomfortable keeping this as anything other than a temporary workaround because this type of "fix" encourages team members to opt out of lints entirely instead of fixing them.

We generally instruct team members to keep their own toolchain updated to stable, and the local VSCode configured to show clippy warnings. This is so that they don't push something that looks clean locally but fails CI. As soon as they updated their Rust version the new lint threw 250+ warnings, all of which need to be either fixed or explicitly ignored. Without a clean local build real warnings get buried in the noise. So you do have to set this locally as well, not just in CI.

As for the last point, forbidding inlined format arguments would be fine for consistency, at least in our case. That said we haven't needed it yet because nobody pushing code even knew about them. I personally feel that mandating them via a default breaking lint is too sharply opinionated (especially considering the rust-analyzer break), which is why I'd suggest they fit better as an opt-in pedantic.

@CAD97
Copy link
Contributor

CAD97 commented Jan 26, 2023

Just for clarity,

  • New lints are explicitly allowed as nonbreaking, even if they're error-by-default.
  • This especially applies to clippy, which is an opinionated linting tool. Whereas rustc lints aim to be essentially free of false positives, clippy expects you to allow lints where they don't apply.
  • You can add -Aclippy::lint to the command line used by rust-analyzer to run clippy to allow the lint globally in the IDE. If you set rust-analyzer.check.extraArgs in the workspace settings, this is a single change that applies to every checkout of the workspace. (It does not impact CLI cargo clippy, though, just the warnings in the IDE.)
    • (If you set rust-analyzer.check.command to clippy, it doesn't matter what the user configuration is, the IDE will show clippy warnings!)
  • If desired, you can use toolchain.toml to ensure everyone's using the same toolchain version that's being used in CI.

I don't disagree that the rust-analyzer limitation w.r.t. inlined format arguments is a motivation to delay raising clippy::uninlined_format_args to warn-by-default, but I do question the benefit of lowering the lint back to allow-by-default now that it's been released as warn-by-default.

Rather, I think it should be taken as motivation to raise the priority of rust-lang/cargo#5034 a bit. It was a 2021 clippy roadmap item, apparently.

There's also the 3rd party cargo-cranky if you don't mind using a 3rd party interface. FWIW, Embark Studios is using .cargo/config.toml with a [target.'cfg(all())'] rustflags = ... section without significant issue since late 2021.

@seanmonstar
Copy link

An additional concern that affects me is that having clippy warn me to use new things conflicts with trying to keep older MSRV support. Effectively this lint is suggesting I require a new compiler, when the existing code was working perfectly fine.

I can add allows, but in the older compiler the lint will be unrecognized.

@CAD97
Copy link
Contributor

CAD97 commented Jan 27, 2023

You can set an msrv tag in clippy.toml, and clippy won't suggest any lints which require a more recent toolchain version than specified.

@Manishearth
Copy link
Member

Yep, this has been a solved problem for years, if this lint doesn't support that it is a bug and needs to be fixed

@nyurik
Copy link
Contributor

nyurik commented Jan 27, 2023

This lint is aware of the MSRV:

if self.msrv.meets(msrvs::FORMAT_ARGS_CAPTURE) {

@dureuill
Copy link

dureuill commented Jan 30, 2023

Hello all 👋,

we (meilisearch) are affected by this issue as well: meilisearch/meilisearch#3434

What's more, we discovered that the unlined_format_args seems to ignore the Rust edition, which changes the behavior of format args in e.g. panic! or assert! messages.

The following:

  1. cargo new eager_clippy --edition 2018
  2. Replace eager_clippy/src/main.rs with the following:
    fn main() {
        let x = 42;
        assert!(x != 42, "x != {}", x);
    }
  3. cargo run yields: thread 'main' panicked at 'x != 42', src/main.rs:3:5
  4. Run cargo clippy --fix --allow-dirty (--allow-dirty to accommodate for the newly created .gitignore etc. files)
  5. cargo run now prints: thread 'main' panicked at 'x != {x}', src/main.rs:3:5

Note that even if we weren't using cargo clippy --fix, we'd have to disable the uninlined_format_args at least for our 2018 edition crates so they don't emit a warning that, once corrected, would make the code incorrect (and cause a rustc warning).

If this lint could be downgraded to restriction or at least this particular issue fixed, it would be great for us! Meanwhile, we're considering the workaround of adding msrv= "1.57.0" to a clippy.toml.

@nyurik
Copy link
Contributor

nyurik commented Jan 30, 2023

@dureuill it seems this was recently fixed, and might not have made it into the release.

if call_site.edition() < Edition2021 && (is_panic(cx, def_id) || is_assert_macro(cx, def_id)) {
// panic!, assert!, and debug_assert! before 2021 edition considers a single string argument as
// non-format
return;
}

It was merged as part of #10126 by @flip1995 in december a month ago, but I can't find the summary, so not even sure if it made it into the release or not.

@Manishearth
Copy link
Member

If this lint could be downgraded to restriction

Restriction lints are emphatically not for this purpose.

The current proposal is nursery or pedantic.

Meanwhile, we're considering the workaround of adding msrv= "1.57.0" to a clippy.toml.

FWIW, clippy is intended to be used with a liberal sprinkling of global and local allows, so I'd mostly just recommend doing that.

We're hoping to eventually have a way of doing that from Cargo config, maybe also in a way that lets you say "no lints from versions beyond XXX" without having to restrict fix MSRV

@nyurik
Copy link
Contributor

nyurik commented Jan 30, 2023

To sum up up to this point, there are two main issues why this lint causes problems:

  • pre-2021 edition assert! is not supported correctly -- clearly a bug (my sincere apologies!) -- this is already fixed and should be released in the next version, unless we do a point release (doubtful) -- so this is a moot point
  • rust-analyzer is not yet supporting format args - support is planned, but ETA is unknown. This mainly affects those who want to use refactoring to rename/inline/find-usage of a variable in a function.

The rust-analyzer issue is significant, but might also be moot because

  • by the next release, most affected projects will already solve it with a project-wide attribute - so little value in the change, while it does benefit all other projects
  • developers have been using captured format args for over a year in stable Rust, regardless of the rust-analyzer or clippy - and there is no way to restrict a project to NOT use captured args - so projects will likely end up with a mix of captured and non-captured args, even without applying the lint
  • rust-analyzer will fix it sooner or later, likely sooner due to the widespread use of captured args

So while I agree this is annoying to those affected (I am partially affected by that too), at this stage I honestly do not see any significant benefit of changing it, unless my reasons are incorrect or incomplete. What do you think?

@Manishearth
Copy link
Member

by the next release, most affected projects will already solve it with a project-wide attribute - so little value in the change, while it does benefit all other projects

I'm not sure if we can rely on this because (a) new projects are still a thing and (b) we can uplift if we really want.

There's another argument in this space, that projects really don't like applying allow()s, because they prefer to be 100% clippy clean (or because it's effort). As far as clippy cleanliness is concerned I do not tend to give much weight to the general argument myself because Clippy's design is deliberately assuming most projects will sprinkle in some global allow()s, and it's fine as long as you don't have a large swath of projects allow()ing the same lint.

(indeed, Clippy cannot exist without this policy: if the entire community were able to come to alignment on a lint, it could reasonably be uplifted into rustc, which has this higher bar.)

So in general my reaction is typically "just allow() it". However, given the growing ubiquity of rust-analyzer, there's a good chance this means saying that to a large swath of projects, which is roughly where the line is for Clippy allow-by-default. I'm not sure yet if this issue crosses that line.

There's also the effort argument: but I'd like to make clippy.toml happen so that it's never a problem again.

so projects will likely end up with a mix of captured and non-captured args, even without applying the lint

There's a reasonable argument to be made that that should be a restriction lint. I think if someone proposed it we would probably accept it.

rust-analyzer will fix it sooner or later, likely sooner due to the widespread use of captured args

It's unclear if this is "sooner" or "later".

cc @rust-lang/clippy

@irevoire
Copy link

irevoire commented Jan 30, 2023

I'm not sure if we can rely on this because (a) new projects are still a thing

Without even talking about new projects, even new crates in the same workspace are affected by this change and will need to sprinkle more allows 😩

but I'd like to make clippy.toml happen so that it's never a problem again.

💯

@Manishearth
Copy link
Member

Without even talking about new projects, even new crates in the same workspace are affected by this change and will need to sprinkle more allows 😩

Generally we expect projects like this to standardize lint-allow boilerplate, much like most of them standardize license header boilerplate already.

@lnicola
Copy link
Member

lnicola commented Jan 30, 2023

Generally we expect projects like this to standardize lint-allow boilerplate, much like most of them standardize license header boilerplate already.

I personally don't like those. In rust-analyzer (which has quite a bunch of crates) there's a cargo lint alias that runs Clippy with a couple of "known reasonable" lints. We should probably expand that set, though.

@Manishearth
Copy link
Member

Yeah, doing it at the CLI level works as well.

@Veykril

This comment was marked as outdated.

@lnicola

This comment was marked as outdated.

@Manishearth Manishearth changed the title Downgrade uninlined_format_args to restriction (temporarily) Temporarily downgrade uninlined_format_args (to pedantic or nursery) Jan 30, 2023
@Manishearth
Copy link
Member

Opened #10265

I'm not going to push for uplifting this myself, but folks are free to ask upstream to uplift to beta or even a point release; I'm not opposed to doing one.

bors bot added a commit to meilisearch/meilisearch that referenced this issue Jan 31, 2023
3434: Make clippy happy for Rust 1.67, allow `uninlined_format_args` r=Kerollmops a=dureuill

# Pull Request

This PR allows `uninlined_format_args` in CI for clippy.

This is due to rust-lang/rust-clippy#10087, which in particular has correctness issues wrt edition 2018 crates, and is a big change altogether. rust-lang/rust-clippy#10265 is already open in order to change the category of this lint to "pedantic", meaning that if this latter PR merges, a future Rust release will accept our code unmodified wrt uninlined format arguments.

As a result, this PR introduces the following changes:

1. Allow `uninlined_format_args` in the clippy command in CI.
2. Use rewind rather than seek(0)
3. Remove lifetimes that clippy deems needless.

Co-authored-by: Louis Dureuil <[email protected]>
bors added a commit that referenced this issue Jan 31, 2023
Mark uninlined_format_args as pedantic

Fixes #10087

We should restore this when rust-analyzer support gets better. Worth filing an issue to track.
@bors bors closed this as completed in 2880dcc Jan 31, 2023
@Manishearth
Copy link
Member

This is being uplifted in rust-lang/rust#107743 (comment)

@lnicola
Copy link
Member

lnicola commented Feb 10, 2023

Moreover, the rust-analyzer code itself has had all rust-lang/rust-analyzer#13835, so assuming the team is dogfooding their own code, it seem to work ok for them.

I wanted to let this pass, but I keep seeing this argument and think it's disingenuous. RA uses inline format args because you (and then another drive-by contributor) sent a pull request for it. We merged it knowing full well that rename doesn't work with these, but all in all, we're probably well used-to working around buggy IDE support. Us willing to merge a first-time contributor's work of running cargo clippy --fix and filing a PR, doesn't mean that every Rust developer out there is supposed to be fine with this lint, IDE limitations or not.

Personally, I find the CI breakage everywhere much more annoying, especially since cargo clippy --fix doesn't fix build scripts, but cargo clippy complains about them. But the lint itself is pretty much fine, unlike others which have a lot of false positives. I'd even prefer it to be in rustfmt (which would make the transition much worse for everyone, of course).

bors bot added a commit to meilisearch/meilisearch that referenced this issue Apr 26, 2023
3464: Remove CLI changes for clippy r=curquiza a=dureuill

# Pull Request

## Related issue

Reverts #3434, which was linked to rust-lang/rust-clippy#10087, as putting the lint in the pedantic group [is being uplifted to Rust 1.67.1](rust-lang/rust#107743 (comment)) (my thanks to everyone involved in this work 🎉).

## Motivation

- Using "standard issue" clippy in the CI spares our contributors and us from knowing/remembering to add the lint when running clippy locally
- In particular, spares us from configuring tools like rust-analyzer to take the lint into account.
- Should this lint come back in another form in the future, we won't blindly ignore it, and we will be able to reassess it, which will be good wrt writing idiomatic Rust. By the time this occurs, lints might be configurable through `clippy.toml` too, which would make disabling one globally much more convenient if needs be.

## Note

We should wait for the release of Rust 1.67.1 and its propagation to our CI before merging this. The PR won't pass CI before this.


Co-authored-by: Louis Dureuil <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.