-
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
Add support for patterns referencing non-trivial statics #15650
Conversation
I am not thrilled about this approach. It definitely seems ad-hoc but the the only other solution I could think of would I think be inferior. So the other option I've considered was not to do any of the inlining this PR encompasses and completely exclude references to static items from exhaustiveness analysis, which I think would be really confusing. What would also follow from that is that those patterns would have to be treated specially in the decision tree construction in trans/_match.rs because in: static FOO = Some(true);
match x {
Some(false) => (),
FOO => (),
None => ()
} Values We already kind of do it for slice patterns and I think that's bad. I'd expect that the decision tree that a match expression is compiled to would not differ based on whether the arms are referring to static items or are "inlined" by the user. The hack I've put in in resolve is definitely not going to stay, by the way. @pcwalton @edwardw I would very much appreciate your feedback on this. |
Awesome! 🚀 |
let const_expr = lookup_const_by_id(cx.tcx, did).unwrap(); | ||
vec!(ConstantValue(eval_const_expr(cx.tcx, &*const_expr))) | ||
}, | ||
Some(&DefStatic(..)) => unreachable!(), |
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.
Something like span_bug(..., "static pattern should've been rewritten")
to make the error explicit would be better. So the other similar arms below.
A mutable static seems to pose some difficulties. For example, a slightly modified version of one of your tests gives error: fn main() {
type Foo = (i32, i32);
static ON: Foo = (1, 1);
static mut OFF: Foo = (0, 0);
match (1, 1) {
OFF => unreachable!(),
ON => (), // error: unreachable pattern [E0001]
_ => unreachable!() // error: unreachable pattern [E0001]
}
} And fn main() {
type Foo = (i32, i32);
static ON: Foo = (1, 1);
static mut OFF: Foo = (0, 0);
// error: cannot determine a type for this expression: cannot determine the type of this integer
match (1, 1) {
OFF => unreachable!(),
}
} And finally, the following code compiles but the supposedly unreachable arm is reached: fn main() {
type Foo = (i32, i32);
static mut OFF: Foo = (0, 0);
match (1i32, 1i32) {
OFF => unreachable!(),
}
} |
@edwardw Great find. Although we need a better error message, I think this is intended. If it's a mutable static, it means you intend to mutate it in an unsafe block which means we can't inline them at compile-time. So what happens in all your examples is that |
I removed the resolve hack, added a warning for mutable statics and changed Still not entirely convinced this is a great idea in how we transplant a part of the AST to be used in a different context. But maybe there's a bigger picture here, which is that we should be more aggressive in treating patterns as duals of expressions in the compiler and that this particular rewriting is actually a step in that direction. |
While not overly familiar with this portion of the compiler, the strategy taken here seems reasonable to me as it's much more likely to be foolproof moving into the future. |
Closing 5 issues simultaneously is totally ⛵ |
@pcwalton Thanks for the review. I had to rebase again. r=? |
This is accomplished by rewriting static expressions into equivalent patterns. This way, patterns referencing static variables can both participate in exhaustiveness analysis as well as be compiled down into the appropriate branch of the decision trees that match expressions are codegened to. Fixes #6533. Fixes #13626. Fixes #13731. Fixes #14576. Fixes #15393.
Well, this one'll be fun to track down. This time it failed on the test It'd be nice to be able to SSH into that build agent and poke around with RUST_LOG. My conservative assumption is this patch causes rustc to miscompile itself at the last stage in some very peculiar way... |
That's actually a symptom of a spurious failure I've seen before, so let's just retry it and see what happens |
This is accomplished by rewriting static expressions into equivalent patterns. This way, patterns referencing static variables can both participate in exhaustiveness analysis as well as be compiled down into the appropriate branch of the decision trees that match expressions are codegened to. Fixes #6533. Fixes #13626. Fixes #13731. Fixes #14576. Fixes #15393.
This is accomplished by rewriting static expressions into equivalent patterns.
This way, patterns referencing static variables can both participate
in exhaustiveness analysis as well as be compiled down into the appropriate
branch of the decision trees that match expressions are codegened to.
Fixes #6533.
Fixes #13626.
Fixes #13731.
Fixes #14576.
Fixes #15393.