-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Followup to #83944 #84377
Followup to #83944 #84377
Conversation
…ead of trait_ref_hack
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.
Seems like this might make things simpler. Thoughts?
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 is looking good. Left a few comments.
hir::GenericBound::LangItemTrait(_, _, hir_id, _) if self.trait_ref_hack.is_none() => { | ||
self.map.late_bound_vars.insert(*hir_id, vec![]); | ||
hir::GenericBound::LangItemTrait(_, _, hir_id, _) => { | ||
self.map.late_bound_vars.insert(*hir_id, binders); |
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.
Except that I don't quite get why we need any code here at all. It seems like the LangItemTrait
is basically a "poly trait ref" that binds no variables at all. We could probably pull out a helper from visit_poly_trait_ref
that does the exact same setup code -- which would be good -- or else delete this code altogether, because it's not really needed. I can't quite see any case where introducing a Binder
here changes behavior.
I guess the only change could be that there is a PolyTraitRef
boundary (with no variables) instead of a TraitRefBoundary
binder. Are those equivalent? I would think so (does that suggest a further simplification is possible?).
@@ -618,6 +567,43 @@ fn late_region_as_bound_region<'tcx>(tcx: TyCtxt<'tcx>, region: &Region) -> ty:: | |||
} | |||
} | |||
|
|||
impl<'a, 'tcx> LifetimeContext<'a, 'tcx> { | |||
/// Returns the binders in scope and the type of `Binder` that should be created for a poly trait ref. |
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.
Maybe link to the Concatenating
variant for more comments
this looks great. @bors r+ |
📌 Commit c78724f has been approved by |
…omatsakis Followup to rust-lang#83944 Some cleanups requested by `@nikomatsakis` r? `@nikomatsakis`
Rollup of 7 pull requests Successful merges: - rust-lang#84343 (Remove `ScopeTree::closure_tree`) - rust-lang#84376 (Uses flex to fix formatting of h1 at any width) - rust-lang#84377 (Followup to rust-lang#83944) - rust-lang#84396 (Update LLVM submodule) - rust-lang#84402 (Move `sys_common::rwlock::StaticRWLock` etc. to `sys::unix::rwlock`) - rust-lang#84404 (Check for intrinsics before coercing to a function pointer) - rust-lang#84413 (Remove `sys::args::Args::inner_debug` and use `Debug` instead) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Some cleanups requested by @nikomatsakis
r? @nikomatsakis