-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Improve errors on const mismatches #90987
Improve errors on const mismatches #90987
Conversation
r? @jackh726 (rust-highfive has picked a reviewer for you, use r? to override) |
r? @BoxyUwU |
d7d3787
to
978b853
Compare
978b853
to
c3d3f4d
Compare
LL | fn foo5<const N: usize>() -> HasTrait<HasCastInTraitImpl<{ N + N }, { 2 * N }>> { | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| | ||
= help: try adding a `where` bound using this expression: `where [(); { M + 1 }]:` |
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 this needs updating to print out with substs applied too
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Don't see why CI fails, I did bless those tests. |
Can you check if this handles precedence right 🤔 fn foo<const M: usize>() -> [(); 3 * M];
let _: [(); 3 * 2 + N] foo::<{ 2 + N }>(); do we get a silly error message like |
yeah we do, but I think it shouldn't be too hard to include precedence of operations here when constructing the suggestion. I can try that. |
☔ The latest upstream changes (presumably #91104) made this pull request unmergeable. Please resolve the merge conflicts. |
Taking precedence into account when applying substitutions is anything but straightforward. I'd prefer to do this in another PR, since this is basically more effort than the original PR. For now I'd suggest we just fall back to the unsubstituted suggestion, in case we would output the same suggestion as the error. |
@BoxyUwU Would that be ok for you? |
03909d7
to
27facbf
Compare
Did decide to implement this, wasn't as hard as I initially thought. |
This comment has been minimized.
This comment has been minimized.
27facbf
to
517c887
Compare
This comment has been minimized.
This comment has been minimized.
517c887
to
08f70c5
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
0ba4a85
to
3e2ee2c
Compare
This comment has been minimized.
This comment has been minimized.
Ok, I only blessed the tests in ui/const-generics, but they all passed in that CI run. So it seems as if for some reason the |
☔ The latest upstream changes (presumably #91996) made this pull request unmergeable. Please resolve the merge conflicts. |
ty::ConstKind::Value(ConstValue::Scalar(scalar)) => match scalar.to_i64() { | ||
Ok(s) => { | ||
let s = format!("{}", s); | ||
|
||
if applied_subst { | ||
return Some(PrintStyle::Braces(None, s)); | ||
} else { | ||
return Some(PrintStyle::NoBraces(s)); | ||
} | ||
} | ||
Err(_) => return None, | ||
}, |
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 the cause of the CI failures. the CI is failing on 32 bit stuff which means if we have a usize
scalar here and call .to_i64
we get an InterpErrorInfo(InterpErrorInfoInner { kind: scalar size mismatch: expected 8 bytes but got 4 bytes instead, backtrace: None })
from this call but on 64bit it works fine because the sizes match.
the docs for to_i64
say "Fails if the scalar is a pointer" but I guess that is wrong .-.
this code would also just be wrong if the scalar was a u128 or something since those values cant all fit in a i128
not sure exactly what the right thing to replace this with is but we surely have code somewhere for printing this 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.
Thanks, that's a great catch. I'll try to figure out how to handle this correctly here.
…able error message
a5e50e3
to
57d137e
Compare
This comment has been minimized.
This comment has been minimized.
For some reason I had to add the |
57d137e
to
6ed9dfd
Compare
This comment has been minimized.
This comment has been minimized.
6ed9dfd
to
7b72ba8
Compare
The job Click to see the possible cause of the failure (guessed by this bot)
|
☔ The latest upstream changes (presumably #92556) made this pull request unmergeable. Please resolve the merge conflicts. |
Ping from triage: |
@BoxyUwU Any ideas why |
Not sure why your code isnt working but I looked around a bit and I think |
Fixes #86198
This allows us to suggest a correct anonymous constant in case there's a mismatch, e.g. in:
we now get a mismatch const error of the following form:
instead of the
found { M + 1 }
constant that was previously output.