-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Enforce syntactical stability of const traits in HIR #135423
base: master
Are you sure you want to change the base?
Enforce syntactical stability of const traits in HIR #135423
Conversation
Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter cc @rust-lang/miri, @rust-lang/wg-const-eval |
), | ||
EvalResult::Unmarked => unmarked(span, def_id), | ||
} | ||
|
||
is_allowed | ||
} | ||
|
||
pub fn check_const_stability(self, def_id: DefId, span: Span, const_kw_span: Span) { |
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 function is analogous to check_optional_stability
but with the logic in eval_stability_allow_unstable
inlined, and which operating on const stability instead of regular stability.
And I don't think we have soft unstability for consts, so I asserted against that.
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.
That seems like a comment worth putting into the code.
Isn't #133999 doing essentially the same thing? Cc @fee1-dead |
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
FWIW, you can't do all the recursive stability checks in MIR. Currently, we allow something like #[rustc_const_stable(feature = "dummy", since = "1.0.0")]
const fn foo<T: ~const UnstableTrait>() { } If the feature for that unstable trait is enabled. For recursive stability, we would need to check the bounds (which is different than running passes on MIR bodies). Several more places for this check would be const impls, const traits, and associated type bounds on const traits, etc. |
@RalfJung: Kind of, but no. This implements a subset of that PR in a totally different way, and I put it up bc @fee1-dead mentioned that they wouldn't have time to finish up this work. |
@fee1-dead: I think that over complicates the problem a bit? Recursive const stability shouldn't need to be concerned with things that don't end up in the MIR, method calls in this case. Simply adding a where clause to a function feels separate from that -- after all, isn't recursive const stability checking motivated by making sure we don't expose new const intrinsics (and other functionality that would make the evaluator do new things) to stable const Rust? naming a where clause does not feel like that to me. |
Having talked with @compiler-errors, I agree with them. We should ensure that method calls follow the established recursive const stability rules, but the trait system so far hasn't done recursive stability and I don't think it has to start now. What is important is that if |
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 agree with the general approach and the test results look good. I'm not familiar enough with the implementation of our general stability checks to sign off on that, though.
r? compiler
@@ -1,6 +1,6 @@ | |||
//@ known-bug: #103507 | |||
|
|||
#![feature(const_trait_impl)] | |||
#![feature(const_trait_impl, const_drop)] |
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.
#![feature(const_trait_impl, const_drop)] | |
#![feature(const_trait_impl, const_destruct)] |
), | ||
EvalResult::Unmarked => unmarked(span, def_id), | ||
} | ||
|
||
is_allowed | ||
} | ||
|
||
pub fn check_const_stability(self, def_id: DefId, span: Span, const_kw_span: Span) { |
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.
That seems like a comment worth putting into the code.
Uh what, we got two reviewers?^^ r? compiler |
This PR enforces what I'm calling syntactical const stability of traits. In other words, it enforces the ability to name
~const
/const
traits in trait bounds in various syntax positions in HIR (including in the trait of an impl header). This functionality is analogous to the regular item stability checker, which is concerned with making sure that you cannot refer to unstable items by name, and is implemented as an extension of that pass.This is separate from enforcing the recursive const stability of const trait methods, which is implemented in MIR and runs on MIR bodies. That will require adding a new
NonConstOp
to the const checker and probably adjusting some logic to deduplicate redundant errors.However, this check is separate and necessary for making sure that users don't add
~const
/const
bounds to items when the trait is not const-stable in the first place. I chose to separate enforcing recursive const stability out of this PR to make it easier to review. I'll probably open a follow-up following this one, blocked on this PR.r? @RalfJung cc @rust-lang/project-const-traits