Skip to content

Invalid unreachable case warning on Java-defined class with unbounded type parameter #15717

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

Closed
Arthurm1 opened this issue Jul 21, 2022 · 5 comments · Fixed by #15769
Closed

Comments

@Arthurm1
Copy link

Arthurm1 commented Jul 21, 2022

Compiler version

3.1.3

Minimized code

object TestMatch extends App {
  def getValues: Object = {
    val x = new java.util.Vector[Double]()
    x.add(1.0)
    x
  }
  val x = getValues
  x match {
    case l: java.util.Vector[_] =>
      l.get(0) match {
        case d: Double => println(s"got first $d") // unreachable case warning
        case _ => println("shrug")
      }
  }
  x match {
    case l: java.util.Vector[_] =>
      l.get(0) match {
        case d: Double => println(s"got second $d") // no warning
      }
  }
}

Output

unreachable case warning on line indicated with comment

Expectation

Running the app shows that the case is used so I'd expect no warning.

The second match statement doesn't have a warning for case d: Double so why does the first?

I get the correct warnings if getValues is not returned as Object.

I don't think this is the same as other open unreachable case issues. It doesn't involve unions or nulls

@Arthurm1 Arthurm1 added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Jul 21, 2022
@szymon-rd szymon-rd added area:pattern-matching and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Jul 21, 2022
@dwijnand dwijnand self-assigned this Jul 25, 2022
@SethTisue
Copy link
Member

SethTisue commented Jul 27, 2022

Nice catch! Thanks for the report.

Also reproduces on latest nightly.

Presumably it's crucial that java.util.Vector is Java-defined and thus there's confusion about whether the relevant top type is Any or AnyRef.

@SethTisue SethTisue changed the title Invalid unreachable case warning Invalid unreachable case warning on Java-defined class with type parameter Jul 27, 2022
@SethTisue
Copy link
Member

SethTisue commented Jul 27, 2022

The nested pattern match isn't needed; the following is a sufficient reproducer:

def foo(xs: java.util.Vector[_]): Unit =
  xs.get(0) match {
    case d: Double =>
    case _ =>
  }

(A second case is needed in order for redundancy checking to kick in.)

@SethTisue
Copy link
Member

SethTisue commented Jul 27, 2022

Note that you get the same unreachable warning if you change the type of xs to java.util.Vector[AnyRef]. But in Scala 2 if you make that change, the compiler rejects the code outright ("pattern type is incompatible with expected type").

(Dale believes this was an intentional design change in Scala 3, but we're not sure if there's a ticket or discussion of record on that. But the long comment at the top of PatternTypeConstrainer.scala is useful background.)

@SethTisue
Copy link
Member

SethTisue commented Jul 27, 2022

Maybe it's “simply” that the redundancy checker “just” needs to be made to handle FromJavaObject properly, here? (See the long comment on FromJavaObjectSymbol in Definitions.scala for a crash course on FromJavaObject.)

We've verified that an upper bound of FromJavaObject is, as hoped/expected, reaching the redundancy checker as input. (Note that FromJavaObject ordinarily prints as Object, which is frustrating when you care about the difference; isFromJavaObject can be used to distinguish.)

@SethTisue SethTisue changed the title Invalid unreachable case warning on Java-defined class with type parameter Invalid unreachable case warning on Java-defined class with unbounded type parameter Jul 27, 2022
@dwijnand dwijnand linked a pull request Jul 27, 2022 that will close this issue
@SethTisue
Copy link
Member

SethTisue commented Jul 27, 2022

Indeed, it was actually that simple. The right FromJavaObject logic already existed in subtype checking (in firstTry in TypeComparer), and we added the same logic to provablyDisjoint. The PR (#15769) includes both the test case from this report, and an inverted version of it where it's the pattern type rather than the scrutinee type that's FromJavaObject.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants