Closed eddyb closed 4 years ago
This seems equivalent to something like
type A<T> = [u8; std::mem::size_of::<T>()]; // ERROR the size for values of type `T` cannot be known at compilation time
where T
is "obviously" in scope, it's just not concrete enough for a use in a constant (the current error is weird though).
I don't like the idea of hiding the name T
, but if this is going to be done in resolve, we could setup a "barrier" making it an error to use it, like in this case
fn f<T>() {
fn f() {
std::mem::size_of::<T>(); // ERROR can't use generic parameters from outer function
}
}
, but I'd still prefer the option 2.
where
T
is "obviously" in scope, it's just not concrete enough for a use in a constant (the current error is weird though).
No, that's #43408, which is why I added #![feature(const_generics)]
to enable the fix above.
This is how it should work and maybe we should just apply an approach like #70452 to as many cases that won't cause query cycle errors, as we can, even before lazy normalization is ready with the full fix.
but if this is going to be done in resolve, we could setup a "barrier" making it an error to use it
Right, this matches my intuition for 1., another example of that error being:
fn f<T>() {
const _: usize = std::mem::size_of::<T>(); // ERROR can't use generic parameters from outer function
}
but I'd still prefer the option 2.
Me too, if for no other reason that we shouldn't paint ourselves into a corner if we can avoid it.
We've discussed this briefly during pre triage, I think this was I-nominated
in order to be discussed during some T-lang
and/or T-compiler
meeting. Prioritizing it as P-medium
mainly given that it requires nightly.
@spastorino "requires nightly" is misleading, I'm only using #![feature(const_generics)]
to bypass #43408 (enum
discriminant expressions are in the same boat as array lengths today).
I'll adjust the labels to match reality and reduce confusion.
More to the point: if we apply the #70452 treatment to enum
discriminants (as a partial #43408 fix), then #![feature(const_generics)]
won't be necessary.
Also, here's an example that compiles on nightly, I'll add it to the issue description:
#![feature(const_generics, arbitrary_enum_discriminant)]
#[repr(usize)]
enum MyWeirdOption<T> {
None = 0,
Some(T) = std::mem::size_of::<*mut T>(),
}
And using *mut *mut T
instead of *mut T
would make it work even when T: Sized
isn't known.
(If not for the substitution failure, you could remove #![feature(const_generics)]
)
I've just removed the ICE label, after reorganizing the issue description to indicate that the question in the title is the important part, and the ICE is just an example (bonus: the ICE becomes a regular error if we choose option 2, it's not exactly a bug, it's just not printed as a regular diagnostic).
@eddyb
I'm not sure this can be backwards-compatible with 2.
Is the reason for this that there might be some type in scope that would otherwise be shadowed?
e.g.,
struct Foo { }
impl Foo { const Constant: usize = 22; }
enum Bar<Foo> { X = Foo::Constant }
@nikomatsakis Shadowing is one of the theoretical concerns, sure. Note that you can also do this:
enum Bar<Foo> {
X = {
struct Foo { }
impl Foo { const Constant: isize = 22; }
Foo::Constant
}
}
But even scarier than shadowing is this:
enum Bar<FOO> {
X = {
const FOO: isize = 3;
struct Baz<T>(T);
Baz::<FOO>;
0
}
}
Currently errors with a #43408 error (which #![feature(const_generics)]
makes it go away), but if we suddenly remove the type parameter, const FOO
resolves instead (playground):
enum Bar {
X = {
const FOO: isize = 3;
struct Baz<T>(T);
Baz::<FOO>;
0
}
}
error[E0107]: wrong number of const arguments: expected 0, found 1
--> src/lib.rs:5:15
|
5 | Baz::<FOO>;
| ^^^ unexpected const argument
error[E0107]: wrong number of type arguments: expected 1, found 0
--> src/lib.rs:5:9
|
5 | Baz::<FOO>;
| ^^^^^^^^^^ expected 1 type argument
More to the point, this compiles today (playground), and relies only on the simplest subset of const
generics, so it might be stable in a year or two, but would break if FOO
resolved as a type parameter:
#![feature(const_generics)]
struct IsizeVal<const X: isize>;
impl<const X: isize> IsizeVal<X> {
const VAL: isize = X;
}
enum Bar</*FOO*/> {
X = {
const FOO: isize = 3;
IsizeVal::<FOO>::VAL
}
}
Personally, I think we should make the type parameters in scope, but I'm fine with being conservative in terms of how they may be used. I'd be ok with it being an error to even reference them, actually, but I think limiting ourselves to constant expressions that can be statically resolved is pretty reasonable. (Long term, I'd probably also be ok with moving these errors to be "post-monomorphization errors", perhaps with an opt-in, in exchange for looser rules, but I think we don't have to discuss that now.)
2 we keep the current name resolution support but error if we can't evaluate to concrete values
I'm also fine with this option with the restrictions to known concrete values given the compatibility hazards that the first option is introducing. Presumably, "making it an error to even reference them" as an added restriction would be implemented in resolve. I'm also not opposed to such an additional restriction if it is not too complicated to implement. (I'll just note that I'm not OK with more post-monomorphization errors -- I see it as a great strength of Rust that its model of generics works like Haskell and ML, not C++, but let's indeed defer that discussion.)
Note that the WF precautions we're taking with constants-in-types (see #70107), would result in "prove that this evaluates successfully" requirements bubbling up (when we get a syntax for said bubbling up, i.e. #68436), if we switch enum
discriminants from early evaluation to that system in the future.
So whichever part of the code passes in types/consts concrete enough for the value to evaluate, will get an error if it fails to evaluate (that's our general constants-in-types WF approach)
- we keep the current name resolution support but error if we can't evaluate to concrete values
Opened #70825 for this alternative (+ #70452-like treatment for explicit enum
discriminant expressions).
I'm not sure if this should still be nominated but in any case I guess it's nominated for T-lang
cc @eddyb
Based on the discussion thus far, I believe everyone who has commented is in favor of option 2. (see https://github.com/rust-lang/rust/issues/70453#issue-588822132) which means that the answer to the question in the title is: "Yes, enum discriminants should have generics in scope."
However, this also makes certain commitments which will be observable on stable, e.g., when #![feature(arbitrary_enum_discriminant)]
is stabilized. Therefore, to confirm this choice through the proper process, I would propose that we confirm that we are going with option 2 (https://github.com/rust-lang/rust/pull/70825).
With that said, @rfcbot merge
Team member @Centril has proposed to merge this. The next step is review by the rest of the tagged team members:
No concerns currently listed.
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!
See this document for info about what commands tagged team members can give me.
:bell: This is now entering its final comment period, as per the review above. :bell:
Unnominating this one given that it was already discussed in last compiler weekly meeting.
The final comment period, with a disposition to merge, as per the review above, is now complete.
As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.
The RFC will be merged soon.
What are our options?
rustc_resolve
) to hideenum
generic parameters from the discriminant expression!=
bounds#![feature(const_generics)]
(see examples below) by using the same approach as in #70452, asenum
s are in a similar situation that shouldn't cause query cyclesExamples of current behavior:
Note:
#![feature(const_generics)]
is used below so that this special-case kicks in: https://github.com/rust-lang/rust/blob/62c6006450d8bd33a351673c1f969846d768aab4/src/librustc_typeck/collect.rs#L1183-L1185Also, note that the reason for the uniform current treatment (which doesn't consider
enum
parameters as not in scope of discriminants), is becauserustc_resolve
treats surrounding generics as being in scope ofAnonConst
s, always - all the bugs later in compilation are due to the lack of lazy normalization (see #43408).This ICEs currently (playground):
with
Same thing happens if a
const
parameter is used instead ofsize_of::<T>()
.If we choose option 2, this ICE will turn into a regular error.
EDIT: and this currently compiles on nightly (playground):
And using
*mut *mut T
instead of*mut T
would make it work even whenT: Sized
isn't known. (If not for the substitution failure, you could remove#![feature(const_generics)]
)cc @rust-lang/compiler @rust-lang/lang