-
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
Represent trait constness as a distinct predicate #131985
base: master
Are you sure you want to change the base?
Conversation
Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred in src/librustdoc/clean/types.rs cc @camelid This PR changes Stable MIR cc @oli-obk, @celinval, @ouz-a Some changes occurred in match checking cc @Nadrieril changes to the core type system changes to the core type system Some changes occurred in match lowering cc @Nadrieril HIR ty lowering was modified cc @fmease |
This comment has been minimized.
This comment has been minimized.
127ab2c
to
1139e91
Compare
This comment has been minimized.
This comment has been minimized.
1139e91
to
efc40e1
Compare
☔ The latest upstream changes (presumably #131980) made this pull request unmergeable. Please resolve the merge conflicts. |
efc40e1
to
a2df5ea
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.
Left some comments after an initial pass, looking pretty nice! Will leave more structured thoughts on the Zulip thread.
// We don't lower any bounds except for `~const` for `ConstIfConst` | ||
// filtering. |
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.
The wording is a little roundabout here. I feel like all occurrences of this could be replaced with something like
// We don't lower any bounds except for `~const` for `ConstIfConst` | |
// filtering. | |
// `ConstIfConst` is only interested in `~const` bounds. |
// FIXME(effects): We should be enforcing these effects unconditionally. | ||
// This can be done as soon as we convert the standard library back to | ||
// using const traits, since if we were to enforce these conditions now, | ||
// we'd fail on basically every builtin trait call (i.e. `1 + 2`). |
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.
👍
PredicateFilter::ConstIfConst => { | ||
for (clause, _) in bounds { | ||
match clause.kind().skip_binder() { | ||
ty::ClauseKind::HostEffect(_) => {} |
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.
could probably check that the predicate indeed has HostPolarity::Maybe
here also.
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.
Indeed.
} | ||
} | ||
} | ||
_ => return Default::default(), |
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.
could extend to DefKind::OpaqueTy
or even DefKind::Closure
later? leave a comment here so we can come back to this.
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.
Yes, definitely we will need to. Both opaques and (const) closures should inherit the const conditions of their calling function. I'll leave a comment.
} | ||
} | ||
|
||
let (generics, is_trait, has_parent) = match tcx.hir_node_by_def_id(def_id) { |
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.
rename is_trait
to trait_id_and_supertraits
or something else that is clearer
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.
👍
predicates.extend( | ||
tcx.const_conditions(def_id) | ||
.instantiate_identity(tcx) | ||
.into_iter() | ||
.map(|(trait_ref, _)| trait_ref.to_host_effect(tcx, ty::HostPolarity::Maybe)), | ||
); |
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.
add some comments here
// `T: ~const Trait` implies `T: ~const Supertrait`. | ||
ty::ClauseKind::HostEffect(data) => self.extend_deduped( | ||
cx.implied_const_bounds(data.def_id()).iter_identity().map(|trait_ref| { | ||
elaboratable.child( | ||
trait_ref | ||
.to_host_effect(cx, data.host) | ||
.instantiate_supertrait(cx, bound_clause.rebind(data.trait_ref)), | ||
) | ||
}), | ||
), |
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.
Nice
@@ -111,6 +111,13 @@ impl<I: Interner> ty::Binder<I, TraitRef<I>> { | |||
pub fn def_id(&self) -> I::DefId { | |||
self.skip_binder().def_id | |||
} | |||
|
|||
pub fn to_host_effect(self, cx: I, host: HostPolarity) -> I::Clause { |
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.
could be named to_host_effect_clause
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 I like that better
@@ -37,6 +37,9 @@ pub enum ClauseKind<I: Interner> { | |||
|
|||
/// Constant initializer must evaluate successfully. | |||
ConstEvaluatable(I::Const), | |||
|
|||
/// Whether `T: Trait`'s host effect is `true`/`false`. |
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 probably outdated
tests/crashes/118320.rs
Outdated
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.
could move these crashes to a ui test
/// This query also computes the `~const` where clauses for associated types, which are | ||
/// not "const", but which have item bounds which may be `~const`. These must hold for | ||
/// the `~const` item bound to hold. | ||
pub(super) fn const_conditions<'tcx>( |
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.
what does kinda suck tho is that we do need to end up duplicating a fair portion of gather_explicit_predicates_of in the const_conditions impl because they differ just enough that it's kinda not worth unifying atm, tho that could probably be changed later.
predicates: tcx.arena.alloc_from_iter(bounds.clauses(tcx).map(|(clause, span)| { | ||
( | ||
clause.kind().map_bound(|clause| match clause { | ||
ty::ClauseKind::HostEffect(ty::HostEffectPredicate { |
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 kinda sucks that for the sake of not rewriting all of the code, we end up collecting a list of HostEffect(T: Trait, Maybe) then stripping away everything but the trait ref when actually in the const condition query lol
I don't see how functions like lower_poly_trait_ref
could easily be made to be generic over their output, tho.
QueryResult, | ||
}; | ||
|
||
impl<D, I> assembly::GoalKind<D> for ty::HostEffectPredicate<I> |
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.
beware that the only thing that is not yet implemented is ~const
in item bounds. Not because I couldn't do it, but mostly because I didn't want to complicate the new trait solver implementation yet.
So this code doesn't work yet:
#![feature(const_trait_impl, effects)]
#[const_trait]
trait Bar {}
#[const_trait]
trait Foo {
type Bar: ~const Bar;
}
const fn needs_const_bar<T: ~const Bar>() {}
const fn test<T: ~const Foo>() {
needs_const_bar::<T::Bar>();
}
We just need to change the item bound candidate assembly to also have a callback for assembling "custom" item bounds, tho.
@@ -150,6 +150,13 @@ fn param_env(tcx: TyCtxt<'_>, def_id: DefId) -> ty::ParamEnv<'_> { | |||
}); | |||
} | |||
|
|||
predicates.extend( |
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.
Here we extend the param-env with the ~const
bounds of the item, so that we can type check it with those assumptions.
PredicateFilter::ConstIfConst => { | ||
for (clause, _) in bounds { | ||
match clause.kind().skip_binder() { | ||
ty::ClauseKind::HostEffect(_) => {} |
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.
Indeed.
None => return, | ||
}; | ||
|
||
if self.tcx.is_const_fn(callee_did) |
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 think it can be is_const_fn
so we don't report both a const stability error and a ~const
bound error. But I don't think it matters 🤔 definitely worth investigation, I'll leave a comment.
ty::ClauseKind::HostEffect(predicate) => { | ||
p!("the trait `", print(predicate.trait_ref), "` is const") | ||
} |
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.
Honestly I think I should just change this to stop using words, and just render it like T: ~const Trait
.
@@ -0,0 +1,295 @@ | |||
//! Dealing with trait goals, i.e. `T: Trait<'a, U>`. |
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.
Yes, copypasta 🍝 I will fix.
@@ -690,6 +689,9 @@ impl<'tcx> Stable<'tcx> for ty::ClauseKind<'tcx> { | |||
ClauseKind::ConstEvaluatable(const_) => { | |||
stable_mir::ty::ClauseKind::ConstEvaluatable(const_.stable(tables)) | |||
} | |||
ClauseKind::HostEffect(..) => { | |||
todo!() |
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'll leave this unimplemented currently, at least until we get consensus that this is something we want :D
@@ -111,6 +111,13 @@ impl<I: Interner> ty::Binder<I, TraitRef<I>> { | |||
pub fn def_id(&self) -> I::DefId { | |||
self.skip_binder().def_id | |||
} | |||
|
|||
pub fn to_host_effect(self, cx: I, host: HostPolarity) -> I::Clause { |
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 I like that better
cc @rust-lang/project-const-traits
r? @ghost for now
Also mirrored everything that is written below on this hackmd here: https://hackmd.io/@compiler-errors/r12zoixg1l
Tl;dr:
I'm putting this up both as a request for comments and a vibe-check, but also as a legitimate implementation that I'd like to see land (though no rush of course on that last part).
Background
Early days
Once upon a time, we represented trait constness in the param-env and in
TraitPredicate
. This was very difficult to implement correctly; it had bugs and was also incomplete; I don't think this was anyone's fault though, it was just the limit of experimental knowledge we had at that point.Dealing with
~const
within predicates themselves meant dealing with constness all throughout the trait solver. This was difficult to keep track of, and afaict was not handled well with all the corners of candidate assembly.Specifically, we had to (in various places) remap constness according to the param-env constness:
rust/compiler/rustc_trait_selection/src/traits/select/mod.rs
Line 1498 in 574b64a
This was annoying and manual and also error prone.
Beginning of the effects desugaring
Later on, #113210 reimplemented a new desugaring for const traits via a
<const HOST: bool>
predicate. This essentially "reified" the const checking and separated it from any of the remapping or separate tracking in param-envs. For example, if I was in a const-if-const environment, but I wanted to call a trait that was non-const, this reification would turn the constness mismatch into a simple type mismatch of the effect parameter.While this was a monumental step towards straightening out const trait checking in the trait system, it had its own issues, since that meant that the constness of a trait (or any item within it, like an associated type) was early-bound. This essentially meant that
<T as Trait>::Assoc
was distinct from<T as ~const Trait>::Assoc
, which was bad.Associated-type bound based effects desugaring
After this, #120639 implemented a new effects desugaring. This used an associated type to more clearly represent the fact that the constness is not an input parameter of a trait, but a property that could be computed of a impl. The write-up linked in that PR explains it better than I could.
However, I feel like it really reached the limits of what can comfortably be expressed in terms of associated type and trait calculus. Also,
<const HOST: bool>
remains a synthetic const parameter, which is observable in nested items like RPITs and closures, and comes with tons of its own hacks in the astconv and middle layer.For example, there are pieces of unintuitive code that are needed to represent semantics like elaboration, and eventually will be needed to make error reporting intuitive, and hopefully in the future assist us in implementing built-in traits (eventually we'll want something like
~const Fn
trait bounds!).elaboration hack:
rust/compiler/rustc_type_ir/src/elaborate.rs
Lines 133 to 195 in 8069f8d
trait bound remapping hack for diagnostics:
rust/compiler/rustc_trait_selection/src/error_reporting/traits/fulfillment_errors.rs
Lines 2370 to 2413 in 8069f8d
I want to be clear that I don't think this is a issue of implementation quality or anything like that; I think it's simply a very clear sign that we're using types and traits in a way that they're not fundamentally supposed to be used, especially given that constness deserves to be represented as a first-class concept.
What now?
This PR implements a new desugaring for const traits. Specifically, it introduces a
HostEffect
predicate to represent the obligation an impl is const, rather than using associated type bounds and the compat trait that exists for effects today.HostEffect
predicateA
HostEffect
clause has two parts -- theTraitRef
we're trying to prove, and aHostPolarity::{Maybe, Const}
.HostPolarity::Const
corresponds toT: const Trait
bounds, which must always be proven as const, and which can be written in any context. These are lowered directly into the predicates of an item, since they're not "context-specific".On the other hand,
HostPolarity::Maybe
corresponds toT: ~const Trait
bounds which must only exist in a conditionally-const context like a method in a#[const_trait]
, or aconst fn
free function. We do not lower these immediately into the predicates of an item; instead, we collect them into a new query called theconst_conditions
. These are the set of trait refs that we need to prove have const implementations for an item to be const.Notably, they're represented as bare (poly) trait refs because they are meant to be paired back together with a
HostPolarity
when they're being registered in typeck (see next section).For example, given:
foo
's const conditions would containT: A
, but notT: B
. On the flip side, foo's predicates (predicates_of
) query would containHostEffect(T: B, HostPolarity::Const)
but notHostEffect(T: A, HostPolarity::Maybe)
since we don't need to prove that predicate in a non-const environment (and it's not even the right predicate to prove in an unconditionally const environment).Type checking const bodies
When type checking bodies in HIR, when we encounter a call expression, we additionally register the callee item's const conditions with the
HostPolarity
from the body we're typechecking (Const
for unconditionally const things likeconst
/static
items, andMaybe
for conditionally const things like const fns; and we don't registerHostPolarity
predicates for non-const bodies).When type-checking a conditionally const body, we augment its param-env with
HostEffect(..., Maybe)
predicates.Checking that const impls are WF
We extend the logic in
compare_method_predicate_entailment
to also check the const-conditions of the impl method, to make sure that we error for:We also extend the WF check for impls to register the const conditions of the trait that is being implemented. This is to make sure we error for:
Proving a
HostEffect
predicateWe have several ways of proving a
HostEffect
predicate:HostEffect
predicate from the param-env~const
where clauses).Later I expect that we will add more built-in implementations for things like
Fn
.What next?
After this PR, I'd like to split out the work more so it can proceed in parallel and probably amongst others that are not me.
HostEffect
goal for places in HIR typeck that correspond to call terminators, like autoderef.HostEffect
rules for traits likeFn
.So what?
This ends up being super convenient basically everywhere in the compiler. Due to the design of the new trait solver, we end up having an almost parallel structure to the existing trait and projection predicates for assembling
HostEffect
predicates; adding new candidates and especially new built-in implementations is now basically trivial, and it's quite straightforward to understand the confirmation logic for these predicates.Same with diagnostics reporting; since we have predicates which represent the obligation to prove an impl is const, we can simplify and make these diagnostics richer without having to write a ton of logic to intercept and rewrite the existing
Compat
trait errors.Finally, it gives us a much more straightforward path for supporting the const effect on the old trait solver. I'm personally quite passionate about getting const trait support into the hands of users without having to wait until the new solver lands1, so I think after this PR lands we can begin to gauge how difficult it would be to implement constness in the old trait solver too. This PR will not do this yet.
Footnotes
Though this is not a prerequisite or by any means the only justification for this PR. ↩