Skip to content

Conversation

@samueltardieu
Copy link
Member

When lowering, a..=b is transformed into RangeInclusive::new(a, b) without any desugaring information on the spans. It looks like a..=b requires Rust 1.27 (stabilization of RangeInclusive::new()), while a..=b was introduced in Rust 1.26.

Marking the desugaring as such in the HIR allows Clippy to filter it out and realize that this is not the original code coming from the user.

@rustbot
Copy link
Collaborator

rustbot commented Feb 13, 2025

r? @davidtwco

rustbot has assigned @davidtwco.
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 rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 13, 2025
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this special-cased for RangeClosed? Shouldn't this also be applied to the open ranges?

@samueltardieu
Copy link
Member Author

samueltardieu commented Feb 14, 2025

Why is this special-cased for RangeClosed? Shouldn't this also be applied to the open ranges?

Other kind of ranges build a struct instead of calling an initializer function, but this applies as well. I have updated the patch to do so.

I also made expr_field() private as it is not used anywhere, which let me change the span parameter into lowered_span as I want to use a span marked for desugaring. Would it be better to inline this function in lower_expr_range() instead, or is it ok like that?

@rust-log-analyzer

This comment has been minimized.

@matthewjasper
Copy link
Contributor

The compiler generally checks for hir::QPath::LangItem(...) to find range patterns (e.g. https://github.com/rust-lang/rust/blob/54cdc751df770517e70db0588573e32e6a7b9821/compiler/rustc_hir_typeck/src/method/suggest.rs#L2395-2407)

@samueltardieu
Copy link
Member Author

The compiler generally checks for hir::QPath::LangItem(...) to find range patterns (e.g. https://github.com/rust-lang/rust/blob/54cdc751df770517e70db0588573e32e6a7b9821/compiler/rustc_hir_typeck/src/method/suggest.rs#L2395-2407)

We could specialize the MSRV check in Clippy to try and detect this desugaring, but we would have to also compare the source code strings to distinguish between a desugaring and a genuine user call to RangeInclusive::new(). It seemed simpler to use the dedicated mechanism present in the compiler for marking desugaring happening at lowering time.

@matthewjasper
Copy link
Contributor

Writing RangeInclusive::new() wouldn't produce a QPath::LangItem

@samueltardieu
Copy link
Member Author

samueltardieu commented Feb 14, 2025

Writing RangeInclusive::new() wouldn't produce a QPath::LangItem

Good point. While you're right on this, I can't understand if you're objecting to marking the desugaring as a desugaring and you suggest Clippy special-cases this particular desugaring MSRV (none other is), or if this was just a side remark.

@samueltardieu
Copy link
Member Author

@matthewjasper While I would prefer to mark desugared hir as such, your comments made me realize that this is not necessary for what I want to achieve, as excluding all the QPath::LangItem from MSRV checking will give me the extra bit of information I need. Thanks!

I'll close this PR.

@samueltardieu samueltardieu deleted the range-desugaring branch February 15, 2025 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants