Skip to content

Fix exhaustiveness checking of strings#78553

Merged
bors merged 2 commits intorust-lang:masterfrom
Nadrieril:fix-78549
Nov 1, 2020
Merged

Fix exhaustiveness checking of strings#78553
bors merged 2 commits intorust-lang:masterfrom
Nadrieril:fix-78549

Conversation

@Nadrieril
Copy link
Copy Markdown
Member

@Nadrieril Nadrieril commented Oct 30, 2020 •
edited
Loading

Before #78430, the code worked because specialize_constructor didn't actually care too much which constructor was passed to it unless needed. That PR however handles &str as a special case, and I did not anticipate patterns for the &str type other than string literals.
I am not very confident there are not other similar oversights left, but hopefully only &str was different enough to break my assumptions.

Fixes #78549

@rustbot rustbot added the A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns label Oct 30, 2020
@rust-highfive
Copy link
Copy Markdown
Contributor

rust-highfive commented Oct 30, 2020

r? @varkor

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 30, 2020
@jyn514 jyn514 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 30, 2020
varkor
varkor reviewed Oct 30, 2020
View reviewed changes
@Nadrieril
Copy link
Copy Markdown
Member Author

Nadrieril commented Oct 30, 2020 •
edited
Loading

After a good night's sleep I found a way of solving the issue that involves fewer footguns. I feel a bit more confident about my solution now.

@Nadrieril
Copy link
Copy Markdown
Member Author

Nadrieril commented Oct 30, 2020

@varkor it's ready to go

@Nadrieril Nadrieril requested a review from varkor October 30, 2020 21:22
@jyn514 jyn514 mentioned this pull request Oct 31, 2020
Closed
@Nadrieril Nadrieril mentioned this pull request Nov 1, 2020
Closed
@Nadrieril
Fix rust-lang#78549
4cd3019 
Before rust-lang#78430, string literals worked because `specialize_constructor`
didn't actually care too much which constructor was passed to it unless
needed. Since then, string literals are special cased and a bit hacky. I
did not anticipate patterns for the `&str` type other than string
literals, hence this bug. This makes string literals less hacky.
@Nadrieril
The need for Single to cover Unlistable was a hack
1bdcd02 
It is now unneeded, since we handle `&str` patterns in a consistent way.
@varkor
Copy link
Copy Markdown
Contributor

varkor commented Nov 1, 2020

@bors r+

@bors
Copy link
Copy Markdown
Collaborator

bors commented Nov 1, 2020

📌 Commit 1bdcd02 has been approved by varkor

@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 Nov 1, 2020
@bors
Copy link
Copy Markdown
Collaborator

bors commented Nov 1, 2020

⌛ Testing commit 1bdcd02 with merge 1899c48...

@bors
Copy link
Copy Markdown
Collaborator

bors commented Nov 1, 2020

☀️ Test successful - checks-actions
Approved by: varkor
Pushing 1899c48 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 1, 2020
@bors bors merged commit 1899c48 into rust-lang:master Nov 1, 2020
@rustbot rustbot added this to the 1.49.0 milestone Nov 1, 2020
@Nadrieril Nadrieril deleted the fix-78549 branch November 1, 2020 17:13
@Mark-Simulacrum
Copy link
Copy Markdown
Member

Mark-Simulacrum commented Nov 3, 2020

A fairly large improvement on perf, up to 10% on instructions and this is reflected with a roughly 3% win on wall times. Good work.

@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Nov 3, 2020
Merged
@Nadrieril
Copy link
Copy Markdown
Member Author

Nadrieril commented Nov 3, 2020 •
edited
Loading

Thanks! I was not expecting a perf gain from this one ^^.
I have to point out that the match-stress-enum benchmark is not representative though. Patterns in the wild almost always have depth and matches don't have more than a couple branches. match-stress-enum has a billion branches and no depth. I think it is good for monitoring hot paths in exhaustiveness checking, but I wouldn't trust it to indicate differences on real workloads.
EDIT: oh I hadn't seen the "bootstrap timings" section, those seem to indicate real gains :o

@Nadrieril Nadrieril changed the title Fix #78549 Fix exhaustiveness checking of strings Nov 18, 2020
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@varkor varkor Awaiting requested review from varkor

Assignees

@varkor varkor

Labels

A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns merged-by-bors This PR was explicitly merged by bors. 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

Milestone

1.49.0

Development

Successfully merging this pull request may close these issues.

ICE: trying to compare incompatible ctors while building rustfmt

7 participants

@Nadrieril @rust-highfive @varkor @bors @Mark-Simulacrum @jyn514 @rustbot