Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rust 2024 match ergonomics and needless_borrowed_reference #13616

Open
traviscross opened this issue Oct 28, 2024 · 6 comments
Open

Rust 2024 match ergonomics and needless_borrowed_reference #13616

traviscross opened this issue Oct 28, 2024 · 6 comments
Labels
A-category Area: Categorization of lints C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages D-confusing I-nominated Issue: Nominated to be discussed at the next Clippy meeting S-needs-discussion Status: Needs further discussion before merging or work can be started

Comments

@traviscross
Copy link

traviscross commented Oct 28, 2024

In Rust 2024, we're making some changes to match ergonomics. Those changes are:

  • Rule 1C: When the DBM (default binding mode) is not move (whether or not behind a reference), writing mut, ref, or ref mut on a binding is an error.
  • Rule 2C: Reference patterns can only match against references in the scrutinee when the DBM is move.

The net result of this is that we're pushing some code to use a fully-explicit match rather than using match ergonomics, and this is what the edition migration lints do.

Here's where clippy comes in. Consider:

let [ref _x] = &[0u8]; // _x: &u8

This is accepted in Rust 2021 and clippy does not warn about it even though that ref in the pattern is redundant. We'll be rejecting this code in Rust 2024 under Rule 1C, and the migration lints will move it to the fully-explicit form:

fn main() {
    let &[ref _x] = &[0u8]; // _x: &u8
    //[clippy]~^ WARN dereferencing a slice pattern where every element takes a reference
    //[clippy]~| NOTE `#[warn(clippy::needless_borrowed_reference)]` on by default
}

However, as noted above, clippy lints on that.

warning: dereferencing a slice pattern where every element takes a reference
 --> src/main.rs:2:9
  |
2 |     let &[ref _x] = &[0u8]; // _x: &u8
  |         ^^^^^^^^^
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrowed_reference
  = note: `#[warn(clippy::needless_borrowed_reference)]` on by default
help: try removing the `&` and `ref` parts
  |
2 -     let &[ref _x] = &[0u8]; // _x: &u8
2 +     let [_x] = &[0u8]; // _x: &u8
  |

The suggestion given in this case is accurate, but it may be a bit unfortunate for clippy to lint against how rustc suggested rewriting the code. It's not practical for rustc to be more clever here; we really just want to migrate these cases to the corresponding fully-explicit form.

With my edition hat on, in the interest of making the adoption of Rust 2024 the most seamless for the most people, I'd probably prefer that this lint be made allow-by-default by clippy before the release of Rust 2024, but I'm interested to hear what the @rust-lang/clippy team thinks about this and what options there may be here.

Tracking:

Other open issues about this lint:

cc @Nadrieril @joshtriplett

@flip1995 flip1995 added C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages S-needs-discussion Status: Needs further discussion before merging or work can be started A-category Area: Categorization of lints D-confusing labels Oct 28, 2024
@xFrednet xFrednet added the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Oct 28, 2024
@Centri3
Copy link
Member

Centri3 commented Oct 28, 2024

If I understand, the main concern is that this contradicts the change made by rustc before, and not the suggestion itself? Is there a reason to use the code rustc suggests over clippy's?

@Manishearth
Copy link
Member

Yeah, I don't actually consider this as a case of the lints contradicting, the lint is just refining.

Previous editions had this as a first class concept: the 2018 edition design had "idiom lints", where the edition lints would transform 2015 code to code that compiles on both editions (this was really useful during migration if the lints weren't perfect, you don't want your codebase in a situation where you can't run some compiler to get lint output. You also want your migration lints to not be too complicated). A subsequent set of "edition idiom" lints would "clean it up" to be more idiomatic (but potentially no longer working on 2015).

I don't think such a multi step refinement process causes any problems. It is already the case that a compiler upgrade will often cause new code to be linted with Clippy, this is just a different kind of compiler upgrade.

This particular lint is not one I feel strongly about, but I don't think the justification provided here rises to the level of downgrading this lint.

@flip1995
Copy link
Member

flip1995 commented Oct 28, 2024

Oh, I missed that the Clippy suggestion is correct, when skimming the issue earlier:

2 -     let &[ref _x] = &[0u8]; // _x: &u8
2 +     let [_x] = &[0u8]; // _x: &u8

In that case, I would say this is already the behavior we want. Clippy lints also sometimes suggest code, that is then linted by another Clippy lint, so it is the same multi step refinement process.

I agree with Manish, that this is not a reason for downgrading the lint, but rather a lucky coincidence, that Clippy can help improving the code that the rust edition lints suggest.

@traviscross
Copy link
Author

traviscross commented Oct 28, 2024

Another option to consider would be to have clippy start to lint against this:

let [ref _x] = &[0u8]; // _x: &u8

The ref there is redundant in all editions and can be removed.

That way, with respect to the edition migration, clippy would be linting on the "before" code as well as on the "after" code.

@Manishearth
Copy link
Member

Clippy lints also sometimes suggest code, that is then linted by another Clippy lint, so it is the same multi step refinement process.

Yeah, that's a good point: This sort of multi step refinement is standard in the Clippy world, we try to produce lint free suggestions but it's not a strong attempt. This isn't considered a bug in the first lint.

That way, with respect to the edition migration, clippy would be linting on the "before" code as well as on the "after" code.

That would be acceptable I think.

@Centri3
Copy link
Member

Centri3 commented Oct 29, 2024

Another option to consider would be to have clippy start to lint against this:

Agree with this solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-category Area: Categorization of lints C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages D-confusing I-nominated Issue: Nominated to be discussed at the next Clippy meeting S-needs-discussion Status: Needs further discussion before merging or work can be started
Projects
None yet
Development

No branches or pull requests

5 participants