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

match expression falling through? #26251

Closed
jrvidal opened this issue Jun 12, 2015 · 11 comments
Closed

match expression falling through? #26251

jrvidal opened this issue Jun 12, 2015 · 11 comments
Assignees
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jrvidal
Copy link
Contributor

jrvidal commented Jun 12, 2015

I tried this code:

fn main() {
    let x = 'a';

    match x {
            'a'...'b' if false => {
                println!("one");
            },

            'a' => {
                println!("two");
            },

            'a'...'b' => {
                println!("three");
            },

            _ => panic!("what?")
    }
}

I expected to get two but I get three. Is this the expected semantics?

Meta

rustc --version --verbose:

rustc 1.1.0-beta (cd7d89a 2015-05-16) (built 2015-05-16)
binary: rustc
commit-hash: cd7d89a
commit-date: 2015-05-16
build-date: 2015-05-16
host: x86_64-unknown-linux-gnu
release: 1.1.0-beta

@shepmaster
Copy link
Member

If you increase the first range to 'a'...'c', then the second match arm is triggered. If you comment out either match arm with a range, then the expected match arm is triggered. Pure speculation, I wonder if there is an optimization that is reordering the match arms with the same check but different guard clauses.

With modified range

fn main() {
    let x = 'a';

    match x {
        'a'...'c' if false => {
            println!("one");
        },

        'a' => {
            println!("two");
        },

        'a'...'b' => {
            println!("three");
        },

        _ => panic!("what?")
    }
}

With commented-out following match arm

fn main() {
    let x = 'a';

    match x {
        'a'...'b' if false => {
            println!("one");
        },

        'a' => {
            println!("two");
        },

/*
        'a'...'b' => {
            println!("three");
        },
*/

        _ => panic!("what?")
    }
}

@Stebalien
Copy link
Contributor

#18060 is similar but doesn't have guards. IMO, these should both be marked P-high, I-wrong.

@arielb1 arielb1 added I-wrong P-high High priority labels Jun 12, 2015
@alexcrichton
Copy link
Member

triage: I-nominated

@rust-highfive rust-highfive added I-nominated and removed P-high High priority labels Jun 12, 2015
@alexcrichton alexcrichton added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 12, 2015
@nikomatsakis
Copy link
Contributor

cc @jakub-

@nikomatsakis
Copy link
Contributor

I agree this is totally bogus... seems like the chance of someone relying on this behavior is... low but not non-trivial?

@nikomatsakis
Copy link
Contributor

triage: P-high

@rust-highfive rust-highfive added P-high High priority and removed I-nominated labels Jun 17, 2015
@Stebalien
Copy link
Contributor

jrvidal ran across this in the wild so it can't be that low.

@jrvidal
Copy link
Contributor Author

jrvidal commented Jun 17, 2015

FWIW, here is a simplified version of my use case, where I try to parse a number literal:

loop {
    match c {
        // ...
        '0'...'9' | 'a'...'f' if valid_digit(c, radix) => {/* parse digit */},
        'e' | 'f' | 's' | 'd' | 'l' if is_decimal_literal() => {/* parse exponential marker */},
        '0'...'9' | 'a'...'f' => {/* return error, bad digit */},
        _ => break
    }
}

@ghost
Copy link

ghost commented Jun 20, 2015

The root problem is the same as #18060.

I've gotta say that I'm convinced the trans bits for match expressions are desperate for a rewrite that I've always hoped to take on and that I've heard @eddyb beg for as well. Sadly, time is extremely scarce for me right now so I can't promise much (oh, the bus factors…).

This, however, is extremely worrisome in that it can lead to really nasty bugs in user code.

eefriedman added a commit to eefriedman/rust that referenced this issue Jul 15, 2015
The old code was not well structured, difficult to understand,
and buggy.

The new implementation is completely naive, so there may be a slight
runtime performance loss. That said, adding optimizations on top of
a clear and correct implementation seems easier than trying to
fix the old mess.

Fixes issue rust-lang#19064.
Fixes issue rust-lang#26989.
Fixes issue rust-lang#26251.
Fixes issue rust-lang#18060.
Fixes issue rust-lang#24875.
Fixes issue rust-lang#23311.
Fixes issue rust-lang#20046.
@nikomatsakis nikomatsakis self-assigned this Oct 1, 2015
@nrc
Copy link
Member

nrc commented Nov 19, 2015

triage: P-medium

also mir, apparently

@rust-highfive rust-highfive added P-medium Medium priority and removed P-high High priority labels Nov 19, 2015
@nrc nrc added A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html and removed P-medium Medium priority labels Nov 19, 2015
@jrvidal
Copy link
Contributor Author

jrvidal commented Aug 12, 2016

I just compiled with the latest nightly and it seems to be working!

rustc 1.12.0-nightly (0ef24eed2 2016-08-10)

@eddyb eddyb added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Aug 12, 2016
bors added a commit that referenced this issue Aug 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants