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.

Sign up for GitHub

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

normalize adt fields during structural match checking #72897

Merged
merged 2 commits into from
Jun 11, 2020
Merged

normalize adt fields during structural match checking #72897

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
29508ce
normalize adt fields during structural match check
lcnr Jun 1, 2020
4725af3
remove assert
lcnr Jun 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/librustc_trait_selection/traits/structural_match.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

@eddyb eddyb Jun 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this block this PR? Afaik it's currently not possible on stable to hit this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 🤔

Copy link
Member

Choose a reason for hiding this comment

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

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.

debug!("structural-match ADT: field_ty={:?}, ty={:?}", field_ty, ty);

if ty.visit_with(self) {
// found an ADT without structural-match; halt visiting!
assert!(self.found.is_some());
return true;
Expand Down
23 changes: 23 additions & 0 deletions src/test/ui/match/issue-72896.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// run-pass
trait EnumSetType {
type Repr;
}

enum Enum8 { }
impl EnumSetType for Enum8 {
type Repr = u8;
}

#[derive(PartialEq, Eq)]
struct EnumSet<T: EnumSetType> {
__enumset_underlying: T::Repr,
}

const CONST_SET: EnumSet<Enum8> = EnumSet { __enumset_underlying: 3 };

fn main() {
match CONST_SET {
CONST_SET => { /* ok */ }
_ => panic!("match fell through?"),
}
}