Skip to content
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

Closed
wants to merge 1 commit into from
Closed

Add support for patterns referencing non-trivial statics #15650

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 13, 2014

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.

@ghost
Copy link
Author

ghost commented Jul 13, 2014

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 x that are of variant Some would have to be tested against multiple separate branches in the decision tree: one that is expanded from all the inline patterns that match values of variant Some, in this case Some(false), and one for each pattern that is just a static identifier, like FOO.

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.

@alexcrichton
Copy link
Member

Awesome! 🚀

let const_expr = lookup_const_by_id(cx.tcx, did).unwrap();
vec!(ConstantValue(eval_const_expr(cx.tcx, &*const_expr)))
},
Some(&DefStatic(..)) => unreachable!(),
Copy link
Contributor

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.

@edwardw
Copy link
Contributor

edwardw commented Jul 14, 2014

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!(),
    }
}

@ghost
Copy link
Author

ghost commented Jul 14, 2014

@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 OFF is treated as an identifier that catches all values. Same thing in master: http://is.gd/HeIF6z. We do need a warning of some sort, though.

@ghost
Copy link
Author

ghost commented Jul 14, 2014

I removed the resolve hack, added a warning for mutable statics and changed unreachable!() calls to use span_bug() instead, per @edwardw's suggestion.

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.

@alexcrichton
Copy link
Member

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.

@brson
Copy link
Contributor

brson commented Jul 15, 2014

Closing 5 issues simultaneously is totally ⛵

@ghost
Copy link
Author

ghost commented Jul 18, 2014

@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.
@ghost
Copy link
Author

ghost commented Jul 18, 2014

The test failed because of #15793. I changed it so that it doesn't run into that issue as we've determined it's unrelated.

@pcwalton would you mind again? :-)

@ghost
Copy link
Author

ghost commented Jul 19, 2014

Well, this one'll be fun to track down. This time it failed on the test compile-fail/match-ill-type-2.rs (that is, it successfully compiled it) but only on the optimised 32-bit Windows build. The test isn't really related.

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...

@alexcrichton
Copy link
Member

That's actually a symptom of a spurious failure I've seen before, so let's just retry it and see what happens

bors added a commit that referenced this pull request Jul 19, 2014
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.
@bors bors closed this Jul 19, 2014
@ghost ghost deleted the patterns-statics branch August 18, 2014 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment