More de-abuse of TyKind::Error #71074
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
8 tasks
|
cc @Centril |
|
Does |
|
@eddyb Ah, I didn't try it because it failed on the last PR. But let me do so now... |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@mark-i-m It can fail in individual parts of the compiler, but it's the correct thing to pass to So I would always try it first then run |
|
@eddyb It almost works. |
|
You're talking about the use site in this PR? Or something else? |
|
@mark-i-m Are you building with system LLVM by any chance? I'm asking because of this: rust/src/test/ui/issues/issue-69841.rs Lines 1 to 2 in 3712e11 |
|
Yes, I am building with system llvm. Hmm... let me push the PR and we can see what CI says. I think that is the only failing test. |
mark-i-m
force-pushed
the
de-abuse-err-3
branch
2 times, most recently
from
9594f6e
to
ba914f1
Compare
Centril
reviewed
Apr 13, 2020
| @@ -893,11 +893,12 @@ impl<'tcx> Constructor<'tcx> { | |||
| // Treat all uninhabited types in non-exhaustive variants as | |||
| // `TyErr`. | |||
| (_, true, true) => cx.tcx.types.err, | |||
| // Treat all non-visible fields as `TyErr`. They can't appear | |||
| // in any other pattern from this match (because they are | |||
| // Treat all non-visible fields as `()`. They can't appear in | |||
There was a problem hiding this comment.
This seems arbitrary; cc @varkor @Nadrieril
There was a problem hiding this comment.
On the other hand it says that we are picking some inhabited dummy, so () is as good as any I suppose.
|
And it results in an ICE: |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
eddyb
reviewed
Apr 13, 2020
| (false, ..) => cx.tcx.types.err, | ||
| // to know they are uninhabited, so we choose an inhabited | ||
| // dummy. | ||
| (false, ..) => cx.tcx.types.unit, |
There was a problem hiding this comment.
Maybe the ICE is because this has a constructor with 0 leaves? You could try some type like u8 I guess.
But I wonder if ty::Error is specially-handled elsewhere in this module.
|
Grmbl I new this tyerr hack looked fishy. Ok, it looks like the code uses tyerr to signal something specific: namely that we shouldn't observe uninhabitedness of private fields and non-exhaustive variants. So the correct fix is to return the correct type of the field always in this match https://github.com/mark-i-m/rust/blob/fcf45999f7441721965cb3f07a996d2c11793365/src/librustc_mir_build/hair/pattern/_match.rs#L892 but make the uninhabitedness check aware of private fields or whatever. Replacing tyerr by units or anything else is probably not a good idea. EDIT: If stage0 ever finishes to compile I'll try some things |
|
Ah, so, the type flows into |
We could but I don't think that's needed. I believe we already do a lot of the right checks without relying on tyerr being there, so it's just a matter of making sure that's the case. |
|
Btw, if I only change the match block, we fail the following two tests (which IIUC is expected):
|
mark-i-m
force-pushed
the
de-abuse-err-3
branch
from
cb3ddc9
to
3594bf8
Compare
|
@mark-i-m I'm on it ! I think I've found a clean solution to this |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
That's what I have so far: master...Nadrieril:exhaustiveness-remove-tyerr . |
|
@Nadrieril does it make sense to wrap those structs with some other struct that forces you to decide whether to see fields or not? EDIT: created this zulip thread to discuss more: https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/uninhabitedness.20check |
mark-i-m
force-pushed
the
de-abuse-err-3
branch
from
3594bf8
to
a3acff1
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Status update: I rebased to keep the PR up to date, but we have hit a hard roadblock with this problem: #71074 (comment) |
mark-i-m
force-pushed
the
de-abuse-err-3
branch
from
a3acff1
to
c0765ac
Compare
|
I've pulled in @Nadrieril's commits from their branch (@Nadrieril I can also just give you direct access to this branch if you want). |
|
☔ The latest upstream changes (presumably #71556) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 21, 2020
…, r=varkor De-abuse TyKind::Error in exhaustiveness checking Replaces rust-lang#71074. Context: rust-lang#70866. In order to remove the use of `TyKind::Error`, I had to make sure we skip over those fields whose inhabitedness should not be observed. This is potentially error-prone however, since we must be careful not to mix filtered and unfiltered lists of patterns. I managed to hide away most of the filtering behind a new `Fields` struct, that I used everywhere relevant. I quite like the result; I think the twin concepts of `Constructor` and `Fields` make a good mental model. As usual, I tried to separate commits that shuffle code around from commits that require more thought to review. cc @varkor @Centril
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
r? @eddyb
Sorry, I feel pretty out of my depth here. I have no idea if what I'm doing is reasonable.
cc #70866