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

Deal with the new clippy lint: uninlined_format_args #643

Closed
cvybhu opened this issue Feb 5, 2023 · 4 comments · Fixed by #945
Closed

Deal with the new clippy lint: uninlined_format_args #643

cvybhu opened this issue Feb 5, 2023 · 4 comments · Fixed by #945
Assignees

Comments

@cvybhu
Copy link
Contributor

cvybhu commented Feb 5, 2023

New version of clippy introduced the uninlined_format_args lint which recommends putting variable names directly into format strings.

In #639 we decided that the new lint should be disabled for now, because rust-analyzer has limited support for the code that clippy proposes.
The lint was disabled in CI and Makefile by adding a flag, but manually running cargo clippy would still throw a ton of errors.
Having to always add this flag is an unneeded annoyance for developers.

Maybe we could disable the lint on the level of the whole crate?
I think there's a special syntax for macros that are supposed to cover the whole crate, something like #![allow(clippy::new_ret_no_self)] (saw it here).

I also created this issue to track the problem and remeber to make the code clippy compliant once rust-analyzer is able to handle everything.

@piodul
Copy link
Collaborator

piodul commented Feb 6, 2023

New version of clippy introduced the uninlined_format_args lint which recommends putting variable names directly into format strings.

In #639 we decided that the new lint should be disabled for now, because rust-analyzer has limited support for the code that clippy proposes. The lint was disabled in CI and Makefile by adding a flag, but manually running cargo clippy would still throw a ton of errors. Having to always add this flag is an unneeded annoyance for developers.

You can set up the RUSTFLAGS environment variable and not have to worry about it:
export RUSTFLAGS="-Aclippy::uninlined_format_args"

Maybe we could disable the lint on the level of the whole crate? I think there's a special syntax for macros that are supposed to cover the whole crate, something like #![allow(clippy::new_ret_no_self)] (saw it here).

I originally didn't decide to do it like this because we actually have multiple crates: scylla, scylla-cql, scylla-proxy, every integration test and example - they all count as separate crates and need a separate allow attribute. It's not super hard work, but it sounds annoying to do something like this just to disable a single lint, and you need to remember to disable it in new examples/integration tests. Usually we avoid such problems by just fixing the new issues pointed out by clippy, but this time I think we should wait until support for renaming in rust-analyzer gets better.

That said, I'm not strongly opposed to introducing the annotations if it's just for this single lint. If you feel it will make life easier to developers, then please send a PR.

I wish there were a better way to handle this, but AFAIK there is no such way at the moment. In the future it will be possible to disable lints in Cargo.toml: rust-lang/cargo#5034

@piodul
Copy link
Collaborator

piodul commented Feb 10, 2023

Note: version 1.67.1 has been released: https://blog.rust-lang.org/2023/02/09/Rust-1.67.1.html . One of the few changes was to disable the lint. It will take some time until it is available on GitHub's ubuntu-latest, though.

@wprzytula
Copy link
Collaborator

Note: version 1.67.1 has been released: https://blog.rust-lang.org/2023/02/09/Rust-1.67.1.html . One of the few changes was to disable the lint. It will take some time until it is available on GitHub's ubuntu-latest, though.

What's the status of this?

@piodul
Copy link
Collaborator

piodul commented Apr 26, 2023

I believe the issue is not resolved yet because of this:

I also created this issue to track the problem and remeber to make the code clippy compliant once rust-analyzer is able to handle everything.

AFAIK rust-analyzer doesn't support renaming in format strings yet.

@Lorak-mmk Lorak-mmk self-assigned this Nov 15, 2023
avelanarius added a commit to avelanarius/scylla-rust-driver that referenced this issue Mar 4, 2024
This reverts commit 4964b44.

The main reason that uninlined_format_args lint was disabled in Rust 
Driver's CI/Makefile was that inlined variables in format strings had 
spotty support in rust-analyzer: rust-analyzer did not support renaming
variables inside format strings.

Fortunately, rust-analyzer recently gained support for renaming 
variables in format strings: rust-lang/rust-analyzer#16027, therefore
4964b44 can now be reverted. 

Moreover, Rust 1.67.1 downgraded uninlined_format_args to pedantic, 
meaning that it's disabled by default and we don't have to disable it
manually. If it's ever promoted back to a enforced rule by default,
we'll be ready to deal with it again (with a proper rust-analyzer 
support) - not addressing it now to avoid merge conflicts with other
bigger planned PRs.

Closes scylladb#643
avelanarius added a commit to avelanarius/scylla-rust-driver that referenced this issue Mar 4, 2024
This reverts commit 4964b44.

The main reason that uninlined_format_args lint was disabled in Rust 
Driver's CI/Makefile was that inlined variables in format strings had 
spotty support in rust-analyzer: rust-analyzer did not support renaming
variables inside format strings.

Fortunately, rust-analyzer recently gained support for renaming 
variables in format strings: rust-lang/rust-analyzer#16027, therefore
4964b44 can now be reverted. 

Moreover, Rust 1.67.1 downgraded uninlined_format_args to pedantic, 
meaning that it's disabled by default and we don't have to disable it
manually. If it's ever promoted back to an enforced rule by default,
we'll be ready to deal with it again (with a proper rust-analyzer 
support) - not addressing it now to avoid merge conflicts with other
bigger planned PRs.

Closes scylladb#643
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.

4 participants