When should a lint warn by default?

[sanxiyn]

In https://github.com/rust-lang/rust/pull/26684, @alexcrichton said that “any false positive on a lint means that it should not be warn-by-default”. I disagree, and I think false positives are a reason to make something a lint (so that it can be turned off), not a reason to make something an allow-by-default lint, as long as false positives are rare. I am interested in what other people think.

[llogiq]

There is kind of a grey area, it much depends on:

• the severity of the linted issue
• the overall probability of a match
• the probability of a false positive
• the runtime of the lint

I’ve even argued elsewhere that lints without false positives and zero chance that the matched code could do what was intended should be error by default, even if the severity is low. E.g. rust-clippy’s bad_bit_mask.

So from my experience, a lint should only allow by default if either

• the overall match rate is too low to make the cost of linting worthwhile, e.g. lints that apply only in specific circumstances
• the rate of false positives is too high
• the lint is too slow to run on each build

Edit: I’d also like to add that the cost of fixing the issue should probably also figure into it. If fixing the problem (regardless of false positives) is trivial and carries no overhead, the lint may well be warn. The problematic case is when a lint catches legitimate code that cannot be fixed easily or not without inducing compile- or runtime-overhead.

[alexcrichton]

I personally feel that if I’m required to write #[allow] for something where I’m not violating what the lint is linting for (e.g. a false positive) that the lint is incorrect and/or should be turned off by default. To echo what I said on the thread as well, I’ve found that over time projects end up gaining dozens of little #[allow] directives that end up turning into spaghetti because each lint we have has very few false positives but the union of all the false positives means that you’re going to have a lot.

[sanxiyn]

Is the problem with #[allow] in particular, or false positives in general?

For example, dead_code lint is warn-by-default, but some of dead fields it detects are legitimate. One example is _marker field of std::thread::JoinGuard. Since the lint skips things named with a leading underscore, _marker field is equivalent to marker field with #[allow(dead_code)]. Should dead_code lint be allow-by-default, or it’s fine because you don’t need to write #[allow(dead_code)]?

Another example is unused_must_use lint (also warn-by-default), which can be evaded by let _ = .... For the current lint in question, unused_type_parameters lint, it would be easy to skip type parameters named with a leading underscore, like _T. Then you would never need to write #[allow(unused_type_parameters)].

[alexcrichton]

Adding #[allow] is a symptom, but the root of the problem is that there are too many false positives.

All lint warnings should have an immediately actionable way to make the lint go away which doesn’t involve adding #[allow] everywhere. For example dead fields/variables can have an underscore in front to quickly make the error go away (and conventionally say it’s not going to be used). Other kinds of dead_code are typically legitimately dead code and probably need to be acknowledged.

The unused_must_use lint is the same way where it has a clear an actionable way to handle it. I don’t think I’ve ever seen a program with #[allow(unused_must_use)] in it.

it would be easy to skip type parameters named with

Sure, and this may be a reasonable course of action to take, but I feel like that may be taking “ignore things with leading underscores” a bit to the extreme.

[sanxiyn]

It seems to me that @alexcrichton’s position is that false positives are okay, as long as there is “an immediately actionalbe way to handle it”. For some reasons, adding #[allow], which is immeidately actionable, is not to be included. Is that a fair summary?

I think I understand the position, but I don’t think it is a sane position. I am waiting for other people to comment.

[stdsync]

I think Alex’ position is quite reasonable. A linter with lots of false positive-generating rules switched on by default is quite an obnoxious thing (looking at you, Pylint).

[sanxiyn]

We are talking about lints with “any false positives”, not “lots of false positives”. I agree lints with lots of false positives should not warn by default. (Does anyone disagree?)

[stdsync]

We are talking about lints with “any false positives”, not “lots of false positives”.

I understand that. And the best way to avoid “lots of false positives” (which is a subjective category anyway) is to avoid “any false positives” (which is an objective category).

[Manishearth]

I think in these cases using underscores or allow is basically acknowledging a pitfall. For example, with must_use there’s generally a pitfall out there it helps you avoid, and the underscore or allow is just a way of telling the compiler to trust the programmer (kind of like unsafe)

[alexcrichton]

It seems to me that @alexcrichton’s position is that false positives are okay, as long as there is “an immediately actionalbe way to handle it”.

I wouldn’t summarize my point quite like this, I don’t consider any false positive OK and #[allow] is not an acceptable way to handle a warning in my opinion. The examples I gave I do not consider as false positives.

We are talking about lints with “any false positives”,

The problem with this is that we have dozens of lints, so if they all have just a few false positives then it adds up very very quickly.

[seanmonstar]

FWIW, to provide some data: hyper has only 1 required instance of allow (specifically #[allow(non_camel_case_types)]). There’s 2 others, but they’re simply to let the test module to be lazy.

Also, I instead opt for all warnings to be errors, via #![deny(warnings)].

[bluss]

Also, I instead opt for all warnings to be errors, via #![deny(warnings)].

Then you run the risk of not building at all with a new Rust version that introduces new warnings. Of course, being forced to fix those might be a net win.

[seanmonstar]

Then you run the risk of not building at all with a new Rust version that introduces new warnings. Of course, being forced to fix those might be a net win.

Indeed, I actually use #[cfg_attr(test, deny(warnings))], so that a build doesn’t break, but CI would catch it. And I’d rather be forced to fix those errors when developing (testing).

[llogiq]

If this is the case, fixing the lint to get rid of false positives would be the correct solution.

My experience from Java has been quite different. In a 100kloc codebase, I have about 70 findbugs warnings, 30 of which are false positives, all of which fall into one error class that we just disable for the offending package.

[system]

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.