Range with inverse bounds without negative stepping

[llogiq]

E.g. range(100, 0), but not range(100, 0).step_by(-1). Also skip if we have non-const bounds.

[Manishearth]

More concrete lint specification:

Match bare uses of x..y for y<x in a desugared for loop.

(Should we expand this to handle general cases or just do AST matching on loops?)

[llogiq]

Matching the AST is quite easy (a good portion of the code is already in bit_mask, approx_const and elsewhere) and gives us 80% of the benefit.

Using this, we should be able to catch bare uses as well as uses where the .step(_) goes in the wrong direction.

[Manishearth]

Yep!

[Manishearth]

Note: I'm tagging all the issues with an AST vs middle classification, as well as difficulty (medium is basically good second bug), to help newcomers. I'll add a CONTRIBUTING.md soon.

[llogiq]

The difficulty here is to ascertain the absence of .step_by. I notice that this is a pattern we have in other places (e.g. string_add vs. string_add_assign) – perhaps we should create some abstraction to base those lints on that doesn't involve getting the parent node? Something like a trait that impls LintPass where all visit... methods by default call a visitSub... on all AST childs, and which can be overridden? I'm not sufficiently knowledgeable in that particular area of Rust, but I'd write it with some coaching.

[Manishearth]

We don't need to. As an AST match it's simple: we look for desugared for loops (i.e. match statements) which match on ::std::iter::IntoIterator::into_iter(<range literal>). If there was a step_by, this would be ExprMethodCall(<range_literal>, step_by), which is different :)

[llogiq]

Fair enough – although that wouldn't catch let r = (1..-1); for x in r { .. }. 😜

The visitSub...-based abstraction would enable us to catch those, too, and be reusable in other places – if it's possible to write such a thing in Rust at all.

[Manishearth]

Getting the parent node is a fast operation (hashmap lookup+field access), btw, so I don't see why we need to cache it ourselves.

[llogiq]

Then we should provide a utils method for that.

[Manishearth]

No need, http://manishearth.github.io/rust-internals-docs/rustc/ast_map/struct.Map.html#method.get_parent_node

[Manishearth]

Btw, https://github.com/Manishearth/rust-clippy/blob/master/CONTRIBUTING.md. Let me know if you don't want to explicitly be listed as a mentor, I just added you since you have helped folks out a couple of times before 😄

[llogiq]

I'm always glad to help.