Always check ConstArgHasType even when otherwise ignoring#152931
Always check ConstArgHasType even when otherwise ignoring#152931khyperia wants to merge 1 commit intorust-lang:mainfrom
ConstArgHasType even when otherwise ignoring#152931Conversation
| { | ||
| let filtered_obligations = | ||
| unnormalized_obligations.into_iter().filter(|o| { | ||
| matches!(o.predicate.kind().skip_binder(), |
There was a problem hiding this comment.
| matches!(o.predicate.kind().skip_binder(), | |
| matches!(o.predicate.kind().no_bound_vars().unwrap(), |
I think this is safer in that we won't silently skip stuff if we unexpectedly have higher ranked const arg has type goals. Instead we'll loudly explode and can fix that 😌
|
thx for taking this on for me |
309ca91 to
620edeb
Compare
|
changes to the core type system cc @lcnr |
|
r? @nnethercote rustbot has assigned @nnethercote. Use Why was this reviewer chosen?The reviewer was selected based on:
|
Background ContextFirst, it's important to note that:
This PR does not fundamentally change the above. Second, in the past, we lowered many things to anon consts. In doing so, we implicitly caused a typecheck of constants to happen, by enforcing that the body of the anon const matched with the type of the anon const.
This typecheck always happend, regardless of if the usage of the anon const was inside a free type alias or trait object reference. In other words, these used to cause compiler errors:
However, recently (a few months ago) we changed how some constants are desugared, no longer generating anon consts for them, and instead representing them immediately. This caused consts in these locations to stop being typechecked. Actual ChangeEventually, we would like to begin wfchecking free type aliases and trait objects. Today is not that day, so instead, this PR is reintroducing typechecking for constants, to minimize the eventual breakage that will be caused by wfchecking type aliases and trait objects.
Important to note is that ConstArgHasType cannot interact with lifetimes from a Crater BreakageA few crates have already slipped in some "invalid" code, here's a crater run: #150322 (comment) There are no crater breakages around the trait objects part of this PR. Here's an example breakage of the free type alias part of this PR: error: the constant `MODE` is not of type `usize`
--> src/lfstd.rs:92:1
|
92 | type AllocScqRing<const MODE: bool> = ScqRing<Box<[CachePadded<AtomicUsize>]>, MODE>;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `usize`, found `bool`
|
note: required by a const generic parameter in `ScqRing`
--> src/scq.rs:47:23
|
47 | pub struct ScqRing<I, const MODE: usize> {
| ^^^^^^^^^^^^^^^^^ required by this const generic parameter in `ScqRing`Note that even though these type aliases are clearly incorrect, they are still useable by downstream code - this code compiles successfully before this PR, but fails after it: struct Struct<const N: usize> {}
type Foo<const B: bool> = Struct<B>;
type Bar<const N: usize> = Foo<N>;
let x: Bar<1_usize>;The breakage is small enough that one person (me) could easily help fix up broken crates. However, the longer we wait, the more breakage will creep in. We've waited a few months already so it's not desperately urgent, but still. |
|
r? BoxyUwU |
|
cc @rust-lang/types See: #152931 (comment) |
|
Team member @BoxyUwU 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. |
This PR checks that the type of constant arguments matches the generic parameter instantiated by them when checking that the containing item is well-formed. We do not go back to calling HIR typeck for them. Is that correct? |
| // cannot be referenced by a ConstArgHasType clause. However, | ||
| // under `generic_const_parameter_types`, it can. Ignore those | ||
| // predicates for now, to not have HKT-ConstArgHasTypes. | ||
| !kind.has_escaping_bound_vars() |
There was a problem hiding this comment.
this should always return true as we assert o.predicate.kind().no_bound_vars() being Some above, maybe also rewrite to assert!(o.predicate.kind().no_bound_vars().is_some());
There was a problem hiding this comment.
hmm? I don't think so:
#![feature(generic_const_parameter_types)]
Trait<'a, const N: &'a u32>
&dyn for<'b> Trait<'b, false>This has o.predicate.kind().no_bound_vars().unwrap() be Some, as the ConstArgHasType doesn't have a Binder with vars, but there's an escaping bound var 'b, that we skipped via principal.skip_binder().
Unfortunately I'm not able to write a test for this due to #153039
Perhaps nominal_obligations actually should return a Binder with vars in it and stuff in this case, but that's all unimplemented as of now AFAIK (and will be implemented in the work to wfcheck dyn types in general, not just ConstArgHasType).
There was a problem hiding this comment.
Your example is incomplete and I don't fully get what you mean? Do you want const N: &'a u32 and then the const arg type is higher ranked 🤔
In this case the assert o.predicate.kind().no_bound_vars().unwrap(); would trigger, would it not?
The check !kind.has_escaping_bound_vars() is just o.predicate.kind().skip_binder().has_escaping_bound_vars()?
fn no_bound_vars is implemented if self.value.has_escaping_bound_vars() { None } else { Some(self.skip_binder()) }, so the assertion and the boolean returned afterwards are the same
There was a problem hiding this comment.
Aah, shoot, I mucked this up in a refactoring, I think.
I meant to assert that o.predicate.kind() introduces no bound vars of its own, that it's an empty for<>. I didn't realize that .no_bound_vars() checks has_escaping_bound_vars, it doesn't check if self.bound_vars is empty.
(edit: and oh, yeah, sorry, typing too fast, I did mean const N: &'a u32 - the full example is in the linked issue)
There was a problem hiding this comment.
In other words, the construct that we could have, in pseudocode:
for<'b> ( for<> ConstArgHasType(_, &'b u32) )
^ this binder is from the syntax &dyn for<'b> Trait<'b, false>
^ this binder is from the return type of nominal_obligations
I want to assert that the inner binder is empty, because ConstArgHasType obligations should not be higher-ranked, and we need to reevaluate this code if they are.
But if the outer binder is non-empty, and the ConstArgHasType obligation references that parameter (only possible with feature(generic_const_parameter_types), not possible on stable rust), we should ignore the obligation, as a sort of "WIP, implement this in the work for generic_const_parameter_types, etc."
There was a problem hiding this comment.
ah I guess isntead of using no_bound_vars above it should be bound_vars().is_empty() or sth like that.
doublechecked with boxy (was only 90% sure of exact definitions of words), that is correct! "most" constants still lower to anonconsts still, it's just very simple stuff that is now directly represented (as of a few months ago), without lowering to an anon const. So, this PR is just covering those simple things that are no longer anonconsts. AFAIK, anonconsts always have been and continue to be HIR typechecked. |
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
620edeb to
c48caa2
Compare
fixes #149774
helping @BoxyUwU finish up #150322, this is a simple tweak/finishing-up of that PR.
this is a breaking change that crater detected has some issues with in Boxy's PR, and hence needs a t-types FCP. I can go and help fix those crates if/once the break is signed off on.