gccrs icon indicating copy to clipboard operation
gccrs copied to clipboard

`match` of tuple expressions fails to compile

Open tschwinge opened this issue 4 years ago • 2 comments

I tried this code, similar to existing rust/compile/torture/match1.rs, but using tuple expressions:

enum Foo {
    A,
    B,
}

fn inspect(f: Foo, g: u8) {
    match (f, g) {
        (Foo::A, 1) => {}
        (Foo::B, 2) => {}
        _ => {}
    }
}

I expected to see this happen: compile without errors.

Instead, this happened:

[...]:9:21: error: exprwithoutblock requires comma after match case expression in match arm (if not final case)
    9 |         (Foo::B, 2) => {}
      |                     ^
[...]:7:5: error: failed to parse expr with block in parsing expr statement
    7 |     match (f, g) {
      |     ^
[...]:7:5: error: failed to parse statement or expression without block in block expression
[...]:12:1: error: unrecognised token ‘}’ for start of item
   12 | }
      | ^
[...]:12:1: error: failed to parse item in crate

GCC/Rust commit 9011184f38a04f81ba3194b826bec3f30a11c07b

tschwinge avatar Apr 04 '22 08:04 tschwinge

Whats interesting here is that the parser is parsing:

{}
        (Foo::B, 2)`

As a call expression since I guess {} is an expression.

rust1: note: ARM: [
 outer attributes: none
Patterns: 
 TuplePattern: 
 Foo::A
 1
Guard expr: none] EXPR: [CallExpr: 
 outer attributes: none
 Function expr: BlockExpr:

  outer attributes: none
  inner attributes: none
 statements: none
 final expression: none

 Call params:
  Foo::B
  2]

philberty avatar Apr 20 '22 14:04 philberty

Adding some relevant notes

I wonder can you have a think about this issue: https://github.com/Rust-GCC/gccrs/issues/1081

I added the name-resolution and type-resolution steps here: https://github.com/Rust-GCC/gccrs/pull/1144

I got stumped on the code-generation, but I think you have all the primitives required to do this now. I think it would be worth looking at this compiler explorer gimple dump might give some idea how we can do it:

https://godbolt.org/z/xKG68raPr

Not sure whats going on there but I think the code-gen is switching on the discriminate in the enum but not sure if its doing anything with the integer literal but it might be optimized out not 100% sure.

Originally posted by @philberty in https://github.com/Rust-GCC/gccrs/pull/1244#pullrequestreview-969314245

The discriminant of the enum is used but yeah the literal is being ignored, because basically we are just examining the first element of the tuple. (Actually, currently this will fail the assertions in CompileExpr::visit (MatchExpr&) so it won't compile at all, since we don't know how to handle tuples)

I think at the end of the day match on a tuple should become nested switch statements, one for each element of the tuple. It looks like this is what rustc does as well:

https://godbolt.org/z/h5q5arnMK

I'm looking at the best way to do this. Maybe we rearrange the match at the HIR level just before compiling it into nested matches each on one element of the tuple. e.g. turn

match (f, g) {
        (Foo::A, 1) => { return 5; }
        (Foo::A, 2) => { return 10; }
        (Foo::B, 2) => { return 15; }
        _ => {}
    }

into

match f {
  Foo::A => {
    match g {
      1 => { return 5; }
      2 => { return 10; }
      _ => {}
    }
  }
  Foo::B => {
    match g {
      2 => { return 15; }
      _ => {}
    }
  }
}

dafaust avatar May 18 '22 17:05 dafaust