Mark desugared range expression spans with DesugaringKind::RangeExpr

[camsteffen]

This is a prerequisite to removing QPath::LangItem (#115178) because otherwise there would be no way to detect a range expression in the HIR.

There are some non-obvious Clippy changes so a Clippy team review would be good.

[rustbot]

r? @SparrowLii

rustbot has assigned @SparrowLii.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[flip1995]

That this check is required in some many places is a bit of a code smell. A lot of places where from_expansion() was checked, switched to this check instead. I could imagine that there are more lints that would like to continue linting on ranges, but don't have specific tests for ranges. 🤔

[camsteffen]

Yeah it does seem like a code smell. But also I think the explicitness, special casing seems warranted at least in some cases. Like in this case, we are choosing to lint range expressions in addition to struct expressions, and it's not obvious that that is right. In other cases, I think the from_expansion check is over-defensive to begin with, and I was forced to poke a hole in that defense to preserve behavior.

[flip1995]

forced to poke a hole in that defense to preserve behavior.

This is my main concern. There might be other lints that now silently change behavior, because writing explicit tests for ranges was a bit pointless or not on the radar back then.

[camsteffen]

Yeah there is a valid concern here. Let me try to elaborate on it...

Suppose clippy has a lint that triggers the following code:

some_expr.silly_method() // triggers lint

...and suppose there is a from_expansion check that causes a false negative:

some_macro!().silly_method(); // should lint but it doesn't

After this PR, the false negative will be expanded to range expressions:

(1..3).silly_method(); // should lint but (now) it doesn't

And this problem might exist for N lints in Clippy.

On the other hand, there could be false positives in Clippy where it triggers on desugared range expressions because Clippy thinks the user wrote a struct literal. And this PR fixes those. (Though to be fair this seems less likely.)

So, to mitigate risk, I guess someone would have to audit Clippy for extraneous from_expansion checks? I'm just a little unsure if the juice is worth the squeeze.

[flip1995]

Yeah, I also don't have a better idea to deal with it. The motivation of this PR seems more important than the unknown amount of Clippy lints that might stop linting.

Once this is merged and synced to Clippy, I can look in CI if lintcheck now triggers less on real code for this lint. And then potentially fix those in Clippy.

So this is not supposed to block this PR.

However, it would be nice, if you could add a helper function to clippy_utils that does this:

    fn from_expansion_except_range(span: Span) -> bool {
        let span = if span.desugaring_kind() == Some(DesugaringKind::RangeExpr) {
            span.parent_callsite().unwrap()
        } else {
            span
        };
        span.from_expansion()
    }

This is the majority of the changes in Clippy. Or maybe even to Span itself? Not sure if Clippy would stay the only user of that.

[camsteffen]

I'm not sure such a util would be advisable to reuse though. I think the pattern arises because of unnecessary from_expansion checks, with one or two exceptions.

[camsteffen]

I introduced Expr::range_span and it makes the a lot of the Clippy changes cleaner.