Skip to content

Missing check for consistency of arguments of parameterized superclasses leads to soundness bug #11018

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
mario-bucev opened this issue Jan 7, 2021 · 8 comments

Comments

@mario-bucev
Copy link
Contributor

mario-bucev commented Jan 7, 2021

Minimized code

@main def test: Unit =
  trait Foo:
    def name: String
  class Bar
  
  trait C[+A](val a: A)
  trait D[+B] extends C[B]
  final class F extends D[Foo] with C[Bar](new Bar)

  def get[X](c: C[X]): X = c match
    case f: F =>
      // Inferred: X >: Foo & Bar but f.a: Bar
      f.a

  val `:(`: String = get[Foo & Bar](new F).name // ClassCastException: Bar cannot be cast to Foo

Output

java.lang.ClassCastException: main$package$Bar$1 cannot be cast to main$package$Foo$1
	at main$package$.test(main.scala:15)
	at test.main(main.scala:1)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at sbt.Run.invokeMain(Run.scala:115)
	at sbt.Run.execute$1(Run.scala:79)
	at sbt.Run.$anonfun$runWithLoader$4(Run.scala:92)
	at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:23)
	at sbt.util.InterfaceUtil$$anon$1.get(InterfaceUtil.scala:10)
	at sbt.TrapExit$App.run(TrapExit.scala:257)
	at java.lang.Thread.run(Thread.java:748)

Expectation

The compiler should reject the code.

I do not think the problem lies within GADT reasoning since we indeed have F <: C[Foo & Bar], but rather in how trait parameters are handled (hence the issue title; I couldn't come up with a better title :p). Since we have A >: Foo & Bar, F should pass an argument of type Foo & Bar to C.

If we were to replace the trait parameter a with a method, we are enforced to override a with the correct type Foo & Bar:

@main def test: Unit =
  trait Foo:
    def name: String
  class Bar

  trait C[+A]:
    def a: A
  trait D[+B] extends C[B]
  final class F extends D[Foo] with C[Bar]:
    override def a: Foo & Bar =
      class FooBar(val name: String) extends Bar with Foo
      FooBar("hello")

  def get[X](c: C[X]): X = c match
    case f: F => f.a

  val `:)`: String = get[Foo & Bar](new F).name // "hello"
@b-studios
Copy link
Contributor

b-studios commented Jan 8, 2021

Thanks for reporting.

It looks like the parent constructor call to the parametrized trait C[Bar](new Bar) is checked, which succeeds. Then since the type parameter is covariant, the intersection is taken -- which is not correct in this case, since the argument does not conform, anymore.

@b-studios
Copy link
Contributor

b-studios commented Jan 8, 2021

@smarter is it possible to recheck the constructor applications of parametrized traits after unification of <: Foo and <: Bar to <: Foo & Bar? That is, checking:

class F extends D[Foo & Bar] with C[Foo & Bar](new Bar)

@smarter
Copy link
Member

smarter commented Jan 8, 2021

First, we can reproduce the problem with a parameterized class instead of a trait, which means we can see if it compiles with Scala 2:

trait Foo {
  def name: String
}
class Bar

class C[+A](val a: A)
trait D[+B] extends C[B]
final class F extends C[Bar](new Bar) with D[Foo]

object Test {
  def main(args: Array[String]): Unit = {
    def get[X](c: C[X]): X = c match {
      case f: F =>
        // Inferred: X >: Foo & Bar but f.a: Bar
        f.a
    }

    val x: String = get[Foo with Bar](new F).name // ClassCastException: Bar cannot be cast to Foo
  }
}

Turns out it doesn't:

i11018.scala:8: error: illegal inheritance;
 class F inherits different type instances of class C:
C[Foo] and C[Bar]
final class F extends C[Bar](new Bar) with D[Foo]
            ^
1 error

So it looks like all we have to do is port this check: https://github.com/scala/scala/blob/9bb659e62a9239c01aec14c171f8598bb1a576fe/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala#L834-L883

@smarter smarter added this to the 3.0.0-RC1 milestone Jan 8, 2021
@smarter smarter changed the title Parametrized trait not correctly enforcing the type of their parameter in presence of covariant type parameter Missing check for consistency of arguments of parameterized superclasses leads to soundness bug Jan 8, 2021
@smarter
Copy link
Member

smarter commented Jan 8, 2021

We already have related code in our own RefChecks phase, but I guess it's not complete enough: https://github.com/lampepfl/dotty/blob/a7fccd41461e749024d6d1a61de01af5cab5d8ea/compiler/src/dotty/tools/dotc/typer/RefChecks.scala#L763-L807

@b-studios
Copy link
Contributor

Thanks @smarter, I can take a look at it.

@b-studios b-studios self-assigned this Jan 8, 2021
@b-studios
Copy link
Contributor

b-studios commented Jan 11, 2021

Attempting to fix this issue, I stumbled upon #3989, which is roughly related -- there we have the following example:

trait A[+X](val x: X)
class B[+X](val y: X) extends A[X](y)
class C extends B(5) with A[String] // error: illegal inheritance
class D extends B(5) with A[Any] // ok

where the member x of class D has type d.x : Int & Any. In this case, the parameter 5 is Int <:< Int & Any.

@b-studios
Copy link
Contributor

I implemented a check that compares the type of the parameter at the base with the type of the parameter as seen from the inheriting subclass. For D these checks are:

// compare B[Int](5).x.type <:< D.x.type
Int <:< Int & Any // OK
// compare B[Any](5).x.type <:< D.x.type
Any <:< Int & Any // ERROR

So my additional check reports an error here, while the test expects it to be ok. @smarter, do you think it is reasonable to reject the example on these grounds?

26 |  class D extends B(5) with A[Any] // ok
   |        ^
   |        illegal parameter: The types of value x do not match.
   |
   |          value x in trait A has type: Any
   |          but class D expects value x to have type: Int

b-studios added a commit to dotty-staging/dotty that referenced this issue Jan 11, 2021
In order to fix a soundness issue with parametrized classes / traits (scala#11018), we port the
validation of base types from Scala 2 to Scala 3.

The original implementation of the check can be found here:

  https://github.com/scala/scala/blob/9bb659e62a9239c01aec14c171f8598bb1a576fe/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala#L841-L882
@anatoliykmetyuk anatoliykmetyuk modified the milestones: 3.0.0-RC1, 3.0.0-RC2 Feb 11, 2021
@smarter
Copy link
Member

smarter commented Mar 15, 2021

Fixed by #11059

@smarter smarter closed this as completed Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants