Disallow overriding val parameters (#16096)

odersky · web-flow · commit 8d723a9dd5cf · 2022-09-27T14:58:05.000+02:00
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
diff --git a/community-build/community-projects/akka b/community-build/community-projects/akka
@@ -1 +1 @@
-Subproject commit ed97fe5233cbda2da02abad50d48c310077b313c
+Subproject commit 7f5115ebc9cde408433040f11834f5218b4a3357
diff --git a/community-build/community-projects/scalacheck b/community-build/community-projects/scalacheck
@@ -1 +1 @@
-Subproject commit 0ac8005753ab98b6494fd631502201b97a103638
+Subproject commit fbfaabd7b628e9b0d8f78ed8a91a0672cf56ba15
diff --git a/community-build/community-projects/specs2 b/community-build/community-projects/specs2
@@ -1 +1 @@
-Subproject commit e1ae96e7a55fed2268f9ccd391687a5ac96ee4df
+Subproject commit 2bfe446a4e9122b1122a7e13a3d100b3749b8630
diff --git a/community-build/community-projects/spire b/community-build/community-projects/spire
@@ -1 +1 @@
-Subproject commit 6869620975fa84dd1ef78c2711d6a4f8197060ae
+Subproject commit 7f630c0209e327bdc782ade2210d8e4b916fddcc
diff --git a/compiler/src/dotty/tools/dotc/transform/ParamForwarding.scala b/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
diff --git a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala
@@ -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()
 
diff --git a/tests/init/neg/override13.scala b/tests/init/neg/override13.scala
diff --git a/tests/init/neg/override16.scala b/tests/init/neg/override16.scala
diff --git a/tests/init/neg/override5.scala b/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
-}
diff --git a/tests/neg-custom-args/deprecation/i11344.scala b/tests/neg-custom-args/deprecation/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
+
+
diff --git a/tests/neg-strict/i16092.scala b/tests/neg-strict/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) {
+  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)
diff --git a/tests/neg/i16092-members-only.scala b/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)
diff --git a/tests/neg/i9460.scala b/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()
diff --git a/tests/pos-with-compiler/tasty/test-definitions.scala b/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]
diff --git a/tests/pos/i16092.scala b/tests/pos/i16092.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)
+
diff --git a/tests/pos/i2051.scala b/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)
diff --git a/tests/run/i11344.scala b/tests/run/i11344.scala
diff --git a/tests/run/i16092.scala b/tests/run/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)
diff --git a/tests/run/paramForwarding.scala b/tests/run/paramForwarding.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
 }
 

Original file line number	Diff line number	Diff line change
`@@ -25,9 +25,3 @@ trait Base {`
`25`	`25`
`26`	`26`	`val message = "hello, " + name`
`27`	`27`	`}`
`28`		`-`
`29`		`-class Derived(val name: String) extends Base`
`30`		`-`
`31`		`-class Derived2 extends Derived("hello") {`
`32`		`- override val name: String = "ok" // error`
`33`		`-}`