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

Associated-const ranges in patterns are miscompiled in beta #57960

Closed
bytwise opened this issue Jan 28, 2019 · 6 comments · Fixed by #58161
Closed

Associated-const ranges in patterns are miscompiled in beta #57960

bytwise opened this issue Jan 28, 2019 · 6 comments · Fixed by #58161
Assignees
Labels
A-NLL Area: Non Lexical Lifetimes (NLL) NLL-complete Working towards the "valid code works" goal P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@bytwise
Copy link
Contributor

bytwise commented Jan 28, 2019

This code demonstrates the problem (playpen):

#![allow(dead_code)]

trait Range {
    const FIRST: u8;
    const LAST: u8;
}

struct OneDigit;
impl Range for OneDigit {
    const FIRST: u8 = 0;
    const LAST: u8 = 9;
}

struct TwoDigits;
impl Range for TwoDigits {
    const FIRST: u8 = 10;
    const LAST: u8 = 99;
}

struct ThreeDigits;
impl Range for ThreeDigits {
    const FIRST: u8 = 100;
    const LAST: u8 = 255;
}

fn digits(x: u8) -> u32 {
    match x {
        OneDigit::FIRST...OneDigit::LAST => 1,
        TwoDigits::FIRST...TwoDigits::LAST => 2,
        ThreeDigits::FIRST...ThreeDigits::LAST => 3,
        _ => unreachable!(),
    }
}

fn main() {
    println!("{}", digits(100));
}

With stable-1.32.0 it prints 3. With beta-1.33.0 it wrongly prints 1.

Beta also produces these wrong warnings:

warning: unreachable pattern
  --> src/main.rs:29:9
   |
29 |         TwoDigits::FIRST...TwoDigits::LAST => 2,
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: #[warn(unreachable_patterns)] on by default

warning: unreachable pattern
  --> src/main.rs:30:9
   |
30 |         ThreeDigits::FIRST...ThreeDigits::LAST => 3,
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

It seems that Rustc decides, that the first arm covers the whole range of a
u8 and optimizes out the other arms before they even reach MIR.

The code is very similar to this testcase.
But the testcase does not test that the other arms can be reached.

I was able to bisect the regression to #55937.

(This issue seems similar to #57894, but the reduced testcase there does not
use any associated consts)

@jonas-schievink
Copy link
Contributor

jonas-schievink commented Jan 28, 2019

Seems like a duplicate of #57894 (that issue applies to range matches in general, whether you're using an assoc. const or a literal doesn't matter) maybe not

Not the same as #57894

@jonas-schievink jonas-schievink added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Jan 28, 2019
@pnkfelix
Copy link
Member

triage: P-high, A-NLL, NLL-complete, assigning to self.

@pnkfelix pnkfelix added P-high High priority A-NLL Area: Non Lexical Lifetimes (NLL) NLL-complete Working towards the "valid code works" goal labels Jan 31, 2019
@pnkfelix pnkfelix self-assigned this Jan 31, 2019
@pnkfelix
Copy link
Member

cc @davidtwco

@pnkfelix
Copy link
Member

pnkfelix commented Feb 1, 2019

I have confirmed that the bug appears to be specifically injected by commit 4be7214

(I've skimmed over that commit several times and still can't see the problem, but I'm going to try again now. Maybe the actual issue is that we are miscompiling PatternKind::AscribeUserType downstream, but that pattern is not sufficiently exercised elsewhere in the compiler today...)

@pnkfelix
Copy link
Member

pnkfelix commented Feb 1, 2019

Well this is making my spidey-sense tingle:

pub fn add_cases_to_switch<'pat>(&mut self,

PatternKind::AscribeUserType { .. } |
PatternKind::Leaf { .. } |
PatternKind::Deref { .. } => {
// don't know how to add these patterns to a switch
false
}

but even when that returns false, it seems like that would only imply that we fall through to presumably more general purpose code here:

// perform the test, branching to one of N blocks. For each of

(still looking. Going to add a bit of instrumentation to these parts of the compiler.)

@davidtwco
Copy link
Member

@pnkfelix I don't mean to step on any toes, but I looked into this a little as an earlier PR of mine introduced it. I've got a fix up at #58161 but if that's not the approach we want to take then I can leave this to you.

bors added a commit that referenced this issue Feb 8, 2019
Lower constant patterns with ascribed types.

Fixes #57960.

This PR fixes a bug introduced by #55937 which started checking user
type annotations for associated type patterns. Where lowering a
associated constant expression would previously return a
`PatternKind::Constant`, it now returns a `PatternKind::AscribeUserType`
with a `PatternKind::Constant` inside, this PR unwraps that to
access the constant pattern inside and behaves as before.

r? @pnkfelix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NLL Area: Non Lexical Lifetimes (NLL) NLL-complete Working towards the "valid code works" goal P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants