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

Wrong unreachable and exhaustiveness warnings for large tuples #19084

Closed
odersky opened this issue Nov 26, 2023 · 7 comments · Fixed by #19212
Closed

Wrong unreachable and exhaustiveness warnings for large tuples #19084

odersky opened this issue Nov 26, 2023 · 7 comments · Fixed by #19212
Assignees
Labels
area:pattern-matching Spree Suitable for a future Spree

Comments

@odersky
Copy link
Contributor

odersky commented Nov 26, 2023

Compiler version

3.4.0 RC1

Minimized example

@main def Test =
  val y: (
    Int, Int, Int, Int, Int, Int, Int, Int, Int, Int,
    "Bob", Int, 33, Int,
    Int, Int, Int, Int, Int, Int, Int, Int, Int, Int)
     =
      (0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
      "Bob", 0, 33, 0,
      0, 0, 0, 0, 0, 0, 0, 0, 0, 0)

  y match
    case b @ (x0, x1, x2, x3, x4, x5, x6, x7, x8, x9,
          "Bob", y1, 33, y2,
          z0, z1, z2, z3, z4, z5, z6, z7, z8, z9)
     => // !!! spurious unreachable case warning
      println(y)
    case _ => assert(false)

Output

-- [E030] Match case Unreachable Warning: ../new/test.scala:12:9 ---------------
12 |    case b @ (x0, x1, x2, x3, x4, x5, x6, x7, x8, x9,
   |         ^
   |         Unreachable case
13 |          "Bob", y1, 33, y2,
14 |          z0, z1, z2, z3, z4, z5, z6, z7, z8, z9)

Expectation

If I run the program it does get into the case and print the correct value.

Variant: If I drop the last case case _ => assert(false), I get this instead:

-- [E029] Pattern Match Exhaustivity Warning: ../new/test.scala:11:2 -----------
11 |  y match
   |  ^
   |match may not be exhaustive.
   |
   |It would fail on pattern case: _: *:[Int,*:[Int,*:[Int,*:[Int,*:[Int,*:[Int,*:[Int,*:[Int,*:[Int,*:[Int,*:[("Bob" : String),*:[Int,*:[(33 : Int),*:[Int,*:[Int,*:[Int,*:[Int,*:[Int,*:[Int,*:[Int,*:[Int,*:[Int,*:[Int,*:[Int,Tuple$package.EmptyTuple]]]]]]]]]]]]]]]]]]]]]]]]
   |
   | longer explanation available when compiling with `-explain`
@odersky odersky added the stat:needs triage Every issue needs to have an "area" and "itype" label label Nov 26, 2023
@odersky
Copy link
Contributor Author

odersky commented Nov 26, 2023

This will be a huge problem for named tuples. Previously very few people would write a large tuple pattern like this. But with named tuple patterns, all it takes is:

case (name = "Bob", age = a) 

and the wrong unreachable warning will also pop up.

@odersky
Copy link
Contributor Author

odersky commented Nov 26, 2023

Another variant: There's no need to actually match on a tuple element. The following will also issue the wrong warning even though the tuple consists only of pattern variables.

  y match
    case (x0, x1, x2, x3, x4, x5, x6, x7, x8, x9,
          y0, y1, y2, y3,
          z0, z1, z2, z3, z4, z5, z6, z7, z8, z9)

@odersky odersky changed the title Wrong reachable and exhaustiveness warnings for large tuples Wrong unreachable and exhaustiveness warnings for large tuples Nov 26, 2023
@odersky odersky added area:pattern-matching and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Nov 26, 2023
@dwijnand
Copy link
Member

dwijnand commented Nov 27, 2023

When I (or anyone else) gets to this, then I think also #16186, #16657, and #14588 are either straight duplicates or closely related.

odersky added a commit that referenced this issue Nov 27, 2023
Extracted from the named tuples PR, where it allowed the correct typing
of
```scala
val bob = (
  x0 = 0, x1 = 0, x2 = 0, x3 = 0, x4 = 0, x5 = 0, x6 = 0, x7 = 0, x8 = 0, x9 = 0,
  name = "Bob", y1 = 0, age = 33, y2 = 0,
  z0 = 0, z1 = 0, z2 = 0, z3 = 0, z4 = 0, z5 = 0, z6 = 0, z7 = 0, z8 = 0, z9 = 0)

  bob match
    case p @ (name = "Bob", age = a) =>
      p,age
```
The problem was that `p` was typed before as `<some large tuple type> &
TupleXXL` and that prevented the correct analysis of tuple elements.

I am submitting this as a separate PR since it might also be relevant
for #19084.
@bishabosha
Copy link
Member

is this fixed by #19085 ?

@odersky
Copy link
Contributor Author

odersky commented Nov 27, 2023

No, it's independent.

@mbovel mbovel added the Spree Suitable for a future Spree label Nov 29, 2023
@scala-center-bot
Copy link

scala-center-bot commented Nov 29, 2023

This issue was picked for the Issue Spree No. 40 of December 5th, 2023 which takes place in 6 days. @dwijnand, @natsukagami, @iusildra will be working on it. If you have any insight into the issue or guidance on how to fix it, please leave it here.

@dwijnand
Copy link
Member

dwijnand commented Dec 5, 2023

Found some of my notes in #14588 (comment)

We were able to find an initial, quickfix, for reachability, by special casing TupleXXL in isSubType:

    else tp2 match
      case tp2: TypeRef if tp2.symbol == defn.TupleXXLClass && tp1.tupleElementTypes.isDefined => true // arity?
      case _ => tp1 <:< tp2

Necessary because TypeComparer doesn't consider 23 ints to be a subtype of TupleXXL (which the pattern is typed as).

Unfortunately that doesn't help exhaustivity. In exhaustivity we try to calculate the Space of values that remains once all case patterns are considered. Using the test case in #16657, the pattern uses TupleXXL.unapplySeq, and is type cast to TupleXXL, which means that statically we're using a variadic extractor. We've suppressed the precise type of the pattern (with 23 bindings) which would precisely match the arity of the scrutinee (23 ints), so the SpaceEngine is doomed on exhaustivity. So we can either special case more, or try to fix this upstream, so that the abstraction is maintained and doesn't leak here.

@dwijnand dwijnand linked a pull request Dec 6, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:pattern-matching Spree Suitable for a future Spree
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants