normalize adt fields during structural match checking #72897
Merged
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
|
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 |
|
Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
lcnr
force-pushed
the
structurally-match-normalize
branch
from
383ee81
to
4725af3
Compare
|
Apparently this works even if I believe it shouldn't... edit: I think this is ok as we only check if consts are structurally matchable (at least on stable), which should always be fully monomorphic. |
eddyb
reviewed
Jun 4, 2020
| @@ -251,7 +251,10 @@ impl<'a, 'tcx> TypeVisitor<'tcx> for Search<'a, 'tcx> { | |||
| // fields of ADT. | |||
| let tcx = self.tcx(); | |||
| for field_ty in adt_def.all_fields().map(|field| field.ty(tcx, substs)) { | |||
| if field_ty.visit_with(self) { | |||
| let ty = self.tcx().normalize_erasing_regions(ty::ParamEnv::empty(), field_ty); | |||
There was a problem hiding this comment.
ty::Param::empty() is incorrect - frankly I wonder if we should remove it.
You should have a ParamEnv around.
(couple minutes later)
Hang on, this entire file is missing real ParamEnvs?
That's liable to cause ICEs if generic parameters end up in these types.
There was a problem hiding this comment.
Does this block this PR? Afaik it's currently not possible on stable to hit this case.
There was a problem hiding this comment.
I may go ahead and fix this afterwards, but my current priority is mainly to fix #72896
Which probably means that we have to backport this to beta.
Another option would be to revert the structural match check for projections 🤔
There was a problem hiding this comment.
Its probably my fault that we don't have ParamEnvs threaded through here. This whole thing of switching from attributes to traits has been a real rabbit hole.
Still, I think landing this is better than reverting the structural match check for projections.
|
@bors r+ Lets land this band-aid now, even with the |
|
📌 Commit 4725af3 has been approved by |
bors
added
S-waiting-on-bors
|
Should this PR be backported to beta? |
|
@bors rollup=never p=1 |
|
⌛ Testing commit 4725af3 with merge 7262762fc854593a2e5b1ac0715027967415b888... |
|
@bors retry yield |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 10, 2020
Rollup of 9 pull requests Successful merges: - rust-lang#72706 (Add windows group to triagebot) - rust-lang#72789 (resolve: Do not suggest imports from the same module in which we are resolving) - rust-lang#72890 (improper ctypes: normalize return types and transparent structs) - rust-lang#72897 (normalize adt fields during structural match checking) - rust-lang#73005 (Don't create impl candidates when obligation contains errors) - rust-lang#73023 (Remove noisy suggestion of hash_map ) - rust-lang#73070 (Add regression test for const generic ICE in rust-lang#72819) - rust-lang#73157 (Don't lose empty `where` clause when pretty-printing) - rust-lang#73184 (Reoder order in which MinGW libs are linked to fix recent breakage) Failed merges: r? @ghost
spastorino
added
the
beta-nominated
spastorino
added
the
T-compiler
|
Discussed in todays T-compiler meeting. Approved for beta-backport. |
pnkfelix
added
the
beta-accepted
Mark-Simulacrum
removed
the
beta-nominated
Merged
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 15, 2020
…-morse,Mark-Simulacrum [beta] backport This is a beta backport rollup of the following: * [beta] Revert heterogeneous SocketAddr PartialEq impls rust-lang#73318 * Fix emcc failure for wasm32. rust-lang#73213 * Revert rust-lang#71956 rust-lang#73153 * [beta] Update cargo rust-lang#73141 * Minor: off-by-one error in RELEASES.md rust-lang#72914 * normalize adt fields during structural match checking rust-lang#72897 * Revert pr 71840 rust-lang#72989 * rust-lang/cargo#8361 * e658200 from rust-lang#72901 r? @ghost
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.
fixes #72896
currently only fixes the issue itself and compiles stage 1 libs.
I believe we have to use something else to normalize the adt fields here,
as I expect some partially resolved adts to cause problems 🤔
stage 1 libs and the test itself pass, not sure about the rest...
Will spend some more time looking into it tomorrow.
r? @pnkfelix cc @eddyb