Warn for code that depend on subtle semantics of class parameters in constructors

[liufengyun]

Minimized example

class B(val y: Int):
  println(y)                   // A warning is due here
  foo()
  def foo() = println(y)

class C(override val y: Int) extends B(10)

@main
def Test = new C(20)

Output

10
20

Expectation

The code above depends on subtle semantics of class parameters in constructors, for which the programmer should receive a warning.

As discussed in #15723 (comment), one possibility to issue the warning is:

Rule A: Issue a warning for the usage of an unqualified class parameter x in the constructor (including usage in super constructor calls) if this.x is overridden in a subclass.

This rule, however, is too coarse-grained, as programmers will get an annoying warning for the following code:

class B(val y: Int):
  println(y)                   // should not issue a warning here
  foo()
  def foo() = println(y)

class C(override val y: Int) extends B(y)

Here is a better rule to avoid the spurious warnings in the above:

Rule B: Issue a warning for the usage of an unqualified class parameter x in the constructor (including usage in super constructor calls) if its semantics deviate from that of this.x.

The plan is to implement the rule in the initialization checker. Implementation of Rule B would require extending the abstract domain with a symbolic value in the spirit of symbolic execution.

[som-snytt]

There is a Scala 2 lint for the inverse:

➜  ~ scala -Xlint:private-shadow
Welcome to Scala 2.13.8 (OpenJDK 64-Bit Server VM, Java 18.0.1.1).
Type in expressions for evaluation. Or try :help.

scala> class C(var c: Int) ; class D(c: Int) extends C(c) { def f = c }
                                                                    ^
       warning: private[this] value c in class D shadows mutable c inherited from class C.  Changes to c will not be visible within class D - you may want to give them distinct names.
class C
class D

scala> val d = new D(42); d.f; d.c = 27; d.f
val d: D = D@286855ea
// mutated d.c
val res0: Int = 42

I expected it to print both d.f, but suffice to say it returns 42 both times.

[liufengyun]

Thanks @som-snytt . It seems -Xlint:private-shadow is an orthogonal feature. The same feature can be implemented in Dotty and co-exist with the current proposal.

The current proposal only concerns the usage of class parameters in the constructor (including in super constructor calls), it does not check the usage in method bodies.

[olhotak]

I agree with implementing Rule B rather than Rule A, but I wonder if it's possible to implement Rule B more simply, without further complicating the abstract domain, which is already quite tricky, and we want it to be understandable to users, not only compiler implementors.

Are there more complicated cases than just class C(override val y: Int) extends B(y)? If not, could we just do a simple for cases where an override val is passed for a parameter of the same name?

If yes, if there are more complicated cases, could we have a separate domain for this check, separate from the existing initialization checking domain? We would have to trade off how much duplicated logic this would require against complicating the initialization domain.

[liufengyun]

If yes, if there are more complicated cases, could we have a separate domain for this check, separate from the existing initialization checking domain? We would have to trade off how much duplicated logic this would require against complicating the initialization domain.

That is a very good point. I agree that we should avoid complicating the abstract domain of the initialization checker.

I was playing with the following rule:

Rule C: All class parameters are final.

The rationale is that if we just pass a class parameter y unchanged to a base class, there is no need for overriding a class parameter y.

With the following changes:

diff --git a/compiler/src/dotty/tools/dotc/ast/Desugar.scala b/compiler/src/dotty/tools/dotc/ast/Desugar.scala
index 10d4fed7f0..b8795be96b 100644
--- a/compiler/src/dotty/tools/dotc/ast/Desugar.scala
+++ b/compiler/src/dotty/tools/dotc/ast/Desugar.scala
@@ -812,8 +812,8 @@ object desugar {
         val originalVparamsIt = originalVparamss.iterator.flatten
         derivedVparamss match {
           case first :: rest =>
-            first.map(_.withMods(originalVparamsIt.next().mods | caseAccessor)) ++
-            rest.flatten.map(_.withMods(originalVparamsIt.next().mods))
+            first.map(_.withMods(originalVparamsIt.next().mods | caseAccessor | Final)) ++
+            rest.flatten.map(_.withMods(originalVparamsIt.next().mods | Final))
           case _ =>
             Nil
         }

We get a few failed tests:

    tests/neg/i9460.scala failed
    tests/neg/doubleDefinition.scala failed
    tests/run/paramForwarding.scala failed
    tests/run/t3651.scala failed
    tests/run/tcpoly_parseridioms.scala failed
    tests/run/i15723.scala failed
    tests/run/i11344.scala failed
    tests/pos/i2051.scala failed
    tests/pos/i618.scala failed
    tests/init/neg/override5.scala failed
    tests/init/neg/override16.scala failed
    tests/explicit-nulls/pos/flow-inline.scala failed

Examining the tests, only tests/run/tcpoly_parseridioms.scala seems to be a real blocker:

  sealed class ParseResult[+t](val next: Input)
  case class Success[+t](override val next: Input, result: t) extends ParseResult[t](next)
  case class Failure(override val next: Input, msg: String) extends ParseResult[Nothing](next)

The reason is that for case classes, the compiler automatically makes the parameters public, and users cannot change that behavior.

The end users have to refactor the code above:

  abstract sealed class ParseResult[+t]:
    val next: Input

  case class Success[+t](next: Input, result: t) extends ParseResult[t]
  case class Failure(next: Input, msg: String) extends ParseResult[Nothing]

The refactoring is arguably a little better.

So the question is whether Rule C is a good rule. If it is, we will probably need a SIP to change the language and suggest a smooth migration path.

[liufengyun]

Found more instances of overridding class parameters in #15815 .

The following two are in Scaladoc:

enum TemplateName(val name: String):
  case YamlDefined(override val name: String) extends TemplateName(name)
  case SidebarDefined(override val name: String) extends TemplateName(name)
  case FilenameDefined(override val name: String) extends TemplateName(name)

enum Resource(val path: String):
  case Text(override val path: String, content: String) extends Resource(path)
  case Classpath(override val path: String, name: String) extends Resource(path)
  case File(override val path: String, file: Path) extends Resource(path)

The following one comes from munit:

class FailException(
    val message: String,
    val cause: Throwable,
    val isStackTracesEnabled: Boolean,
    val location: Location
) extends AssertionError(message, cause)

class FailSuiteException(
    override val message: String,
    override val location: Location
) extends FailException(message, location)

Fastparse:

community-build/community-projects/fastparse/cssparse/src/cssparse/Ast.scala:37:47 
[error] 37 |  sealed case class BracketsBlock(override val values: Seq[ComponentValue]) extends Block("(", ")", values)
[error]    |                                               ^
[error]    |error overriding value values in class Block of type Seq[cssparse.Ast.ComponentValue];
[error]    |  value values of type Seq[cssparse.Ast.ComponentValue] cannot override final member value values in class Block
[error] -- [E164] Declaration Error: /__w/dotty/dotty/community-build/community-projects/fastparse/cssparse/src/cssparse/Ast.scala:38:52 
[error] 38 |  sealed case class CurlyBracketsBlock(override val values: Seq[ComponentValue]) extends Block("{", "}", values)
[error]    |                                                    ^
[error]    |error overriding value values in class Block of type Seq[cssparse.Ast.ComponentValue];
[error]    |  value values of type Seq[cssparse.Ast.ComponentValue] cannot override final member value values in class Block
[error] -- [E164] Declaration Error: /__w/dotty/dotty/community-build/community-projects/fastparse/cssparse/src/cssparse/Ast.scala:39:53 
[error] 39 |  sealed case class SquareBracketsBlock(override val values: Seq[ComponentValue]) extends Block("[", "]", values)
[error]    |                                                     ^
[error]    |error overriding value values in class Block of type Seq[cssparse.Ast.ComponentValue];
[error]    |  value values of type Seq[cssparse.Ast.ComponentValue] cannot override final member value values in class Block

Effpi:

private class ActorPipeImpl[A](
  override val mbox: Mailbox[A],
  override val ref: ActorRef[A]) extends ActorPipe[A](mbox, ref)

Akka-actor (there are many such code):

final class BackoffSupervisor @deprecated("Use `BackoffSupervisor.props` method instead", since = "2.5.22")(
    override val childProps: Props,
    override val childName: String,
    minBackoff: FiniteDuration,
    maxBackoff: FiniteDuration,
    override val reset: BackoffReset,
    randomFactor: Double,
    strategy: SupervisorStrategy,
    val replyWhileStopped: Option[Any],
    val finalStopMessage: Option[Any => Boolean])
    extends BackoffOnStopSupervisor(
      childProps,
      childName,
      minBackoff,
      maxBackoff,
      reset,
      randomFactor,
      strategy,
      replyWhileStopped.map(msg => ReplyWith(msg)).getOrElse(ForwardDeathLetters),
      finalStopMessage)

endpoints4s:

  class JsonSchema[A](
      val ujsonSchema: ujsonSchemas.JsonSchema[A],
      val docs: DocumentedJsonSchema
  )

  class Record[A](
      override val ujsonSchema: ujsonSchemas.Record[A],
      override val docs: DocumentedRecord
  ) extends JsonSchema[A](ujsonSchema, docs)

  class Tagged[A](
      override val ujsonSchema: ujsonSchemas.Tagged[A],
      override val docs: DocumentedCoProd
  ) extends JsonSchema[A](ujsonSchema, docs)

  class Enum[A](
      override val ujsonSchema: ujsonSchemas.Enum[A],
      override val docs: DocumentedEnum
  ) extends JsonSchema[A](ujsonSchema, docs)

specs2:

community-build/community-projects/specs2/common/shared/src/main/scala/org/specs2/execute/Result.scala:452:17 
[error] 452 |    override val expectationsNb = n
[error]     |                 ^
[error]     |error overriding value expectationsNb in class Result of type Int;

stdlib:

community-build/community-projects/stdLib213/src/library/scala/collection/convert/JavaCollectionWrappers.scala:384:48 
[error] 384 |  class ConcurrentMapWrapper[K, V](override val underlying: concurrent.Map[K, V]) extends MutableMapWrapper[K, V](underlying) with juc.ConcurrentMap[K, V] {
[error]     |                                                ^
[error]     |error overriding value underlying in class MutableMapWrapper of type scala.collection.mutable.Map[K, V];
[error]     |  value underlying of type scala.collection.concurrent.Map[K, V] cannot override final member value underlying in class MutableMapWrapper

jackson-module-scala:

community-projects/jackson-module-scala/src/test/scala/com/fasterxml/jackson/module/scala/deser/CreatorTest.scala:42:38 
[error] 42 |  case class DerivedCase(override val timestamp: Long, name: String) extends AbstractBase(timestamp)
[error]    |                                      ^
[error]    |error overriding value timestamp in class AbstractBase of type Long;
[error]    |  value timestamp of type Long cannot override final member value timestamp in class AbstractBase
[error] -- [E164] Declaration Error: /__w/dotty/dotty/community-build/community-projects/jackson-module-scala/src/test/scala/com/fasterxml/jackson/module/scala/ser/OverrideValSerializerTest.scala:19:30 
[error] 19 |  case class Sub(override val id: UUID, something: String) extends Base(id)
[error]    |                              ^
[error]    |error overriding value id in class Base of type java.util.UUID;
[error]    |  value id of type java.util.UUID cannot override final member value id in class Base

scala-parser-combinator:

community-build/community-projects/scala-parser-combinators/)
[error] -- [E164] Declaration Error: /__w/dotty/dotty/community-build/community-projects/scala-parser-combinators/shared/src/main/scala/scala/util/parsing/combinator/Parsers.scala:194:34 
[error] 194 |  case class Failure(override val msg: String, override val next: Input) extends NoSuccess(msg, next) {
[error]     |                                  ^
[error]     |error overriding value msg in class NoSuccess of type String;
[error]     |  value msg of type String cannot override final member value msg in class NoSuccess
[error] -- [E164] Declaration Error: /__w/dotty/dotty/community-build/community-projects/scala-parser-combinators/shared/src/main/scala/scala/util/parsing/combinator/Parsers.scala:194:60 
[error] 194 |  case class Failure(override val msg: String, override val next: Input) extends NoSuccess(msg, next) {
[error]     |                                                            ^
[error]     |error overriding value next in class NoSuccess of type Parsers.this.Input;
[error]     |  value next of type Parsers.this.Input cannot override final member value next in class NoSuccess
[error] -- [E164] Declaration Error: /__w/dotty/dotty/community-build/community-projects/scala-parser-combinators/shared/src/main/scala/scala/util/parsing/combinator/Parsers.scala:217:32 
[error] 217 |  case class Error(override val msg: String, override val next: Input) extends NoSuccess(msg, next) {
[error]     |                                ^
[error]     |error overriding value msg in class NoSuccess of type String;
[error]     |  value msg of type String cannot override final member value msg in class NoSuccess
[error] -- [E164] Declaration Error: /__w/dotty/dotty/community-build/community-projects/scala-parser-combinators/shared/src/main/scala/scala/util/parsing/combinator/Parsers.scala:217:58 
[error] 217 |  case class Error(override val msg: String, override val next: Input) extends NoSuccess(msg, next) {
[error]     |                                                          ^
[error]     |error overriding value next in class NoSuccess of type Parsers.this.Input;
[error]     |  value next of type Parsers.this.Input cannot override final member value next in class NoSuccess

sconfig:

community-build/community-projects/sconfig/)
[error] -- [E164] Declaration Error: /__w/dotty/dotty/community-build/community-projects/sconfig/sconfig/shared/src/test/scala/org/ekrich/config/impl/TokenizerTest.scala:360:37 
[error] 360 |    case class LongTest(override val s: String, override val result: Token)
[error]     |                                     ^
[error]     |error overriding value s in class NumberTest of type String;
[error]     |  value s of type String cannot override final member value s in class NumberTest
[error] -- [E164] Declaration Error: /__w/dotty/dotty/community-build/community-projects/sconfig/sconfig/shared/src/test/scala/org/ekrich/config/impl/TokenizerTest.scala:360:61 
[error] 360 |    case class LongTest(override val s: String, override val result: Token)
[error]     |                                                             ^
[error]     |error overriding value result in class NumberTest of type org.ekrich.config.impl.Token;
[error]     |  value result of type org.ekrich.config.impl.Token cannot override final member value result in class NumberTest
[error] -- [E164] Declaration Error: /__w/dotty/dotty/community-build/community-projects/sconfig/sconfig/shared/src/test/scala/org/ekrich/config/impl/TokenizerTest.scala:362:39 
[error] 362 |    case class DoubleTest(override val s: String, override val result: Token)
[error]     |                                       ^
[error]     |error overriding value s in class NumberTest of type String;
[error]     |  value s of type String cannot override final member value s in class NumberTest
[error] -- [E164] Declaration Error: /__w/dotty/dotty/community-build/community-projects/sconfig/sconfig/shared/src/test/scala/org/ekrich/config/impl/TokenizerTest.scala:362:63 
[error] 362 |    case class DoubleTest(override val s: String, override val result: Token)
[error]     |                                                               ^
[error]     |error overriding value result in class NumberTest of type org.ekrich.config.impl.Token;
[error]     |  value result of type org.ekrich.config.impl.Token cannot override final member value result in class NumberTest

[liufengyun]

Given so many existing uses of overriding class parameters, Rule C is not practical. It seems Rule B is the best we can do.

Are there more complicated cases than just class C(override val y: Int) extends B(y)?

Yes, in specs2, we have:

community-build/community-projects/specs2/common/shared/src/main/scala/org/specs2/execute/Result.scala:452:17 
[error] 452 |    override val expectationsNb = n
[error]     |                 ^
[error]     |error overriding value expectationsNb in class Result of type Int;

If yes, if there are more complicated cases, could we have a separate domain for this check, separate from the existing initialization checking domain.

I agree that creating a separate analysis is better. It seems for this analysis, we don't need to look into method bodies (including local methods). Only constructor calls need to be analyzed. Approximating a method call with a fresh symbolic value seems to be enough for real-world use cases.

[odersky]

My initial reaction would be that the gain is not worth the effort. Unlike the core of initialization checking I don't see a convincing guarantee here. We know that some code will behave differently in a constructor. That's a subtlety but (1) that's just the way it is and (2) I would argue it is very rarely encountered in practice. Do we want to spend a considerable amount of our complexity budget in warning people on this? Realistically what would the likely percentage be of warnings that are useful in practice, rather than just countering clever people who think they have discovered another puzzler? I don't care about that second use case at all.

So what is the guarantee we provide here? For initialization checking, the ideal guarantee would be that we never hit pre-initialized zeros, so we might as well not initialize objects on creation. It's easy to see why this guarantee is important. Is there something similarly convincing for this PR?

[som-snytt]

The Scala 2 ticket is old enough to be made murky by "auncient" overriding issues. I think "How to avoid accidentally creating an extra field when class parameter is shadowing?" is still relevant.

scala/bug#4762

In the OP example,

class B(val y: Int):
  println(y)
  foo()
  def foo() = println(y)

class C(override val y: Int) extends B(10)

it seems to me that B is OK but C is in violation. Perhaps this should be implicit:

class B(@deprecatedOverriding val y: Int)

I could not find a ticket but I'm sure people ("the people") ask for a way to avoid

class C(y0: Int = 10) extends B(y0)

without renaming, so I could still write C(y = 17).

class C(_) extends B(_)  // like feature request for inferring parameters from override
class C(y: Int) extends B(y')  // y' prime to say the class param is not shadowing

Finally, I noted at the linked Scala 2 PR scala/scala#10104 that I took the hint in the previous comment and fixed the override in Java collection wrappers (bymaking them no longer case classes).

[liufengyun]

Unlike the core of initialization checking I don't see a convincing guarantee here.
So what is the guarantee we provide here?

The guarantee is the following:

• A class parameter may only be overridden by another class parameter.
• If a parameter is overridden, at the end of class constructors, the overridden and overriding parameters have the same value.

We have conducted an empirical study both for Dotty and community projects, we find no valid use which violates the rules/guarantees above.

For a new language design, it seems good to simply make class parameters final -- which we cannot do in Scala because of legacy code. Therefore, we propose the check as an alternative.

Do we want to spend a considerable amount of our complexity budget in warning people on this

The new check is actually very simple. We only reason about the symbolic value for the overridding parameter is passed to the overridden parameter.

For that purpose, the checker only checks super constructor calls, x and this.a, all other expressions are ignored. It's guaranteed to be fast and always terminates.

[liufengyun]

Fixed by #16096.