-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
check object safety of generic constants #78365
Conversation
2a7f342
to
0e419ef
Compare
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.
do you want to wait for the ControlFlow
PR before merging this?
src/test/ui/const-generics/const_evaluatable_checked/object-safety-ok-infer-err.stderr
Show resolved
Hide resolved
I think it's fine to merge this as is without waiting on the FCP on that PR to finish |
false | ||
fn visit_const(&mut self, ct: &ty::Const<'tcx>) -> bool { | ||
// First check if the type of this constant references `Self`. | ||
if self.visit_ty(ct.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.
Can you also add a test for this? That will need the const generics feature gate so you can have non-usize constants, but that's fine.
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 already check this, [u8; bar::<Self>]
only fails because of visit_ty
as bar is Const { ty: fn bar with substs [Self], ct: ZST }
.
If we did not add this visit_ty
here we would never fail for constants, which took me a while to find out
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.
wait... what? as far as I can tell you only have bar::<Self>()
, which is a constant of usize
type. Even if its body contains other stuff.
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, meant bar::<Self>()
. The issue here is that bar::<Self>
is a ZST of type fn() -> usize { bar<Self> }
, so we have to look at the type of constants to see if they reference Self
. The value of a const cannot reference Self
afaik
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 get that, but I don't understand how the constant we are visiting is ever anything but a constant with ty: usize
, where the constant's value may be Unevaluated(DefIdOfbar, [Self])
, but that value is essentially irrelevant for the visit_ty
here. Or is this during the visiting of the MIR of the usize
typed constant? I don't think it ever is that, as the visitor we are currently talking about is visiting the signature of the function
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.
So we are visiting [u8; bar::<Self>()]
and then the abstract const for the array length which is
[Leaf(Const { ty: fn() -> usize {bar::<Self>}, val: Value(Scalar(<ZST>)) }), FunctionCall(n0, [])]
So Self
is only mentioned in the type of the function object.
Or is this during the visiting of the MIR of the usize typed constant?
During the visiting of the AbstractConst
of the usize typed constant
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.
During the visiting of the AbstractConst of the usize typed constant
Oh, now I realize where my confusion is coming from. It's actually both!
So...
struct Foo<T, const X: T>(T);
isn't possible, thus no
trait Foo {
fn bar(&self) -> Foo<Self, something()>;
}
is possible either.
c0ab1b1
to
8546a80
Compare
@bors r+ |
📌 Commit 60bcc58 has been approved by |
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 didn't read this deeply but the tests seem to perform as I would expect.
Rollup of 10 pull requests Successful merges: - rust-lang#78152 (Separate unsized locals) - rust-lang#78297 (Suggest calling await on method call and field access) - rust-lang#78351 (Move "mutable thing in const" check from interning to validity) - rust-lang#78365 (check object safety of generic constants) - rust-lang#78379 (Tweak invalid `fn` header and body parsing) - rust-lang#78391 (Add const_fn in generics test) - rust-lang#78401 (resolve: private fields in tuple struct ctor diag) - rust-lang#78408 (Remove tokens from foreign items in `TokenStripper`) - rust-lang#78447 (Fix typo in comment) - rust-lang#78453 (Fix typo in comments) Failed merges: r? `@ghost`
As
Self
can only be effectively used in constants withconst_evaluatable_checked
this should not matter outside of it.Implements the first item of #72219
r? @oli-obk for now cc @nikomatsakis