Properly generalize unevaluated consts#154053
Properly generalize unevaluated consts#154053khyperia wants to merge 2 commits intorust-lang:mainfrom
Conversation
|
changes to the core type system cc @lcnr |
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
khyperia
force-pushed
the
generalize-unevaluated
branch
from
c14d5fa to
ac88926
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
| let normalized_alias = relation.try_eagerly_normalize_alias(alias); | ||
|
|
||
| if normalized_alias.is_ty_var() { | ||
| if term_vid(normalized_alias).is_some() { |
There was a problem hiding this comment.
| if term_vid(normalized_alias).is_some() { | |
| if normalized_alias.is_infer() { |
| // with `ShallowStructurallyRelateAliases` so we treat the outermost alias as rigid, | ||
| // ensuring this is never a tyvar. | ||
| assert!(!generalized_ty.is_ty_var()); | ||
| assert!(term_vid(generalized_term).is_none()); |
There was a problem hiding this comment.
| assert!(term_vid(generalized_term).is_none()); | |
| assert!(!generalized_term.is_infer()); |
| } | ||
| } | ||
|
|
||
| fn term_vid<'tcx>(term: Term<'tcx>) -> Option<TermVid> { |
There was a problem hiding this comment.
can you move this to a method on Term
There was a problem hiding this comment.
TermVid is defined locally in generalize.rs, which is the reason this is also locally here. Should I migrate TermVid to rustc_middle? Seems vaguely useful in general (ha) but I was trying to rein in my refactorings
There was a problem hiding this comment.
oh, i see yeah if you could move TermVid to whether we define Term/AliasTerm/etc that'd be great
| self.tcx.features().generic_const_exprs() && matches!(target_vid, TermVid::Const(_)); | ||
|
|
||
| let generalized_term = if self.next_trait_solver() | ||
| && !gce_const_fallback |
There was a problem hiding this comment.
GCE is incompatible with the new solver (we don't allow both to be used together) so this check probably doesn't do anything. should be able to remvoe gce_const_fallback
There was a problem hiding this comment.
this check probably doesn't do anything
removing it causes this test to fail:
error[E0119]: conflicting implementations of trait `From<GenericStruct<{T + 1}>>` for type `GenericStruct<{T + 1}>`
--> /d/rust/tests/ui/const-generics/issues/issue-89304.rs:8:1
|
LL | impl<const T: usize> From<GenericStruct<T>> for GenericStruct<{T + 1}> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: conflicting implementation in crate `core`:
- impl<T> From<T> for T;
error[E0119]: conflicting implementations of trait `From<GenericStruct<_>>` for type `GenericStruct<_>`
--> /d/rust/tests/ui/const-generics/issues/issue-89304.rs:14:1
|
LL | impl<const T: usize> From<GenericStruct<{T + 1}>> for GenericStruct<T> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: conflicting implementation in crate `core`:
- impl<T> From<T> for T;
error: aborting due to 2 previous errors
to be honest, I'm not going to pretend I know what's going on.
There was a problem hiding this comment.
yeah ok I guess the "gce isnt allowed with new solver" is wrong for coherence...
There was a problem hiding this comment.
sure yeah seems fine to me to keep this
| match source_term.kind() { | ||
| ty::TermKind::Ty(source_ty) => { | ||
| match source_ty.kind() { | ||
| &ty::Alias(ty::Projection, data) => { |
There was a problem hiding this comment.
we want to mirror this behaviour for projection consts, i.e. <T as Trait>::ASSOC_CONST under mGCA 🤔 Not quite sure what a test case would look like where this matters.
In general though I think instead of matching on the kind of the term and then on what kind of alias it is, we should match try convert to an AliasTerm and then match on the kind of that and handle ProjectionTy/ProjectionConst as one arm, and similarly for FreeTy/FreeConst/InherentConst/InherentTy etc
| } | ||
|
|
||
| Ok(()) | ||
| fn instantiate(&self, l: TermVid, r: ty::Term<'tcx>) { |
There was a problem hiding this comment.
| fn instantiate(&self, l: TermVid, r: ty::Term<'tcx>) { | |
| fn instantiate_var_raw(&self, l: TermVid, r: ty::Term<'tcx>) { |
and maybe a comment to indicate this is a thin wrapper around the actual var table instantiation methods. concerned that as is people might (wrongly) call this instead of instantiate_var or something since this sounds like it'd be the right thing to call but mostly isnt
| source_ct, | ||
| generalized_ct, | ||
| )?; | ||
| fn equate(&self, l: TermVid, r: TermVid) { |
There was a problem hiding this comment.
similarly maybe fn union_vars instead 🤔 + a comment explaining its just a wrapper around the infer var tables
3 participants
ty::Constin MIR body #153831// FIXME(ogca)(I am unaware of an issue for this) added in Implement MVP for opaque generic const arguments #150823r? @BoxyUwU