WF-check all ty::Const's, not just array lengths. #70107
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
the
T-compiler
|
r? @varkor (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
eddyb
reviewed
Mar 18, 2020
|
Does #68977 (comment) compile after this change? |
|
It does not, it has the following errors: warning: the feature `const_generics` is incomplete and may cause the compiler to crash
--> $DIR/issue-68977.rs:1:12
|
LL | #![feature(const_generics)]
| ^^^^^^^^^^^^^^
|
= note: `#[warn(incomplete_features)]` on by default
error: constant expression depends on a generic parameter
--> $DIR/issue-68977.rs:31:44
|
LL | FxpStorageHelper<INT_BITS, FRAC_BITS>: FxpStorage,
| ^^^^^^^^^^
|
= note: this may fail depending on what value the parameter takes
error: aborting due to previous error |
You mean among |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Ah, I was wondering whether this also had implications for consts in arrays. That's fine, then. |
No, those (array lengths) were the only consts being checked, so nothing changes for them. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
don't know how to even start fixing the last failing test 🤷♂️ |
|
It should be producing a type-checking error but it doesn't. |
struct MyStruct<const N: usize>;
fn fact<const N: usize>() {
MyStruct::<{ N - 1 }>;
}
fn main() {}also fails with |
|
What about this? struct MyStruct<const N: usize>;
fn fact<const N: usize>() {
let x: MyStruct<{ N - 1 }> = MyStruct;
}
fn main() {}If this doesn't trigger WF checks, I don't know what does 😕. |
|
@eddyb this works, probably has something to do with the turbofish? i.e. that we don't check WF of const exprs as a param of an expr, ?? |
This comment has been minimized.
This comment has been minimized.
|
I left a comment with the problems with WF here: #68977 (comment). |
|
Open questions (will be edited while I make progress implementing this)
|
Always use the interned version.
|
lcnr
commented
Mar 19, 2020
eddyb
reviewed
Mar 19, 2020
src/librustc/ty/mod.rs
Outdated
| @@ -1535,6 +1541,7 @@ where | |||
| Types(I), | |||
| InputTypes(J), | |||
| ProjectionTypes(K), | |||
| WellFormedConst(Ty<'tcx>, I), | |||
There was a problem hiding this comment.
Predicate::walk_tys is even more cursed than Ty::walk, wow. At least I think your approach here is okay for now.
There was a problem hiding this comment.
FWIW, #70164 removes walk_tys completely, it was a rustdoc-only fragile hack.
eddyb
reviewed
Mar 19, 2020
eddyb
reviewed
Mar 19, 2020
|
It looks like it got canceled? |
|
Let's assume this was spurious. @bors r=eddyb |
|
📌 Commit 631ac9c has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-azure |
This was referenced Jun 3, 2020
Merged
|
This looks like it caused a pretty major regression in wall times (that has held up for some time now) -- https://perf.rust-lang.org/compare.html?start=680a4b2fbdca0dc6c5ceec826a8dbeabe28f305d&end=ff4aff6ce0f216c8cb8d40f432efaacdaca8095b&stat=wall-time -- most of those are tiny deltas but given that they've held up I suspect that this is indeed the cause. We don't observe a similar increase in instruction counts though. I don't see a perf run on this PR; was this expected? |
|
We ran perf in https://perf.rust-lang.org/compare.html?start=783139bd8fc3b94fac9a1bf81bba2c506e8221b6&end=eb0cfcd08d428eeeced6db60ee25d022715932de which looked fairly innocent to me. So this change is not expected 😅 |
|
Hm interesting! It might have also been a change upstream somewhere as we have been recently reworking some of the collector and frontend backends. Would you be up for posting a revert that we can run perf on to see if that recovers performance? |
|
@slightlyoutofphase: unfortunately, without this check, const generic would be unsound. The problem is really that const generics is incomplete and we're missing some critical components that are necessary to get them working properly. Const generics is unstable ("very unstable" in some sense in that it explicitly warns you things may break if you try to use them at the moment). We're grateful to have people testing them so that we can address the flaws before stabilising the feature, but we haven't yet reached the point where we can guarantee any kind of stability. Hopefully that will change before too long :) |
This was referenced Jun 30, 2020
@varkor is this unsound for fundamental reasons, or unsound merely because of temporary implementation shortcomings? I'm interested in the implement-a-trait-if-a-const-predicate-is-satisfied pattern because it has the potential to let me replace typic's type-level computations with plain old Rust. I'm curious whether this pattern will ever be soundly supported. |
|
Const generics would be unsound without this check. However, there should be a way to modify the code slightly to make these now-failing examples work: it's just that we haven't worked out the right way to do it yet. See this issue for more details: #68436. |
16 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #68977
This PR removes the special case for array length in
wf::computeandchecks the well formedness of all consts.
Changes
PredicateKind::WellFormedto take aGenericArgand updateswf::obligations.