`match_as_ref` might suggest invalid code

[samueltardieu]

Summary

Copied from #15928 (review)

    let _: Option<&u32> = match Some(0) {
        //~^ match_as_ref
        None => None,
        Some(ref mut v) => Some(v),
    };

will suggest replacing by

    let _: Option<&u32> = Some(0).as_mut();

which doesn't have the right type (Option<&mut u32> instead of Option<&u32>).

Version

Additional Labels

@rustbot label +I-suggestion-causes-error

[ada4a]

There's already logic to add .map(|x| x as _) when a cast between types is needed, e.g. in a case like this:

let _: Option<&dyn std::fmt::Debug> = match Some(5u32) {
    Some(ref n) => Some(n),
    None => n,
};

the lint suggests this:

let _: Option<&dyn std::fmt::Debug> = Some(5u32).as_ref().map(|x| x as _);

And theoretically, that same cast could take care of the &mut-to-& downcasting as well.

With that, on an expression like this:

let _: Option<&dyn std::fmt::Debug> = match Some(5u32) {
    Some(ref mut n) => Some(n),
    None => n,
};

would you prefer a fix like this:

let _: Option<&dyn std::fmt::Debug> = Some(5u32).as_ref().map(|x| x as _);

or like this?

let _: Option<&dyn std::fmt::Debug> = Some(5u32).as_mut().map(|x| x as _);

I think I like the former more, because it puts a bit less magic into the casting part.

[samueltardieu]

I prefer the former as well.

For &mut to &, we could just suggest using .as_ref(), couldn't we?

[ada4a]

For &mut to &, we could just suggest using .as_ref(), couldn't we?

Exactly, and then only add the .map() if the type needs casting as well.

Should I claim this, or maybe it could be marked as a good first issue?

[samueltardieu]

Should I claim this, or maybe it could be marked as a good first issue?

Since the bug gives wrong suggestions, feel free to claim it.

[samueltardieu]

@rustbot label +Isuggestion-causes-error

Is rustbot on strike? Did I forget how to use it?

[ada4a]

Maybe it's because of the missing dash?

@rustbot label +I-suggestion-causes-error

(EDIT: hm, nope)

[samueltardieu]

Yes, I forgot a dash but I didn't forget it in the summary and it didn't act.

[ada4a]

Weird.. will this work at least?

@rustbot claim

(EDIT: huh)

[samueltardieu]

Just to test since it reacted to claim

@rustbot label S-blocked

[samueltardieu]

I'll talk with the infra team

(edit: thread on Zulip)

[ada4a]

Fixed in #15934