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

Add note on non-exhaustiveness when matching on str and nested non-exhaustive enums #115270

Merged
merged 3 commits into from Sep 3, 2023
Merged

Add note on non-exhaustiveness when matching on str and nested non-exhaustive enums #115270

merged 3 commits into from Sep 3, 2023

Conversation

sebastiantoh
Copy link
Contributor

Fixes #105479

r? @Nadrieril

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 27, 2023
@rustbot
Copy link
Collaborator

rustbot commented Aug 27, 2023

Some changes might have occurred in exhaustiveness checking

cc @Nadrieril

@@ -18,6 +19,7 @@ LL | match "world" {
| ^^^^^^^ pattern `&_` not covered
|
= note: the matched value is of type `&str`
= note: `str` cannot be matched exhaustively, so a wildcard `_` is necessary
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding an additional note, I wonder if it's easier to just specialize the help message "ensure that all possible cases are being handled by adding a match arm with a wildcard pattern or an explicit pattern as shown" below to be a bit more context specific, explaining something a bit more directly about how strings aren't exhaustive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean something like the following?

help: as `str` cannot be matched exhaustively, ensure that all possible cases are being handled by adding a match arm with a wildcard pattern or an explicit pattern as shown 

I think it would make the help message too lengthy. My understanding is that notes can be used to provide further explanation or context on what caused the error instead of cluttering the help message with too much context.

But I don't have a strong opinion on this, so I'll be happy to make the change if needed. If we do make the change, do you think it would be necessary to make the change for pointer-sized ints as well for consistency??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also prefer we didn't add a note, but I don't know how we'd phrase it. The tricky thing is that the non exhaustive type may be deep in a pattern somewhere (e.g. Result<Option<&str>, ...>), which prevents us from saying e.g. "ensure all possible strings are being handled".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh oops forgot about this one before merging. I think it's ok like this, if one of you has a better idea we can make another PR. My secret plan is to suggest actual missing strings, something like patterns Some(""), Some("a"), Some("b") and many others not covered but I expect that will take some work.

@sebastiantoh sebastiantoh changed the title Add note that str cannot be matched exhaustively Add note that on non-exhaustiveness when matching on str and nested non-exhaustive enums Aug 28, 2023
@sebastiantoh sebastiantoh changed the title Add note that on non-exhaustiveness when matching on str and nested non-exhaustive enums Add note on non-exhaustiveness when matching on str and nested non-exhaustive enums Aug 28, 2023
@Nadrieril
Copy link
Member

Thanks again @sebastiantoh! The logic looks sound. r=me once the phrasing nitpicks have been addressed

@sebastiantoh
Copy link
Contributor Author

sebastiantoh commented Sep 3, 2023

Thanks for the review! I've addressed the phrasing comments

@bors r=Nadrieril

@Nadrieril
Copy link
Member

hm maybe you need rights to do this

@bors r+

@bors
Copy link
Contributor

bors commented Sep 3, 2023

📌 Commit d87b87d has been approved by Nadrieril

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 3, 2023
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Sep 3, 2023
…eril

Add note on non-exhaustiveness when matching on str and nested non-exhaustive enums

Fixes rust-lang#105479

r? `@Nadrieril`
@bors
Copy link
Contributor

bors commented Sep 3, 2023

⌛ Testing commit d87b87d with merge 21305f4...

@bors
Copy link
Contributor

bors commented Sep 3, 2023

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 3, 2023
@bors bors merged commit 21305f4 into rust-lang:master Sep 3, 2023
12 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Sep 3, 2023
@sebastiantoh sebastiantoh deleted the issue-105479 branch September 3, 2023 23:30
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (21305f4): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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)
1.1% [0.8%, 1.4%] 6
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.5% [-0.5%, -0.5%] 1
All ❌✅ (primary) 1.1% [0.8%, 1.4%] 6

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)
- - 0
Improvements ✅
(primary)
-1.4% [-1.4%, -1.4%] 1
Improvements ✅
(secondary)
-3.4% [-4.5%, -1.3%] 3
All ❌✅ (primary) -1.4% [-1.4%, -1.4%] 1

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)
3.8% [1.7%, 6.1%] 5
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 3.8% [1.7%, 6.1%] 5

Binary size

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

Bootstrap: 629.273s -> 630.367s (0.17%)
Artifact size: 316.72 MiB -> 316.68 MiB (-0.01%)

@rustbot rustbot added the perf-regression Performance regression. label Sep 4, 2023
@pnkfelix
Copy link
Member

pnkfelix commented Sep 6, 2023

seems like continued spurious noise on bitmaps-3.1.0

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Be more specific on non-exhaustive matches on non-sum-types
7 participants