Fix replace-if-let-with-match generates non-exhausive match

[profetia]

Why

Closes: #8690 and #10583

Continuation of #10491. r? @Veykril if you don't mind?

The reason why the original author were unable to reproduce the first case in #8690 might be because of misuses of minicore, which creates an error in the types and make it fallback to the wildcard. Maybe we should give more information about minicore in the docs?

I extended the patterns marked as FIXME in #10491, while also covering the cases involving literals.

What

• Add test cases for replace_if_let_with_match
• Cover more cases for using wildcards on make_else_arm

[ChayimFriedman2]

I'd be more happy with some approach based on the analysis on the usefulness of the pattern (didn't check if something like that is possible), but this is good enough for now.

[ChayimFriedman2]

Suggested change

	pat.fields().any(|pat| check_pat_variant_nested_or_literal_with_depth(&pat, depth))
	pat.fields().any(|pat| check_pat_variant_nested_or_literal_with_depth(&pat, depth + 1))

Should be depth + 1.

[profetia]

I think nesting tuple pattern is fine if it does not contain literal patterns or tuple struct patterns or else.

let bar: Result<(i32, i32, Option<i32>), ()> = Ok((1, 2, Some(3)));
if let Ok((first, second, third)) = bar {} // fine
if let Ok((1, second, third)) = bar {} // bad
if let Ok((first, second, Some(third))) = bar {} // bad

[ChayimFriedman2]

Then why the others (e.g. OrPat) have a depth limit?

[profetia]

Fair point, I must have missed it. Just fixed it.

[ChayimFriedman2]

It's also in TupleStructPat, and if you're going to not use depth, why not just remove it?

[profetia]

I doubt about TupleStructPat, it should not be nested.

let bar: Result<Option<i32>, ()> = Ok(Some(1));
if let Ok(Some(a)) = bar {} // bad

[ChayimFriedman2]

Ah so this is the reason - then RecordPat should also be depth + 1 (Enum::Variant {}) - and perhaps also rename depth to something more informative, e.g. depth_after_refutable.

[profetia]

Fair. You remind me that I forgot to consider record in enum Enum::Variant {} at all. My current test cases only covered structs MyStruct { a, b }.

[profetia]

Fixed. I also covered cases like MyStruct(i32, i32)

[ChayimFriedman2]

Here too depth + 1

[profetia]

Similar reason to TuplePat

[ChayimFriedman2]

I think this test can benefit from a split to several tests, but I'll leave that to you.

[profetia]

Updated. Thanks for your time reviewing this and happy new year!

[profetia]

@rustbot review

[Veykril]

Maybe we should give more information about minicore in the docs?

Do we talk about minicore in our dev guide? Probably not in which case we absolutely should

[Veykril]

I'd be more happy with some approach based on the analysis on the usefulness of the pattern (didn't check if something like that is possible), but this is good enough for now.

I think thats something @Nadrieril specifically tried to enable with the librarification of the exhaustiveness checking code in rustc (that we now use). Please correct me if I am wrong :)

[Nadrieril]

Indeed using the exhaustiveness checker should be possible here. For this case, I'd expect we do something like: let pat be the pattern in the if let. Pass vec![pat] to MatchCheckCtx.compute_match_usefulness. non_exhaustiveness_witnesses in the output contains a list of patterns that must be added to pat to make an exhaustive match. Simplest behavior is: if this list has a single element, use that for the other match arm; otherwise use _.

[profetia]

Seems MatchCheckCtx is already in the library. Let me see if I can use that instead.

[profetia]

Seems MatchCheckCtx is already in the library. Let me see if I can use that instead.

I'm not an expert in rust analyzer but after some attempts I feel there might be a lot of work to be done before the MatchCheckCtx can be used as a tool outside hir-ty. Indeed this is a good place for the exhaustiveness checker.

[ChayimFriedman2]

Thanks! I think this is enough for now.