Prevent opaque types in impl headers #87382
Conversation
rust-highfive
added
the
S-waiting-on-review
| if let ty::Opaque(def_id, _) = *ty.kind() { | ||
| self.tcx | ||
| .sess | ||
| .struct_span_err(sp, "cannot implement trait on type alias impl trait") |
There was a problem hiding this comment.
I have an idea that could help with this span, not sure if it pans out. Create a new visitor struct. Implement intravisit::Visitor for it, most notably implement the Visitor::visit_qpath method and use qpath_res to get the DefId. If it is equal to the opaque type's use that span. If no span is found during visiting, fall back to the one you have right now.
First visit the self_ty variable, and if no span is found, visit the tr variable.
There was a problem hiding this comment.
IIUC qpath_res doesn't work here since we're not in an Expr or Pat node, but the qpath should be resolved so we can extract its res anyway. However, when visiting the self_ty, the qpath's def_id is not the same as the opaque type's here. (It's almost as if the visitor is seeing the type alias node, while the TypeFolder sees the opaque type node)
|
i wonder if we need to walk trait bounds of the impl, too |
|
IIUC this is where the difference between walking and folding will be most visible: folding over the substs like this PR does will visit the trait bounds of the impl (and it was my understanding that we should do so), while walking will need to visit both the self_ty and the tr ? |
|
yea, I think a visitor is indeed better. I didn't mean walking the
What I mean here is that we just look at the generics on the traitref. So for as the We entirely ignore In order to get these generics, you can call |
|
☔ The latest upstream changes (presumably #87540) made this pull request unmergeable. Please resolve the merge conflicts. |
JohnCSimon
added
S-waiting-on-review
inquisitivecrystal
added
the
T-compiler
JohnCSimon
added
S-waiting-on-review
|
Triage: |
|
Ping from Triage: |
|
Sure. I do have a question for @oli-obk though: does the new lazy TAIT approach change anything with respect to the ICEs, "safe transmute" above, and so on. That is, do we still have the problem and the solution we talked about earlier is still the one to follow ? (Or maybe this would be fixed with the new lazy TAITs ?) |
|
No, lazy taits do not help here, they only change behavior within functions, not anything about their theoretically legal use sites. This site is still as problematic as in the linked comments in the main post |
|
Great, I'll update this PR, thanks @oli-obk ! |
|
Closing so that it doesn't need to be triaged periodically by the WG. Will reopen when I update this PR. |
prevent opaque types from appearing in impl headers cc `@lqd` opaque types are not distinguishable from their hidden type at the codegen stage. So we could either end up with cases where the hidden type doesn't implement the trait (which will thus ICE) or where the hidden type does implement the trait (so we'd be using its impl instead of the one written for the opaque type). This can even lead to unsound behaviour without unsafe code. Fixes rust-lang#86411. Fixes rust-lang#84660. rebase of rust-lang#87382 plus some diagnostic tweaks
Previously, #76940 forbade simple trait impls only from using opaque types, to prevent ICES.
However, this still allowed the same ICEs in codegen on more complex cases like #86411, and could also allow weaponization via a safe transmute, as shown in #84660.
This PR prevents opaque types from appearing in impl headers altogether as requested in Niko's comment.
Fixes #86411.
Fixes #84660.
r? @oli-obk