-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Handle opaques in the new solver (take 2?) #111473
Conversation
This comment has been minimized.
This comment has been minimized.
2baabb7
to
de57bac
Compare
It's not clear to me what is wrong with the current approach of having the opaque type relations in the external constraints. Do we have a write-up of this issue somewhere? |
☔ The latest upstream changes (presumably #111601) made this pull request unmergeable. Please resolve the merge conflicts. |
de57bac
to
aae783e
Compare
This comment has been minimized.
This comment has been minimized.
r? @lcnr |
if result.has_only_region_constraints() { | ||
return Ok(result); | ||
} | ||
//if result.has_only_region_constraints() { |
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.
Leave a comment why
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.
very nice, "a few nits", then r=me
ocx.infcx.tcx.fold_regions((*opaque_ty, *hidden_ty), |_, _| { | ||
ocx.infcx.next_nll_region_var( | ||
NllRegionVariableOrigin::Existential { from_forall: false }, | ||
) | ||
}); |
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.
can you use renumber_regions
here
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.
Region renumberer is private to rustc_borrowck::renumber
:^) I don't think it offers much benefit over just folding here.
} | ||
}, | ||
Reveal::All => { | ||
let actual = tcx.type_of(opaque_ty.def_id).subst(tcx, opaque_ty.substs); |
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.
try add a debug assertion that predefined opaques are empty
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.
:s
I would need to make the inner infcx reachable from this module, or at least expose a new EvalCtxt
helper here. I don't think it's worth just yet, so I'll leave a fixme.
@@ -0,0 +1,15 @@ | |||
#![feature(type_alias_impl_trait)] |
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.
also add test with type params for #110601 and close that issue, i think this should fix it?
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.
We actually don't need to include this commit anymore. It's actually fine that our HIR typeck results have a Map<DefId, HiddenTy>
, since we do the mapping from hidden ty to definition params during writeback:
rust/compiler/rustc_hir_typeck/src/writeback.rs
Lines 582 to 588 in 8b4b208
let hidden_type = hidden_type.remap_generic_params_to_declaration_params( | |
opaque_type_key, | |
self.fcx.infcx.tcx, | |
true, | |
); | |
self.typeck_results.concrete_opaque_types.insert(opaque_type_key.def_id, hidden_type); |
So I'll just land #111853 independently of this PR.
7b9f1ee
to
d576970
Compare
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor |
Alright this is ready for review once again, @lcnr. Left a bunch of comments and a few more modifications. Should be very easy to review now. |
This comment has been minimized.
This comment has been minimized.
@@ -1041,6 +1048,57 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { | |||
checker | |||
} | |||
|
|||
pub(super) fn register_predefined_opaques_in_new_solver(&mut 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.
how is that fine (for TAIT)? 🤔 aren't the preregistered opaque types in the wrong environment now, they have the generic params of the opaque while in the context of the defining scope?
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.
as a thought which also deals with rust-lang/trait-system-refactor-initiative#17
long term we could maybe end up with the following to replace DefiningAnchor
:
enum DefineOpaques {
// the current `DefiningAnchor::Bind`
Yes(&'tcx List<DefId>),
// We do not allow any new definitions for our opaques,
// but when using an opaque with `DefId`, we instantiate the
// inferred hidden type with these substs.
//
// This doesn't quite work because of lifetimes, but we can
// figure something out there :grin:
No(SomeMapCollection<DefId, EarlyBinder<Ty<'tcx>>),
}
anyways, your PR works for RPIT so I wouldn't mind us landing this as long as we put a fixme here.
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.
You're absolutely right lol. We shouldn't map RPITITs back to their declaration params during writeback if we're in the new trait solver. 🤦
I'll add a fixme anyways.
@@ -356,8 +354,11 @@ impl<'tcx> EvalCtxt<'_, 'tcx> { | |||
}), | |||
); | |||
ecx.add_goal(normalizes_to_goal); | |||
let _ = ecx.try_evaluate_added_goals()?; | |||
let _ = ecx.try_evaluate_added_goals().inspect_err(|_| { |
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.
let _ = ecx.try_evaluate_added_goals().inspect_err(|_| { | |
let _certainty = ecx.try_evaluate_added_goals().inspect_err(|_| { |
I guess r=me with FIXME for #111473 (comment) |
☔ The latest upstream changes (presumably #111919) made this pull request unmergeable. Please resolve the merge conflicts. |
…::normalize_opaque_type Co-authored-by: lcnr <[email protected]>
d576970
to
dd98198
Compare
@bors r=lcnr Follow-ups:
|
☀️ Test successful - checks-actions |
Finished benchmarking commit (eb9da7b): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 646.729s -> 646.591s (-0.02%) |
Implement a new strategy for handling opaques in the new solver.
First, queries now carry both their defining anchor and the opaques that were defined in the inference context at the time of canonicalization. These are both used to pre-populate the inference context used by the canonical query.
Second, use the normalizes-to goal to handle opaque types in the new solver. This means that opaques are handled like projection aliases, but with their own rules:
Opaque<'?0r> = HiddenTy1
andOpaque<?'1r> = HiddenTy2
equateHiddenTy1
andHiddenTy2
instead of defining them as different opaque type keys.