Disallow overriding val parameters

We disallow overriding of val parameters, which fixes the soundness problem
discovered in #16092.

There is one exception: If a val parameter is overridden by another val
parameter that can be shown to always have the same value (in the sense
established by Paramforwarding.inheritedAccessor). This exception is needed
to make a not-so-uncommon pattern of case class inheritance go through.

Example:

    abstract class A(val x: Int)
    case class B(override val x: Int) extends A(x)
    case class C(override val x: Int) extends A(x)
    case object D extends A(0)

Here, the `override val`s are necessary since case class parameters are always vals,
so they do override the val in class A. It should be noted that the override val generates
a second field, so this not a very efficient representation. A better design would be
to use an abstract field in `A`:

    abstract class A { val x: Int }
    case class B(val x: Int) extends A
    case class C(val x: Int) extends A
    case object D extends A { val a = 0 }

But that causes slightly more work for cases as in D. Which seems to be why the first pattern
is sometimes used. It might be desirable to disallow the second pattern, but that would cause
quite a bit of migration hassle since it requires synchronized changes at several places of
a class hierarchy.

Author: odersky

compiler/src/dotty/tools/dotc/transform/ParamForwarding.scala

@@ -30,7 +30,8 @@ import NameKinds.ParamAccessorName
  *  The aim of this transformation is to avoid redundant parameter accessor fields.
  */
 class ParamForwarding extends MiniPhase with IdentityDenotTransformer:
-  import ast.tpd._
+  import ast.tpd.*
+  import ParamForwarding.inheritedAccessor
 
   private def thisPhase: ParamForwarding = this
 
@@ -39,20 +40,6 @@ class ParamForwarding extends MiniPhase with IdentityDenotTransformer:
   override def description: String = ParamForwarding.description
 
   def transformIfParamAlias(mdef: ValOrDefDef)(using Context): Tree =
-
-    def inheritedAccessor(sym: Symbol)(using Context): Symbol =
-      val candidate = sym.owner.asClass.superClass
-        .info.decl(sym.name).suchThat(_.is(ParamAccessor, butNot = Mutable))
-        .symbol
-      if !candidate.is(Private)  // candidate might be private and accessible if it is in an outer class
-         && candidate.isAccessibleFrom(currentClass.thisType, superAccess = true)
-      then
-        candidate
-      else if candidate.is(SuperParamAlias) then
-        inheritedAccessor(candidate)
-      else
-        NoSymbol
-
     val sym = mdef.symbol.asTerm
     if sym.is(SuperParamAlias) then
       assert(sym.is(ParamAccessor, butNot = Mutable))
@@ -84,3 +71,17 @@ class ParamForwarding extends MiniPhase with IdentityDenotTransformer:
 object ParamForwarding:
   val name: String = "paramForwarding"
   val description: String = "add forwarders for aliases of superclass parameters"
+
+  def inheritedAccessor(sym: Symbol)(using Context): Symbol =
+    val candidate = sym.owner.asClass.superClass
+      .info.decl(sym.name).suchThat(_.is(ParamAccessor, butNot = Mutable))
+      .symbol
+    if !candidate.is(Private)  // candidate might be private and accessible if it is in an outer class
+        && candidate.isAccessibleFrom(currentClass.thisType, superAccess = true)
+    then
+      candidate
+    else if candidate.is(SuperParamAlias) then
+      inheritedAccessor(candidate)
+    else
+      NoSymbol
+end ParamForwarding

compiler/src/dotty/tools/dotc/typer/RefChecks.scala

@@ -264,6 +264,8 @@ object RefChecks {
    *    1.10.  If O is inline (and deferred, otherwise O would be final), M must be inline
    *    1.11.  If O is a Scala-2 macro, M must be a Scala-2 macro.
    *    1.12.  If O is non-experimental, M must be non-experimental.
+   *    1.13   If O is a val parameter, M must be a val parameter that passes on its
+   *           value to O.
    *  2. Check that only abstract classes have deferred members
    *  3. Check that concrete classes do not have deferred definitions
    *     that are not implemented in a subclass.
@@ -446,6 +448,10 @@ object RefChecks {
         overrideError("cannot be used here - classes can only override abstract types")
       else if other.isEffectivelyFinal then // (1.2)
         overrideError(i"cannot override final member ${other.showLocated}")
+      else if other.is(ParamAccessor) &&
+          !(member.is(ParamAccessor) && ParamForwarding.inheritedAccessor(member) == other)
+      then // (1.13)
+        overrideError(i"cannot override val parameter ${other.showLocated}")
       else if (member.is(ExtensionMethod) && !other.is(ExtensionMethod)) // (1.3)
         overrideError("is an extension method, cannot override a normal method")
       else if (other.is(ExtensionMethod) && !member.is(ExtensionMethod)) // (1.3)

tests/init/neg/override5.scala

@@ -25,9 +25,3 @@ trait Base {
 
   val message = "hello, " + name
 }
-
-class Derived(val name: String) extends Base
-
-class Derived2 extends Derived("hello") {
-  override val name: String = "ok"         // error
-}

tests/neg/i11344.scala

@@ -0,0 +1,6 @@
+trait Pet(val name: String, rest: Int):
+   def f(suffix: String) = s"$name$suffix$rest"
+
+class Birdie(override val name: String) extends Pet("huh", 1) // error
+
+

tests/neg/i16092-members-only.scala

@@ -0,0 +1,31 @@
+trait X:
+  type T
+  def process(t: T): Unit
+
+abstract class Z:
+  def x1: X
+  val x: X = x1
+  def t: x.T
+  def process(): Unit = x.process(t)
+
+class Evil extends Z:
+  def x2: X
+  override val x: X = x2
+
+// alarm bells should be ringing by now
+
+// taking it to its conclusion...
+object X1 extends X:
+  override type T = Int
+  override def process(t: T): Unit = println("Int: " + t)
+
+object X2 extends X:
+  override type T = String
+  override def process(t: T): Unit = println("String: " + t)
+
+@main def Test =
+  new Evil{
+    val x1 = X1
+    val x2 = X2
+    val t = 42  // error
+  }.process() // BOOM: basically did x2.process(42)

tests/neg/i16092.scala

@@ -0,0 +1,24 @@
+trait X {
+  type T
+  def process(t: T): Unit
+}
+
+class Z(val x: X, val t: x.T) {
+  def process(): Unit = x.process(t)
+}
+class Evil(x1: X, x2: X, t: x1.T) extends Z(x1, t) {
+  val x: X = x2 // error breaks connection between x and t
+}
+// alarm bells should be ringing by now
+
+// taking it to its conclusion...
+object x1 extends X {
+  override type T = Int
+  override def process(t: T): Unit = println("Int: " + t)
+}
+object x2 extends X {
+  override type T = String
+  override def process(t: T): Unit = println("String: " + t)
+}
+
+@main def Test = new Evil(x1, x2, 42).process() // BOOM: basically did x2.process(42)

tests/neg/i9460.scala

@@ -1,4 +1,4 @@
-trait A(val s: String) { println(s) }
-trait B extends A { override val s = "B" } // requires override val s
+trait A(s: String) { println(s) }
+trait B extends A { val s = "B" }
 class C extends B  // error
 @main def Test = C()

tests/pos-with-compiler/tasty/test-definitions.scala

@@ -163,9 +163,9 @@ object definitions {
       }
     }
 
-    abstract class LambdaType[ParamInfo, This <: LambdaType[ParamInfo, This]](
+    abstract class LambdaType[ParamInfo, This <: LambdaType[ParamInfo, This]]
+    extends Type {
       val companion: LambdaTypeCompanion[ParamInfo, This]
-    ) extends Type {
       private[Type] var _pinfos: List[ParamInfo]
       private[Type] var _restpe: Type
 
@@ -186,16 +186,21 @@ object definitions {
     }
 
     case class MethodType(paramNames: List[String], private[Type] var _pinfos: List[Type], private[Type] var _restpe: Type)
-    extends LambdaType[Type, MethodType](MethodType) {
+    extends LambdaType[Type, MethodType] {
+      override val companion = MethodType
       def isImplicit = (companion `eq` ImplicitMethodType) || (companion `eq` ErasedImplicitMethodType)
       def isErased = (companion `eq` ErasedMethodType) || (companion `eq` ErasedImplicitMethodType)
     }
 
     case class PolyType(paramNames: List[String], private[Type] var _pinfos: List[TypeBounds], private[Type] var _restpe: Type)
-    extends LambdaType[TypeBounds, PolyType](PolyType)
+    extends LambdaType[TypeBounds, PolyType] {
+      override val companion = PolyType
+    }
 
     case class TypeLambda(paramNames: List[String], private[Type] var _pinfos: List[TypeBounds], private[Type] var _restpe: Type)
-    extends LambdaType[TypeBounds, TypeLambda](TypeLambda)
+    extends LambdaType[TypeBounds, TypeLambda] {
+      override val companion = TypeLambda
+    }
 
     object TypeLambda extends LambdaTypeCompanion[TypeBounds, TypeLambda]
     object PolyType   extends LambdaTypeCompanion[TypeBounds, PolyType]

tests/pos/i16092.scala

@@ -0,0 +1,5 @@
+class A(val x: Int)
+class B(override val x: Int) extends A(x)
+
+class C(x: Int) extends A(x)
+case class D(override val x: Int) extends C(x)

tests/pos/i2051.scala

@@ -4,6 +4,3 @@ class B[T](override val x:T) extends A[T](x)
 class C[T](val x:T, val y: Int, val z: Boolean)
 class D[T](override val x:T, y: Int, z: Boolean) extends C[T](x, y, z)
 
-trait X(val x: Int, y: Int, z: Int)
-trait Y(override val x: Int, y: Int, z: Int) extends X
-class Z(override val x: Int, y: Int, z: Int) extends Y(x, y, z) with X(x, y, z)

tests/run/i11344.scala

@@ -0,0 +1,8 @@
+
+class A(a: Int)
+
+class B extends A(1):
+  val a = 2 // ok
+
+@main def Test =
+  assert(B().a == 2)

tests/run/i16092.scala

@@ -16,8 +16,7 @@ class B(override val theValue: Int) extends A(42) {
 
 // Bz contains a field Bz.theValue$$local accessible using the getter
 // Bz.theValue() which overrides A.theValue()
-class Bz extends A(42) {
-  override val theValue: Int = 10
+class Bz(override val theValue: Int = 10) extends A(42) {
   val theValueInBz = theValue
 }