range_plus_one help suggestion should not remove braces

[alexheretic]

The following code triggers the range_plus_one lint correctly. However the suggestion, if applied explicitly ie by an IDE, removes the braces which causes a compile error.

fn main() {
    let from = 5;
    let to = 16;
    for n in (from..to + 1).rev() {
        eprintln!("{}", n);
    }
}

  --> src/main.rs:17:14
   |
17 |     for n in (from..to + 1).rev() {
   |              ^^^^^^^^^^^^^^ help: use: `from..=to`
   |
   = note: #[warn(range_plus_one)] on by default
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#range_plus_one

Incorrect suggested code:

fn main() {
    let from = 5;
    let to = 16;
    for n in from..=to.rev() {
        eprintln!("{}", n);
    }
}

[daubaris]

I would love to take this one! :) And also a question - the correct version should be for n in (from..=to).rev()?

[flip1995]

Great! It's all yours. Yes exactly. If you have more question, we're happy to answer!

[daubaris]

One issue that I'm facing is while running cargo test - it fails to resolve rustc::hir::Node in various clippy_lints files. The first error gives no Node in hir and others just repeat Use of undeclared type or module Node.

I'm running latest:
stable-x86_64-pc-windows-msvc unchanged - rustc 1.28.0 (9634041f0 2018-07-30)
nightly-x86_64-pc-windows-msvc unchanged - rustc 1.30.0-nightly (f7202e40f 2018-08-27)

I'm wondering, what I forgot to do?

[flip1995]

TL;DR: rustc nightly is not bleeding edge enough

Yeah this is because the Clippy master branch should always compile with the current rustc master and not the nightly, because new nightlies are blocked if Clippy fails to compile. We're currently trying to get Travis and Appveyor to use the rustc master: #2941

Enough with the background info, what do you have to do:
You can install the rustc master toolchain with https://github.com/kennytm/rustup-toolchain-install-master

rustup-toolchain-install-master -f -n master

and compile Clippy with cargo +master test

[daubaris]

Sorry for disturbing again but I dug deeper and got confused.
Should we ignore parentheses all in general and look into the content inside them or am I going the wrong path here.. Also it would be great if I could be navigated to the starting point :/. I've found the range_plus_minus_one.rs file and while changing the tests and adding parantheses around the expressions it seems it deletes the parentheses by default.

[flip1995]

Sure thing! If you want to find what is wrong with a lint, the first step is usually to look at the code, which generates the lint. (In this case search for RANGE_PLUS_ONE in clippy_lints) Second step would be to understand what is going on in the code where you found the lint name. In this case it would be here:
https://github.com/rust-lang-nursery/rust-clippy/blob/0f2eab6337350851d8eb06e2ef439a3540b85a9e/clippy_lints/src/ranges.rs#L140-L152

Look especially at format!("{}..={}", start, end). This generates the string of the suggestion. You see that it will never add parentheses to the suggestion.

Maybe you could also use this function:
https://github.com/rust-lang-nursery/rust-clippy/blob/0f2eab6337350851d8eb06e2ef439a3540b85a9e/clippy_lints/src/utils/sugg.rs#L180-L185

[daubaris]

Yes, i saw that format doesn't add the parentheses, but if lets say they do and the expression doesn't use a method after, it will invoke a warning of unnecessary parentheses.

[flip1995]

Sorry I misunderstood you then. You're right, the parentheses should only be added if the expression span has parentheses. You can use the utils::snippet_opt to get a String from the span to operate on.

[daubaris]

Alright, so I managed fix it for the current issue using snippet_opt and operating on given string with the methods starts_with and ends_with. Although while testing it with current tests there's a problem with the last case which is:
https://github.com/rust-lang-nursery/rust-clippy/blob/b87ab5ccf2d30442cba8d0de08edb0f86a33ca58/tests/ui/range_plus_minus_one.rs#L28
It adds another pair of parentheses. I'm wondering if there is some kind of a method to check this kind of case - checking the closing brackets or something similar? Open to suggestions.

[flip1995]

I didn't thought of this case. You can't differ between (x..y) and x..y on the hir-level (LateLintPass) nicely. Though on ast-level (EarlyLintPass) you can differentiate these cases:

• (x..y) has ast::ExprKind::Paren(expr), with expr.node = ast::ExprKind::Range(..)
• x..y has ast::ExprKind::Range(..)

You have two options:

• Easy: just ignore this case and add the parentheses. Document this in the Known Problems section of the lint
• a little bit more work, but correct: Rewrite it as an EarlyLintPass. You don't really need hir-level type information.

If you want you can also implement the first option for now and leave the second option open.