Skip to content

Conversation

Nadrieril
Copy link
Member

@Nadrieril Nadrieril commented Oct 30, 2020

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
Contributor

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
@Nadrieril
Copy link
Member Author

Nadrieril commented Oct 30, 2020

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
Member Author

@varkor it's ready to go

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.
It is now unneeded, since we handle `&str` patterns in a consistent way.
@varkor
Copy link
Contributor

varkor commented Nov 1, 2020

@bors r+

@bors
Copy link
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
Collaborator

bors commented Nov 1, 2020

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

@bors
Copy link
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
Member

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.

@Nadrieril
Copy link
Member Author

Nadrieril commented Nov 3, 2020

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

ICE: trying to compare incompatible ctors while building rustfmt
7 participants