immutable while condition lint should not trigger on constants

[oli-obk]

cc @kimsnj

currently while false {} triggers the lint, while SOME_CONST {} would also trigger it, neither of them should. The condition should contain some local in order for the lint to trigger.

[kimsnj]

Ooops! I initially had a "has_var" check that I must have removed while moving code to "visitor".

But what would be the usecase for while SOME_CONST {} ?
Isn't it still worth it to trigger the lint in this case?

[oli-obk]

@kimsnj I'm not 100% sure on what we should be doing here. Because you are catching

let x = false;
while x {}

Which is absolutely correct, the condition is useless because it cannot change, but maybe the user meant to iterate if some condition is met, but not otherwise, and the loop will terminate by other means.

"Cleaner" would be if x { loop {} }.

I don't really know how to move forward from here.

[kimsnj]

@oli-obk We could push the reasoning a bit further…

let x = 5;
while x > 1 {}

Should the above snippet be caught or not?
What should be a tolerably complex condition that could be skipped.

Maybe we could split the lint in 2 diffrent ones:

• Constant condition:
  That covers above cases (both literals and Constants).
  Such a lint could be considered part of "clippy_pedantic".
  I realized that there seems to be a const evaluator (in Clippy or rustc?) used in some lints.
  If we manage to evaluate this condition, we could adapt the warning to say wether it's an infinite loop or dead code.

• Unused mutable variable:
  The second part of the lint detects if a mutable var is not used immutably in the body.
  This one could be kept as part of standard lints.

Not directly related to this (as it might actually impact both), what I find a bit lacking in this lint is that cases such as:

let mut vec: Vec::new();
while vec.len() > 0 {
	println!("Hello world");
}

are note covered.
I guess it's a pretty common condition.
I don't know to which extent we could whitelist some methods implemented within some modules such as alloc::vec, std::collections, core::option and core::result

[oli-obk]

That specific one results in

warning: length comparison to zero
 --> src/main.rs:4:11
  |
4 |     while vec.len() > 0 {
  |           ^^^^^^^^^^^^^ help: using `is_empty` is more concise: `!vec.is_empty()`

so I'm not worried.

but I see your point. If we allowed immutable method/function calls and added a check for interior mutability then those cases should be covered, too.

We don't need to be perfect, just without false positives ;)

[oli-obk]

let x = 5;
while x > 1 {}

should be linted, because x is never mentioned in the body.

The current lint message could be adjusted to might lead to infinite loops or never running loops

[kimsnj]

Actually checking that the method only takes its argument immutably wouldn't be sufficient (e.g. generate a random number / reads from file …). Thus having some white-listed modules for which we do not directly bail.

[oli-obk]

oh... right. we'd need purity... well... let's just stay with the obvious numeric cases. And yea, just throw the const evaluator on it. if it returns anything, don't lint.