Skip to content

inconsistency between path-dependent and projection type matching #16728

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
ftucky opened this issue Jan 19, 2023 · 8 comments · Fixed by #17136
Closed

inconsistency between path-dependent and projection type matching #16728

ftucky opened this issue Jan 19, 2023 · 8 comments · Fixed by #17136

Comments

@ftucky
Copy link

ftucky commented Jan 19, 2023

Compiler version

3.2.2

Minimized code

class A:
  class X:
      def outer : A.this.type = A.this

class B extends A
class C extends A

@main def run() : Unit =
  val b0 = new B
  val b1 = b0
  val b2 = new B

  val c0 = new C
  val c1 = c0
  val c2 = new C

  val b0x : A#X = new b0.X

  val pathTypeMatch = b0x match
    case _ : c2.X => "c2.X"
    case _ : c1.X => "c1.x"
    case _ : c0.X => "c0.X"
    case _ : b2.X => "b2.X"
    case _ : b1.X => "b1.X"
    case _ : b0.X => "b0.X"
    case _        => "ELSE"

  println(pathTypeMatch)

  val projectionTypeMatch = b0x match
    case _ : C#X => "C#X"
    case _ : B#X => "B#X"
    case _ : A#X => "A#X"
    case _       => "ELSE"

  println(projectionTypeMatch)

  val failingTypeMatch = b0x match
    case cx : C#X =>
      val c : C = cx.outer
      c

  println(failingTypeMatch)

Output

b1.X
C#X
java.lang.ClassCastException: class B cannot be cast to class C (B and C are in unnamed module of loader java.net.URLClassLoader @55e91e61)

Expectation

Path-dependent type matching behaves as expected. b0x is indeed a b1.X.

Projection type matching is inconsistent. b0x is not a C#X, but a B#X.
Once in the branch, the legitimate type-ascription of (cx:C#X).outer to type C throws the exception.

Apparently, B#X and C#X have a type erasure in A#X. In the byte code, the condition of all 3 branches are strictly identical (isinstance A$X). Thus, branches B#X and A#X are actually unreachable.

The paragraph below suggests the intention to take the prefix into account for the match.
https://scala-lang.org/files/archive/spec/2.13/08-pattern-matching.html#type-patterns

A type pattern T is of one of the following forms: A reference to a class C, p.C, or T#C. This type pattern matches any non-null instance of the given class. Note that the prefix of the class, if it exists, is relevant for determining class instances.

Besides, https://scala-lang.org/files/archive/spec/2.13/03-types.html#type-erasure states:

The erasure of a type projection T#x is |T|#x.

Here, C is concrete, so the erasure of C#X is expected to be C#X, not A#X.

@ftucky ftucky added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Jan 19, 2023
@odersky
Copy link
Contributor

odersky commented Jan 20, 2023

What does Scala-2 do for this?

@odersky odersky added stat:needs info area:pattern-matching and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Jan 20, 2023
@prolativ
Copy link
Contributor

Scala 2 compliant version:

class A {
  class X {
      def outer : A.this.type = A.this
  }
}

class B extends A
class C extends A

object Main {
  def main(args: Array[String]) : Unit = {
    val b0 = new B
    val b1 = b0
    val b2 = new B

    val c0 = new C
    val c1 = c0
    val c2 = new C

    val b0x : A#X = new b0.X

    val pathTypeMatch = b0x match {
      case _ : c2.X => "c2.X"
      case _ : c1.X => "c1.x"
      case _ : c0.X => "c0.X"
      case _ : b2.X => "b2.X"
      case _ : b1.X => "b1.X"
      case _ : b0.X => "b0.X"
      case _        => "ELSE"
    }

    println(pathTypeMatch)

    val projectionTypeMatch = b0x match {
      case _ : C#X => "C#X"
      case _ : B#X => "B#X"
      case _ : A#X => "A#X"
      case _       => "ELSE"
    }

    println(projectionTypeMatch)

    val failingTypeMatch = b0x match {
      case cx : C#X =>
        val c : C = cx.outer
        c
    }

    println(failingTypeMatch)
  }
}

Running this snippet outputs:

b1.X
C#X
Exception in thread "main" java.lang.ClassCastException: B cannot be cast to C
        at Main$.main(PathDep.scala:45)
        at Main.main(PathDep.scala)

@odersky
Copy link
Contributor

odersky commented Jan 20, 2023

@prolativ Thanks for the quick diagnosis!

I also think that the behavior as observed does not match the spec. But if we change the code generation to match the spec, we change the runtime behavior of (possibly a lot of) existing code. Is it worth it? Or should we change the spec to something like the following?

... Note that the prefix of the class, if it is a path, is relevant for determining class instances. On the other hand, a projection prefix P# is not checked.

If we do that, we should probably also issue an unchecked warning.

@ftucky
Copy link
Author

ftucky commented Jan 20, 2023

Type projections have been fundamentally revised in scala3 to make them sound. Is it not the opportunity to align the behavior with the expectations ?
I guess many scala2 codes using type-projections must be significantly revised when ported to scala3. As the change would only impact scala3, would the amount of impacted code be that large ?

Besides, I believe that in many cases, the change of runtime will actually fix a former mishap behavior :-).

@Kordyjan
Copy link
Contributor

@odersky We can always run a full community build with a new code generation to see if there were any projects depending on the old behavior. I really doubt that as it was rather useless and the behavior from the spec is much more intuitive.

@ftucky
Copy link
Author

ftucky commented Jan 20, 2023

There must be a way to qualify the situation where the discrepancy between the spec and the (old) behavior appears.

Something like "a pattern matching on a projection type P#C where the outer class Q of P#C does not conform to P."

  • This is the condition where the warning should be issued
  • This would detect the relevant places in the community build sources at compile time.
  • If the intended behavior was the old one, it is always possible to restate the match as Q#C. The warning could even suggest so. Once restated, the warning disappears.

The condition looks impossible to verify if P is a type parameter, a la scala2.
The update to the intuitive behavior would appear as a side-band benefit of scala3 type-projection cleanup.

odersky added a commit to dotty-staging/dotty that referenced this issue Jan 24, 2023
A type pattern of the form A#B should be checked if the prefix A
is not already the owner of B or a superclass.

Fixes scala#16728
@ftucky
Copy link
Author

ftucky commented Jan 24, 2023

Great ! Thank you.

@ftucky
Copy link
Author

ftucky commented Mar 21, 2023

Hello ! Could a PR be opened to integrate fix above ?
Thank you.

Dedelweiss pushed a commit to Dedelweiss/dotty that referenced this issue Apr 17, 2023
A type pattern of the form A#B should be checked if the prefix A
is not already the owner of B or a superclass.

Fixes scala#16728
@Kordyjan Kordyjan added this to the 3.3.1 milestone Aug 1, 2023
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.

4 participants