2024 edition migration of ref in patterns suboptimal: fixed up by clippy --fix

[VorpalBlade]

The 2024 edition migration changed this:

impl TryFrom<&Selector> for konfigkoll_utils::line_edit::Selector {
    type Error = eyre::Error;

    fn try_from(value: &Selector) -> Result<Self, Self::Error> {
        match value {
            Selector::All => Ok(Self::All),
            Selector::Eof => Ok(Self::Eof),
            Selector::Line(n) => Ok(Self::Line(*n)),
            Selector::Range(a, b) => Ok(Self::Range(*a, *b)),
            Selector::Regex(r) => Ok(Self::Regex(Regex::new(r).wrap_err("invalid regex")?)),
            Selector::Function(ref f) => {
                let f = f.clone();
                Ok(Self::Function(Rc::new(move |lineno, s| {
                    let guard = f.borrow_mut().expect("Failed to borrow function object");
                    match guard.call::<_, bool>((lineno, s)) {
                        VmResult::Ok(v) => v,
                        VmResult::Err(e) => {
                            tracing::error!(
                                "Error in custom selector function {:?}: {:?}",
                                *guard,
                                e
                            );
                            false
                        }
                    }
                })))
            }
        }
    }
}

into this:

impl TryFrom<&Selector> for konfigkoll_utils::line_edit::Selector {
    type Error = eyre::Error;

    fn try_from(value: &Selector) -> Result<Self, Self::Error> {
        match value {
            Selector::All => Ok(Self::All),
            Selector::Eof => Ok(Self::Eof),
            Selector::Line(n) => Ok(Self::Line(*n)),
            Selector::Range(a, b) => Ok(Self::Range(*a, *b)),
            Selector::Regex(r) => Ok(Self::Regex(Regex::new(r).wrap_err("invalid regex")?)),
            &Selector::Function(ref f) => {
                let f = f.clone();
                Ok(Self::Function(Rc::new(move |lineno, s| {
                    let guard = f.borrow_mut().expect("Failed to borrow function object");
                    match guard.call::<_, bool>((lineno, s)) {
                        VmResult::Ok(v) => v,
                        VmResult::Err(e) => {
                            tracing::error!(
                                "Error in custom selector function {:?}: {:?}",
                                *guard,
                                e
                            );
                            false
                        }
                    }
                })))
            }
        }
    }
}

Then running cargo +beta clippy --fix changed it right back to Selector::Function(f).

When runing without --fix this is the clippy message:

warning: dereferencing a tuple pattern where every element takes a reference
   --> crates/konfigkoll_script/src/plugins/patch.rs:105:13
    |
105 |             &Selector::Function(ref f) => {
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = 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
    |
105 -             &Selector::Function(ref f) => {
105 +             Selector::Function(f) => {
    |

warning: dereferencing a tuple pattern where every element takes a reference
   --> crates/konfigkoll_script/src/plugins/patch.rs:208:13
    |
208 |             &Action::Function(ref f) => {
    |             ^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrowed_reference
help: try removing the `&` and `ref` parts
    |
208 -             &Action::Function(ref f) => {
208 +             Action::Function(f) => {
    |

I expected to see this happen: A coherrent decision on what way is the proper way to write this. Rustc and clippy shouldn't fight each other. The edition migration shouldn't be sloppy and need to be fixed up by clippy.

Instead, this happened: Clippy has to clean up after rustc.

I have also pushed this to a branch in my repo, in case you need more context for investigating this: https://github.com/VorpalBlade/paketkoll/tree/feature/edition-2024 (the migration and the reversion are each in a separate commit). There seemed to be a few more instances of the same needless_borrowed_reference in other files too, but I think they are the same underlying cause.

Meta

rustc --version --verbose:

$ rustc +beta --version --verbose                   
rustc 1.85.0-beta.5 (441650704 2025-01-20)
binary: rustc
commit-hash: 44165070434aab38db1115448621880a35da00ff
commit-date: 2025-01-20
host: x86_64-unknown-linux-gnu
release: 1.85.0-beta.5
LLVM version: 19.1.7

Backtrace

Not relevant? Didn't crash after all...

[VorpalBlade]

@rustbot modify labels: A-edition-2024

[Nadrieril]

That's surprising indeed, the lint shouldn't trigger on that. What the type of the payload of the Function variant?

[VorpalBlade]

The type here is:

#[derive(Clone)]
#[non_exhaustive]
pub enum Selector {
    /// Match all lines
    All,
    /// End of file (useful to insert lines at the very end)
    Eof,
    /// Match a specific line number (1-indexed)
    Line(usize),
    /// A range of line numbers (1-indexed, inclusive)
    Range(usize, usize),
    /// A regex to match the line
    Regex(Regex),
    /// A custom function, passed the line number and current line
    #[allow(clippy::type_complexity)]
    Function(Rc<dyn Fn(usize, &str) -> bool>),
}

However, this also happens separately with the function variant of this other enum as well:

#[derive(Debug, Any)]
#[rune(item = ::patch)]
enum Action {
    /// Copy the current line to the output. Only needed when auto-print is
    /// disabled.
    #[rune(constructor)]
    Print,
    /// Delete the current line and short circuit the rest of the program
    /// (immediately go to the next line)
    #[rune(constructor)]
    Delete,

    // ... [cut out for brevity, there are many more variants] ...

    #[rune(constructor)]
    RegexReplace(#[rune(get)] String, #[rune(get)] String),
    /// Like `RegexReplace` but replaces all matches on the line.
    #[rune(constructor)]
    RegexReplaceAll(#[rune(get)] String, #[rune(get)] String),
    /// A sub-program that is executed. Will share pattern space with parent
    /// program
    Subprogram(LineEditor),
    /// A custom function passed the current pattern buffer, returning a new
    /// pattern buffer
    #[rune(constructor)]
    Function(#[rune(get)] Shared<rune::runtime::Function>),
}

As well as this change in save.rs (that clippy also undoes):

--- a/crates/konfigkoll_core/src/save.rs
+++ b/crates/konfigkoll_core/src/save.rs
@@ -28,13 +28,13 @@ pub fn save_fs_changes<'instruction>(
     for instruction in instructions {
         let comment = match (instruction.pkg, &instruction.comment) {
             (None, None) => CompactString::default(),
-            (None, Some(ref comment)) => {
+            (None, &Some(ref comment)) => {
                 format_compact!(" // {comment}")
             }
             (Some(pkg), None) => {
                 format_compact!(" // [{}]", pkg.as_str(interner))
             }
-            (Some(pkg), Some(ref comment)) => {
+            (Some(pkg), &Some(ref comment)) => {
                 format_compact!(" // [{}] {comment}", pkg.as_str(interner))
             }
         };

Where comment is a &CompactString according to the Rust Analyzer inlay hint. So the matched type is (Option<PackageRef>, &Option<CompactString>). Where PackageRef is a newtype around lasso::Spur and CompactString is from the compact_str crate.

And in the third case (state.rs) I have this type, and the second member of the Apply variant is affected (the Arc is not).

pub enum DiffGoal<'map, 'files> {
    Apply(Arc<dyn Files>, &'map PathMap<'files>),
    Save,
}

I would suggest you take a look at the whole diff of VorpalBlade/paketkoll@7ec3d44 to see what clippy changed back as there are several instances of this, with different types. I don't know which ones should be considered separate bugs and which ones should be the same. They all look related to me.

[Nadrieril]

I see two issues at play here:

1. The migration lint is a bit stupid and doesn't suggest the optimal pattern. It should really be suggesting Some(ref comment) => Some(comment) instead of => &Some(ref comment).
2. It seems you've found a case where the migration lint triggers when it really shouldn't. Are you quite sure the initial code wasn't Selector::Function(ref f)? If it was ref f then the lint is working as intended (if stupid).

[VorpalBlade]

Ah, yes it seems you are right looking at VorpalBlade/paketkoll@a715c59. There was an extra ref there. So your point 2 is indeed not an issue.

Point 1 remains. Clippy shouldn't have to clean up from the migration being sloppy.

When I'm not on mobile I will edit the title and the original post to clarify that clippy didn't actually revert, just clean up.

[traviscross]

See also:

• Rust 2024 match ergonomics and needless_borrowed_reference rust-clippy#13616

cc @dianne @Manishearth @Centri3 @flip1995

[ehuss]

Does anyone know how difficult it would be to make rust_2024_incompatible_pat smarter so that it recommends the more concise syntax?

[Manishearth]

Clippy shouldn't have to clean up from the migration being sloppy.

FWIW, the original design for editions had two-step "edition lint" + "idiom lint" phases. The first phase took code that compiled on the old edition and turned it into code that compiled on both editions, but was potentially not fully idiomatic. The second phase took code that compiled on both editions and cleaned it up into code that may only compile in the newest edition.

This was quite necessary since edition migrations aren't always perfect, and it's not great for the codebase to be in a situation where it's partially migrated but no longer compiles on either edition so you can't easily migrate further

So while it's possible that in this specific case the migration could have been smarter, in general Clippy "fixing up after the edition" is fine. Ideally these fixups would be idiom lints, but it's not too bad for Clippy to do it.

[dianne]

Does anyone know how difficult it would be to make rust_2024_incompatible_pat smarter so that it recommends the more concise syntax?

I don't think it'd be too difficult. I've written something very similar for another diagnostic. I'll see what I can do.

@rustbot claim