-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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.
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
privacy: normalize associated types before visiting #126076
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
This permits associated types to reference private types and traits, so long as the normalized type does not itself violate type privacy. Fixes #45713
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -1307,7 +1307,19 @@ impl SearchInterfaceForPrivateItemsVisitor<'_> { | |||||||
|
||||||||
fn ty(&mut self) -> &mut Self { | ||||||||
self.in_primary_interface = true; | ||||||||
self.visit(self.tcx.type_of(self.item_def_id).instantiate_identity()); | ||||||||
let ty = self.tcx.type_of(self.item_def_id).instantiate_identity(); | ||||||||
|
||||||||
// If `in_assoc_ty`, attempt to normalize `ty`. | ||||||||
// Ideally, we would normalize in all circumstances, but doing so | ||||||||
// currently causes some unexpected type errors. | ||||||||
let maybe_normalized_ty = if self.in_assoc_ty { | ||||||||
let param_env = self.tcx.param_env(self.item_def_id); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, let's actually try using the new trait solver, and do it everywhere. So you'll need to actually do like how @lcnr originally suggested to you 😆 I would also pull this into a helper function so that you can just propagate the error out, then do:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! This narrows us down to a single ICE on
...which produces an ICE here: rust/compiler/rustc_infer/src/infer/freshen.rs Lines 174 to 176 in 3ea5e23
The full output is:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. *it gets us down to own ICE if I fallback to the unnormalized type in the event of errors. If I don't do that and
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||
self.tcx.try_normalize_erasing_regions(param_env, ty).ok() | ||||||||
compiler-errors marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
} else { | ||||||||
None | ||||||||
}; | ||||||||
|
||||||||
self.visit(maybe_normalized_ty.unwrap_or(ty)); | ||||||||
compiler-errors marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
self | ||||||||
} | ||||||||
|
||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This visitor is supposed to normalize everywhere, not just in associated types.
If that's not possible for some technical reasons, then it's fine to normalize only in a subset of cases, just need to leave an explaining comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, removing the
self.in_assoc_ty
check caused quite a few unexpected errors (e.g., recursion limit errors). I'll leave a comment to that effect.I don't quite have the expertise to debug these in short-order, and the narrow issue of normalization in associated types is what's blocking me from releasing zerocopy 0.8. If it's possible to merge this narrow fix before 1.80 branches from master, I'd be quite grateful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're delaying it to 1.81 anyway, let's try out both using the new solver and always trying to normalize, even for non-assoc types.