Skip to content
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

Disallow overriding val parameters #16096

Merged
merged 8 commits into from
Sep 27, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 16 additions & 15 deletions compiler/src/dotty/tools/dotc/transform/ParamForwarding.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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))
Expand Down Expand Up @@ -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
15 changes: 13 additions & 2 deletions compiler/src/dotty/tools/dotc/typer/RefChecks.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 on to O.
Copy link
Contributor

@liufengyun liufengyun Sep 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some word is missing after its.

We may want to add a final flag in the desugaring if sourceVersion.isAtLeast(future).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we add ParamAccessor to the set of EffectivelyFinalFlags. But for the moment we want to treat them separately.

odersky marked this conversation as resolved.
Show resolved Hide resolved
* 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.
Expand Down Expand Up @@ -514,6 +516,15 @@ 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)
&& !(member.is(ParamAccessor) && ParamForwarding.inheritedAccessor(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
Expand Down
13 changes: 0 additions & 13 deletions tests/init/neg/override13.scala

This file was deleted.

23 changes: 0 additions & 23 deletions tests/init/neg/override16.scala

This file was deleted.

6 changes: 0 additions & 6 deletions tests/init/neg/override5.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
6 changes: 6 additions & 0 deletions tests/neg-custom-args/deprecation/i11344.scala
Original file line number Diff line number Diff line change
@@ -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


24 changes: 24 additions & 0 deletions tests/neg-strict/i16092.scala
Original file line number Diff line number Diff line change
@@ -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)
31 changes: 31 additions & 0 deletions tests/neg/i16092-members-only.scala
Original file line number Diff line number Diff line change
@@ -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)
4 changes: 2 additions & 2 deletions tests/neg/i9460.scala
Original file line number Diff line number Diff line change
@@ -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()
15 changes: 10 additions & 5 deletions tests/pos-with-compiler/tasty/test-definitions.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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]
Expand Down
5 changes: 5 additions & 0 deletions tests/pos/i16092.scala
Original file line number Diff line number Diff line change
@@ -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)
3 changes: 0 additions & 3 deletions tests/pos/i2051.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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)
8 changes: 0 additions & 8 deletions tests/run/i11344.scala

This file was deleted.

8 changes: 8 additions & 0 deletions tests/run/i16092.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@

class A(a: Int)

class B extends A(1):
val a = 2 // ok

@main def Test =
assert(B().a == 2)
3 changes: 1 addition & 2 deletions tests/run/paramForwarding.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down