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

It is impossible to understand from the readme file how to supress lints using clippy.toml #3164

Closed
Tracked by #22
iddm opened this issue Sep 11, 2018 · 28 comments
Closed
Tracked by #22

Comments

@iddm
Copy link
Contributor

iddm commented Sep 11, 2018

It is impossible to understand from the readme file how to disable some lints using clippy.toml, it seems to be explained only for nightly compiler and touching the code. I suggest to add an example of disabling some lints using clippy.toml for rust stable (let's take single_match for example) into the readme.

@iddm iddm changed the title Impossible to understand from readme how to supress lints using clippy.toml It is impossible to understand from the readme file how to supress lints using clippy.toml Sep 11, 2018
@flip1995
Copy link
Member

It is not possible to disable lints in the clippy.toml. You have to either add #[allow(clippy::lint_name/group)] in front of the item you want to disable the lint for or #![allow(clippy::lint_name/group)] to the beginning of your lib.rs file.

For stable see also: #3159

clippy.toml will probably be deprecated with Clippy 1.0. See rust-lang/rfcs#2476 (comment) and #3013

@iddm
Copy link
Contributor Author

iddm commented Sep 11, 2018

So I am obligated either to have a feature called "cargo-clippy" for this:

#![cfg_attr(feature = "cargo-clippy", feature(tool_lints))]
#[cfg_attr(feature = "cargo-clippy", warn(clippy::single_match))]

or to use nightly. I doubt an ordinary user wants to change his code. Why can't we do that without touching the code at all? It had been okay when it was allowed to use clippy.toml. If this is going to be deprecated, why not to also leave us with something that can be treated as equal replacement?

Also, I still don't see this information in the README file which every user sees and where it can only see this information. My opinion is that it should be added there. Don't you think the opposite, do you, @flip1995 ?

@phansch
Copy link
Member

phansch commented Sep 11, 2018

@vityafx Regarding the replacement for the clippy.toml, there is work being done to manage lints via Cargo.toml. The lint configuration is being moved to cargo because that allows users to manage rustc lints as well and it's one less configuration file to manage.

@iddm
Copy link
Contributor Author

iddm commented Sep 11, 2018

@phansch The cargo will always warn that there are unused sections in the Cargo.toml if we put something unknown to cargo. Also, this is still WIP while the old way can not be used already, sounds unfair.

@mati865
Copy link
Contributor

mati865 commented Sep 11, 2018

@vityafx clippy.toml from the start wasn't meant to disable lints. It just allows to configure some of them.
Until Cargo supports lints configuration there will be a way configure lints but it's not agreed yet.

@flip1995
Copy link
Member

@vityafx The cargo-clippy feature workaround is only temporary until tool_lints are stable. It will only be one line people need to add to their code:

#![cfg_attr(feature="cargo-clippy", feature(tool_lints))]

allowing or denying lints was always in the code. Also the cfg_attr(cargo-clippy, ...) attribute was always necessary for Clippy lints. We're currently moving to tool_lints to avoid needing the cargo-clippy feature. Sadly this cannot be done within a few days.


You're right about the tool_lint part missing in the README. The tool_lints change landed about a week ago and I didn't get to update the README yet. But it needs an update, everyone who wants to help with that is invited to do so!

@iddm
Copy link
Contributor Author

iddm commented Sep 12, 2018

It would be nice if we did not have to add anything to the source code. Okay, whatever, you'll not listen to me as a user.

@flip1995
Copy link
Member

Sadly this is not possible #3159 (comment). If you have a better solution how this could be implemented, feel free to suggest it or open an rustc PR.

Reminder: #2718 (comment)

@Manishearth
Copy link
Member

@vityafx I don't think clippy.toml ever supported flipping lint levels, you seem to be under the impression that we did and removed that ability. There is no "old way", the in-source attributes are the old way. The new way will eventually be through Cargo, and work is being done in that already. I'm not sure what we can do here to help you.

@iddm
Copy link
Contributor Author

iddm commented Sep 13, 2018

@Manishearth Yes, I had thought so until you told me. Anyway, this is must have feature - to be able to use clippy without touching the code, even Cargo.toml, in my opinion. Also, it is interesting to me how you are going to deal with cargo warnings about unknown fields in the Cargo.toml?

@Manishearth
Copy link
Member

We'll probably use [lints.clippy] or some such, similarly to how the clippy:: lint attributes are completely ignored by the compiler when clippy is not in use.

If you want to set lint levels on clippy without touching any code, pass -A flags to clippy itself.

@e-oz
Copy link

e-oz commented Sep 30, 2018

I tried to use Clippy, on small projects it was pleasure to use (thank you for this tool), on large codebase - unusable without turning off some lints and I definitely will not switch code to nightly just for Clippy.

@Manishearth
Copy link
Member

I'm not sure I get what you're asking for.

The tool lints stuff will stabilize soon. Even if we did implement toml stuff for Clippy that would still be nightly-only for at least as long.

Wait a couple releases, or use #![cfg_attr(clippy, allow(foo)]. There's nothing more we can do here to solve your problem, the solution for your problem is already in the release pipeline.

@e-oz
Copy link

e-oz commented Oct 2, 2018

We are asking for more detailed documentation: it's impossible to find in README how to allow/deny particular lints.

For those who found this issue, here is my example of how to turn off lints. This code is for binary and it's placed as first lines of main.rs:

#![allow(renamed_and_removed_lints)] 
#![allow(unknown_lints)]  
#![cfg_attr(feature = "cargo-clippy", allow(redundant_field_names,assign_op_pattern,cyclomatic_complexity,unreadable_literal,collapsible_if))]
#![cfg_attr(feature = "cargo-clippy", allow(toplevel_ref_arg,match_bool))]

@iddm
Copy link
Contributor Author

iddm commented Oct 2, 2018

@e-oz There is also a way without touching the code, which, I think, everyone should prefer by default:

cargo-clippy -- -A single_match

(for single_match lint).

@Manishearth
Copy link
Member

What's missing from https://github.com/rust-lang-nursery/rust-clippy/blob/master/README.md#allowingdenying-lints ? It's all there.

We just don't support it in Clippy.toml.

@iddm
Copy link
Contributor Author

iddm commented Oct 2, 2018

@Manishearth Where is the information in the README.md you have just given to us regarding -- -A single_match form? The problem is due to somewhy you think that everyone wants to put clippy stuff inside the code but I disagree with that statement. I am the one who does not want to touch the code just for adding clippy support to it. And so, reading the README.md file does not give me or anyone else information about lints disabling without touching the code, with or without clippy.toml, there must be a way.

@Manishearth
Copy link
Member

My comment was responding to @e-oz's question about the readme.

IIRC until tool_lints stabilizes -A is in flux (will only work sometimes). I'm willing to accept a PR that adds that to the readme as an alternate way to disable lints, mentioning that you may need to add clippy:: on newer compiler versions.

I never made any statement about folks wanting to put lint levels in code, that's just the general convention in the Rust community.

You're being really demanding/abrasive here, please try and be more understanding. Things are in flux right now, not everything is a deliberate decision against your use cases.

@e-oz

This comment has been minimized.

@Manishearth

This comment has been minimized.

@iddm
Copy link
Contributor Author

iddm commented Oct 2, 2018

@Manishearth Did not know that it is a sort of general convention, anyway, I don't understand what benefits I have if I need to change my code everytime I need some thing to check it.

Also, yes, you have not made any statement, I just see that there is no such idea flying around that someone doesn't want to touch the code.

Yes, I might sound offensive, I am asking for forgiveness now. I do understand that you are not obligated to do anything, but I would do that in another way, letting other people use clippy easily without touching the code, that's it. I am contributing a lot and I do understand what it means, so don't treat my words as like "I don't pay you but I want everything from you anyway". The only reason I talk to you here is because I want clippy to be easy for everyone and not requiring anything to do, or at least less things to do, especially working good without touching the code. I want to improve rust and clippy and everything related, I want to help. So, don't feel offended. Explain me where I am wrong and you won't see me again.

@Manishearth
Copy link
Member

so don't treat my words as like "I don't pay you but I want everything from you anyway"

I'm not. I'm trying to listen. However, you came in here asking for us to document a feature that didn't exist, and then got angry it didn't exist as if we'd made a deliberate decision not to support it. When to the contrary we had an explicit plan for it, but were holding off on things while stuff was in flux (and don't have the bandwidth to implement it ourselves right now).

It's possible to ask for new features without coming off as super entitled about it. It's possible to have a nice, civil discussion about new features without assuming intent from the maintainers and yelling at them. I'm not faulting you for asking for new features -- you have a right to -- I'm asking you to be nice about it.

I don't understand what benefits I have if I need to change my code everytime I need some thing to check it.

You don't have to change it every time. Leave the lint levels in the code. That's what everyone does, this way everyone on a project shares decisions on lint levels. It also lets you turn off lints for specific modules or functions.

(This is why we moved to tool_lints, if you have #![allow(clippy::foo)] in your code rustc will not complain about an unknown lint. Previously, you needed to #![cfg_attr(feature=clippy, allow(foo))] to avoid that error)

@Manishearth
Copy link
Member

I opened #3250 to mention -A / -W in the readme.

However, does my comment address your use case well enough? You only have to change the code once and leave it in, not every time you wish to run the tool. If so, I'd rather not land that pull request since -A / -W aren't really common in the rust community and I'd rather wait for Cargo.toml to support these things better.

@iddm
Copy link
Contributor Author

iddm commented Oct 2, 2018

@Manishearth #3250 is okay I think. I use -A for a year or so because I wanna leave my code untouched, I think, if a 3rd-party tool needs to access your code, it does not need you to change the code for making it to work, just my opinion. And I think clippy should also have a way to work for people right now without making them to touch the code, if we don't support disabling lints or other configuration in Cargo.toml - we must provide another way then, again, just my opinion, this is how I'd do it.

The first disadvantage of disabling lints or doing something else in the code is that it will need you to recompile everything, and also, is one more place you'll have to support in the future. For example, if lint name changes, you'll also need to change it in the code. Am I wrong here?

Another thing, imagine like one hundred of different tools like clippy - would you add a few module files with compiler cfg_attr/allow/disallow things into them just for these tools?

@Manishearth
Copy link
Member

Clippy's an official rust tool, not third-party 🙂

if we don't support disabling lints or other configuration in Cargo.toml - we must provide another way then, again, just my opinion, this is how I'd do it.

Yeah, but that's changing, I'm not sure if I want to popularize -A if it's going to become unnecessary later. Eh, might as well merge it, we can always deprecate it later.

For example, if lint name changes, you'll also need to change it in the code.

We don't really rename lints, and when we do we keep the old name as a redirect, IIRC.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 2, 2018

For example, if lint name changes, you'll also need to change it in the code. Am I wrong here?

This is the same with command line arguments and a hypothetical lints.toml. You'll have to change these. The compiler renamed lints, too, but kept the old names as a deprecated redirect.

And I think clippy should also have a way to work for people right now without making them to touch the code,

I think the main complaint I have is with the "right now". With this argument we'd never even have RFCs, FCPs and unstable features. We'd need to "move fast and break things" for every feature, which would definitely take a lot of effort on the user side

if we don't support disabling lints or other configuration in Cargo.toml - we must provide another way then, again, just my opinion, this is how I'd do it.

I don't see how editing the Cargo.toml is advantageous over editing lib.rs. With incremental compilation, changing the lint levels should be a barely noticeable slowdown. Clippy will rebuild your main library anyway, so the fact that editing the lib.rs rebuilds your library is not changeable at the moment.

@e-oz
Copy link

e-oz commented Oct 2, 2018

I don't see how editing the Cargo.toml is advantageous over editing lib.rs

Code will compile even if clippy has warnings. When you work in a team and can use Clippy with config file, you don't have to create new git branch for every Clippy change, don't have to create PR, pass code review stage. It's especially true for binaries.

@iddm
Copy link
Contributor Author

iddm commented Oct 2, 2018

I don't see how editing the Cargo.toml is advantageous over editing lib.rs. With incremental compilation, changing the lint levels should be a barely noticeable slowdown.

CI services don't benefit from incremental compilation because, I believe, in most cases they don't store intermediate artifacts, because it is nearly useless. Also, changing your code gives you a small chance of stopping it from compiling. I have recently occured in such a problem: nightly allowed something but stable not, and there were contradictions between compiler lints and clippy lints and stable compiler. So I also made an issue about adding compiler version check macro in the rust-lang repository.

But I agree with an argument that it does not matter whether I change toml file or lib.rs because if I change it then the project will need to be rebuilt anyway.

So, basically, we came to my personal opinion of using tools that are not enabled by default in rustc and installed with it, such as clippy.

We don't really rename lints, and when we do we keep the old name as a redirect, IIRC.

I am not sure about this, I think I have recently seen one lint I use renamed.

Clippy's an official rust tool, not third-party slightly_smiling_face

Does that mean that no one else can do clippy analogs or other tools which use your code? :) My idea was to imagine a situation where every tool needs changing your code when there is a chance that it could not make you changing the code.

I guess, I am out of other arguments, I got sick and so can't explain my thoughts properly, feel free to point me where I am wrong please, I will not feel offended or anything, because we always try to help each other here and I strongly believe in that.

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

No branches or pull requests

7 participants