-
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
Add const generics to infer (and transitive dependencies) #59008
Conversation
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.
LGTM, modulo comments, some of which require someone who understands certain typesystem interactions - I've pinged @nikomatsakis but I suspect @rust-lang/wg-traits can also help here.
src/librustc/ty/relate.rs
Outdated
} | ||
} | ||
} | ||
// FIXME(const_generics): this is probably wrong (regarding TyProjection) |
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 wrong because it is a projection, like TyProjection
. cc @nikomatsakis
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.
Well, to be quite honest, I'm a bit confused by what the "normalization policy" is here. If this were doing eager normalization, as I would expect at the moment, then this doesn't seem wrong. But if it's lazy normalization, then indeed it is wrong. It seems like we can land it as is, but I think that overall it would be good to try and document and talk out the design of constants and const generics.
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.
Design meeting! :)
src/librustc/infer/const_variable.rs
Outdated
#[derive(Copy, Clone, Debug)] | ||
pub enum ConstVariableValue<'tcx> { | ||
Known { value: &'tcx ty::LazyConst<'tcx> }, | ||
Unknown { universe: ty::UniverseIndex }, |
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.
Is this enough? Don't we also need the type?
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.
Well, I think the type is carried in the ty::Const
wrapper, for better or worse:
#[inline]
pub fn mk_const_var(self, v: ConstVid<'tcx>, ty: Ty<'tcx>) -> &'tcx Const<'tcx> {
self.mk_const(ty::Const {
val: ConstValue::Infer(InferConst::Var(v)),
ty,
})
}
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.
It's not carried around in Unknown
, which I think is the part @eddyb was referring to. (There's some other discussion about this elsewhere, but there's a FIXME
in the code about this now.)
self.tcx.mk_lazy_const(ty::LazyConst::Evaluated( | ||
ty::Const { | ||
val: ConstValue::Placeholder(placeholder_mapped), | ||
ty: self.tcx.types.err, // FIXME(const_generics) |
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.
Sounds like PlaceholderConst
should contain the type.
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.
Yeah, it probably should. The reason I didn't do it earlier was it ended up propagating <'tcx>
everywhere and I wanted to make sure it was definitely necessary before going down that route.
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.
I attempted propagating <'tcx>
around when originally adding CanonicalVarKind::Const
and found it to be impossible because CanonicalVarInfo
s are allocated in the global arena.
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.
Sound like we need to bother @nikomatsakis.
But also, aren't const types supposed to be always be "global"?
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.
AFAIK, types are only global after they have been lifted and their dependencies on type parameters and such are taken care of.
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.
Type parameters are "global" in this sense, only inference variables aren't.
I'm saying that I don't think we should ever have a constant value in the type system with an uninferred type.
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.
Hmm. Something feels wrong here indeed. I'm trying too put my finger on what it is. I guess in short I don't like the idea of there being a type in the canonical-var-info -- I feel like types should in the canonicalized value.
I think what I would expect is that the placeholder doesn't replace the entire ty::Const
, but rather just the ConstValue
. In this way, it can be instantiated without needing the type.
(Similarly, in the existential case that appears above, you wouldn't make a "Fresh type variable" to represent its type -- that also seems wrong.)
☔ The latest upstream changes (presumably #56864) made this pull request unmergeable. Please resolve the merge conflicts. |
e1d5810
to
94b7984
Compare
This is currently blocked on rust-lang/ena#19 and response from @nikomatsakis regarding the review comments above. |
This comment has been minimized.
This comment has been minimized.
ena 0.12.0 is released. |
I missed a detail in the previous pull request to ena: this is now blocked on rust-lang/ena#21. |
☔ The latest upstream changes (presumably #58305) made this pull request unmergeable. Please resolve the merge conflicts. |
@varkor This should be unblocked now, just released |
This is now blocked on #59415. |
eb2e115
to
a5d19dc
Compare
I've addressed all the comments that aren't waiting on feedback from @nikomatsakis. |
Reassigning the remainder of the review to r? @nikomatsakis then. |
Anyone else from @rust-lang/wg-traits want to take a look at the questions I left for @nikomatsakis, too? |
Does this still block on @nikomatsakis and @rust-lang/wg-traits? |
@WiSaGaN Yes, althrough niko has responded that he'll add it to his list last week: |
Co-Authored-By: Gabriel Smith <yodaldevoid@users.noreply.github.com>
Co-Authored-By: Gabriel Smith <yodaldevoid@users.noreply.github.com>
Co-Authored-By: Gabriel Smith <yodaldevoid@users.noreply.github.com>
aa95b41
to
a68ed06
Compare
@bors r+ |
📌 Commit a68ed06 has been approved by |
Add const generics to infer (and transitive dependencies) Split out from #53645. This work is a collaborative effort with @yodaldevoid. There are a number of stubs. These are mainly to ensure we don't overlook them when completing the implementation, but are not necessary for the initial implementation. We plan to address these in follow up PRs. r? @eddyb / @nikomatsakis
☀️ Test successful - checks-travis, status-appveyor |
… r=eddyb Refactor `TypeVariableOrigin` Removes some unused variants and extracts the common `Span` field. As suggested in rust-lang#59008 (comment). r? @eddyb
Refactor `TypeVariableOrigin` Removes some unused variants and extracts the common `Span` field. As suggested in #59008 (comment). r? @eddyb
Split out from #53645. This work is a collaborative effort with @yodaldevoid.
There are a number of stubs. These are mainly to ensure we don't overlook them when completing the implementation, but are not necessary for the initial implementation. We plan to address these in follow up PRs.
r? @eddyb / @nikomatsakis