Disallow overriding val parameters (#16096)

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 first pattern, but that would cause quite a bit of
migration hassle since it requires synchronized changes at several
places of a class hierarchy.

Fixes #16092

Author: odersky

community-build/community-projects/akka

@@ -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

community-build/community-projects/scalacheck

@@ -15,8 +15,8 @@ import config.Printers.{checks, noPrinter}
 import Decorators._
 import OverridingPairs.isOverridingPair
 import typer.ErrorReporting._
-import config.Feature.{warnOnMigration, migrateTo3}
-import config.SourceVersion.`3.0`
+import config.Feature.{warnOnMigration, migrateTo3, sourceVersion}
+import config.SourceVersion.{`3.0`, `future`}
 import config.Printers.refcheck
 import reporting._
 import Constants.Constant
@@ -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   Under -source future, if O is a val parameter, M must be a val parameter
+   *           that passes its value on 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.
@@ -514,12 +516,26 @@ object RefChecks {
           overrideError(i"needs to be declared with @targetName(${"\""}${other.targetName}${"\""}) so that external names match")
         else
           overrideError("cannot have a @targetName annotation since external names would be different")
+      else if other.is(ParamAccessor) && !isInheritedAccessor(member, other) then // (1.13)
+        if sourceVersion.isAtLeast(`future`) then
+          overrideError(i"cannot override val parameter ${other.showLocated}")
+        else
+          report.deprecationWarning(
+            i"overriding val parameter ${other.showLocated} is deprecated, will be illegal in a future version",
+            member.srcPos)
       else if !other.isExperimental && member.hasAnnotation(defn.ExperimentalAnnot) then // (1.12)
         overrideError("may not override non-experimental member")
       else if other.hasAnnotation(defn.DeprecatedOverridingAnnot) then
         overrideDeprecation("", member, other, "removed or renamed")
     end checkOverride
 
+    def isInheritedAccessor(mbr: Symbol, other: Symbol): Boolean =
+      mbr.is(ParamAccessor)
+      && {
+        val next = ParamForwarding.inheritedAccessor(mbr)
+        next == other || isInheritedAccessor(next, other)
+      }
+
     OverridingPairsChecker(clazz, self).checkAll(checkOverride)
     printMixinOverrideErrors()
 

community-build/community-projects/specs2

@@ -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
-}

community-build/community-projects/spire

@@ -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
+
+

compiler/src/dotty/tools/dotc/transform/ParamForwarding.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) {
+  override 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)

compiler/src/dotty/tools/dotc/typer/RefChecks.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/init/neg/override13.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/init/neg/override16.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/init/neg/override5.scala

@@ -0,0 +1,18 @@
+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)
+
+// The following is extracted from akka:
+trait LogEvent {
+  def cause: Throwable
+}
+
+/**
+ * For ERROR Logging
+ */
+case class Error(override val cause: Throwable) extends LogEvent
+class Error2(override val cause: Throwable) extends Error(cause)
+class Error3(override val cause: Throwable) extends Error2(cause)
+

tests/neg-custom-args/deprecation/i11344.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/neg-strict/i16092.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/neg/i16092-members-only.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
 }