-
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
Fix various issues with control-flow statements inside anonymous constants #51967
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
Minor nitpicks, after that r=me.
src/librustc_mir/hair/pattern/mod.rs
Outdated
@@ -1152,7 +1156,11 @@ fn lit_to_const<'a, 'tcx>(lit: &'tcx ast::LitKind, | |||
ty::TyInt(other) => Int::Signed(other), | |||
ty::TyUint(UintTy::Usize) => Int::Unsigned(tcx.sess.target.usize_ty), | |||
ty::TyUint(other) => Int::Unsigned(other), | |||
_ => bug!(), | |||
ty::TyError => { | |||
// Avoid ICE |
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 Issue Number
src/librustc_mir/hair/pattern/mod.rs
Outdated
@@ -1123,7 +1127,7 @@ fn lit_to_const<'a, 'tcx>(lit: &'tcx ast::LitKind, | |||
tcx: TyCtxt<'a, 'tcx, 'tcx>, | |||
ty: Ty<'tcx>, | |||
neg: bool) | |||
-> Result<&'tcx ty::Const<'tcx>, ()> { | |||
-> Result<&'tcx ty::Const<'tcx>, bool> { |
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.
Use an enum instead of a boolean? That way you can communicate the meaning of the different states, not only in the method signature, but also at the return point and when being checked.
src/librustc/ty/layout.rs
Outdated
bug!("LayoutDetails::compute: unexpected type `{}`", ty) | ||
} | ||
ty::TyError => { |
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.
Merge with the ty::TyParam(_)
branch above, as they return the same thing.
src/librustc_mir/hair/pattern/mod.rs
Outdated
// Avoid ICE | ||
return Err(false); | ||
} | ||
_ => bug!("{:?}", ty.sty), |
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.
Expand the bug
description a little with something like "literal integer with bad type".
LL | [(); return |ice| {}]; | ||
| ^^^^^^^^^^^^^^^ | ||
|
||
error[E0572]: return statement outside of function body |
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 don't really like the wording here, but that is the current wording, so don't bother coming up with better wording if you can't think of one immediately (I can't).
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 thought the same thing. It's hard to think of something snappy. Perhaps simply:
`return` found outside a function
?
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.
Maybe "outside of function scope"? It'd be good to also expand the diagnostic to include an explanation that it is in a const scope, presenting a terse explanation that whatever is in such a scope will be evaluated at compile time, as well as a link to the documentation (https://doc.rust-lang.org/book/second-edition/ch03-01-variables-and-mutability.html#differences-between-variables-and-constants seems like the appropriate place, but it doesn't go into detail about this case).
Regardless, out of scope for this PR.
1e28faa
to
9132e85
Compare
@bors r+ |
📌 Commit 9132e8510337bdee76562e6e594016ac9e530c12 has been approved by |
☔ The latest upstream changes (presumably #51321) made this pull request unmergeable. Please resolve the merge conflicts. |
9132e85
to
adf4ef7
Compare
@varkor: 🔑 Insufficient privileges: Not in reviewers |
@bors r+ |
📌 Commit adf4ef7 has been approved by |
…=estebank Fix various issues with control-flow statements inside anonymous constants Fixes rust-lang#51761. Fixes rust-lang#51963 (and the host of other reported issues there). (Might be easiest to review per commit, as they should be standalone.) r? @estebank
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 |
Looks like a spurious failure to me. |
@bors retry |
☀️ Test successful - status-appveyor, status-travis |
Fixes #51761.
Fixes #51963 (and the host of other reported issues there).
(Might be easiest to review per commit, as they should be standalone.)
r? @estebank