Add note when matching on tuples/ADTs containing non-exhaustive types

[sebastiantoh]

Fixes #85222

r? @Nadrieril

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Nadrieril (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[rustbot]

Some changes might have occurred in exhaustiveness checking

cc @Nadrieril

[Nadrieril]

Hi, thanks for tackling this!

This is a good attempt! However I think looking at the type is not the right approach. For example, in

match Some(1usize) {
	None => {}
}

we want to report that Some(_) is missing but we don't want to show the note about non-exhaustiveness. In

match Some(1usize) {
	None => {}
	Some(0) => {}
}

we do want the note. We can't tell the difference from the types alone.

The information we want is however present in the witnesses. The way it works is that a _ in the error message means a DeconstructedPat with either a Constructor::Wildcard or a Constructor::NonExhaustive. If it's Wildcard it's the common case where we don't need the note. If it's NonExhaustive it means some unwritable patterns are missing like in the second case, and we want the note.

The NonExhaustive check you removed was doing what I just describe, however it was doing it only at the top of the pattern. If we want it to work for Some(_) etc then we'll need to recursively look inside witnesses[0] for NonExhaustive constructors of the right type. Does that all makes sense?

[sebastiantoh]

thanks for the feedback!

The NonExhaustive check you removed was doing what I just describe, however it was doing it only at the top of the pattern. If we want it to work for Some(_) etc then we'll need to recursively look inside witnesses[0] for NonExhaustive constructors of the right type.

I'm not sure how can I "recursively look inside witnesses[0]`.

+    debug!("witnesses={:?}", witnesses);
+    debug!("witnesses[0].ctor()={:?}", witnesses[0].ctor());
+    debug!("witnesses[0].fields()={:?}", witnesses[0].iter_fields().collect::<Vec<_>>());
+    debug!("witnesses[0].ty()={:?}", witnesses[0].ty());
    if !is_empty_match && witnesses.len() == 1 {
        report_non_exhaustive_tys(cx, scrut_ty, &mut err, &mut FxHashSet::default());
    }

Adding the above debugging code, I get the same outputs for both of the examples you listed. Specifically, it doesn't seem possible to recurse into witnesses[0].ctor() to check for Constructor::NonExhaustive

│ │ │ │ │ ├─1ms DEBUG rustc_mir_build::thir::pattern::check_match witnesses=[Some(_ : usize)]
│ │ │ │ │ ├─1ms DEBUG rustc_mir_build::thir::pattern::check_match witnesses[0].ctor()=Variant(1)
│ │ │ │ │ ├─1ms DEBUG rustc_mir_build::thir::pattern::check_match witnesses[0].fields()=[_ : usize]
│ │ │ │ │ ├─1ms DEBUG rustc_mir_build::thir::pattern::check_match witnesses[0].ty()=std::option::Option<usize>

Is it correct to say that I need to do the following:

1. Fix witnesses[0].ctor() such that it returns Constructor::NonExhaustive instead of Constructor::Variant(1) for the second example you highlighted
2. Update the condition for emitting the new note to be roughly as follows

if (scrut_ty.contains(cx.tcx.types.usize) || scrut_ty.contains(cx.tcx.types.isize))
        && !is_empty_match
        && witnesses.len() == 1
        && matches!(witnesses[0].ctor(), Constructor::NonExhaustive) { ... }

[Nadrieril]

What you need to look into recursively is witness.fields(). Here you got witnesses[0].fields()=[_ : usize]; this _ : usize is actually a DeconstructedPat whose ctor() could be Wildcard or NonExhaustive.

To explain quickly the constructor/fields thing, the idea is that all patterns/values either look like X(A, B, C)/X { a: A, b: B } i.e. something with "inner values", or they don't. When they exist, these inner values are stored in fields(). Whatever is left is stored in ctor(). So if the pattern is Some(Ok(_)), we might get more or less:

DeconstructedPat {
	ctor: "Some",
	fields: [
		DeconstructedPat {
			ctor: "Ok",
			fields: [
				DeconstructedPat {
					ctor: "Wildcard",
					fields: [],
				}
			],
		}
	],
}

[sebastiantoh]

I see, thanks for the guidance. I've updated my PR. Hopefully it's correct now!

[sebastiantoh]

For this case, should the ... does not have a fixed maximum value note be reported for both isize and usize?

I would think it should, but my PR currently outputs the note for isize only. When I recurse into the witness of (0isize, 0usize), I get NonExhaustive for the isize, but Wildcard for the usize. Is this intentional?

Likewise for the case at https://github.com/rust-lang/rust/pull/114397/files#diff-079dd1fff457e8e3896d1365b6d8780efeadcf0b79e01928f36f4f81874213a9R54-R58

[Nadrieril]

Ah yeah, this is a limitation/feature of the exhaustiveness algorithm: it reports the "first pattern it finds", so in this case it didn't bother looking into the second part of the tuple before returning the error. It seems fine to report only the first one.

[Nadrieril]

That looks great! Love the thoroughness of the tests. Let's merge this

@bors r+

[bors]

📌 Commit 82ce7b1 has been approved by Nadrieril

It is now in the queue for this repository.

[bors]

⌛ Testing commit 82ce7b1 with merge c75b6bd...

[bors]

☀️ Test successful - checks-actions
Approved by: Nadrieril
Pushing c75b6bd to master...

[rust-timer]

Finished benchmarking commit (c75b6bd): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.2%	[-2.2%, -2.2%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.6%	[1.0%, 2.9%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.0%	[-2.0%, -2.0%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 631.442s -> 629.758s (-0.27%)
Artifact size: 315.61 MiB -> 315.65 MiB (0.01%)