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

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Sep 25, 2022

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

We disallow overriding of val parameters, which fixes the soundness problem
discovered in scala#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.
Currently, the following CB projects have illegal overrides of val parameters

 - spire
 - scalaz
 - specs2
 - akka

I checked the spire issue and its seems to require a non-trivial refactoring to avoid the problem.
More than I could achieve, given that I know nothing of spire.

In light of this I think we can enforce the restriction only under -source future and make it a
deprecation warning for now.
@odersky
Copy link
Contributor Author

odersky commented Sep 25, 2022

Currently, the following CB projects have illegal overrides of val parameters

  • spire
  • scalaz
  • specs2
  • akka

I checked the spire issue and its seems to require a non-trivial refactoring to avoid the problem.
More than I could achieve, given that I know nothing of spire.

In light of this I think we can enforce the restriction only under -source future and make it a
deprecation warning for now.

@odersky
Copy link
Contributor Author

odersky commented Sep 25, 2022

@sjrd: jsinterop/NonNativeJSTypeTest.scala also has a by-now illegal override, which needs to be fixed.

@odersky
Copy link
Contributor Author

odersky commented Sep 26, 2022

In fact, there is a systematic refactoring available, which should work for spire and all other projects. Given a super-class

class A(val x: T):
  ...

where x is overridden in a subclass, change it to

class A(_x: T):
  val x = _x

That works universally. It does not have the same initialization behavior as the superclass, since x now always would have the value of the override. But given that the open PR #16066 is precisely about warning about this dubious behavior we should be safe.

@dwijnand
Copy link
Member

_x doesn't need to be a val, right? Just thinking in terms of binary compatibility, not making it val means not changing the binary API.

@sjrd
Copy link
Member

sjrd commented Sep 26, 2022

The serious issue I see here is for the subclasses. The maintainer of the superclass can address the problem, but it's the maintainer of the subclass who gets the error. When the subclass upgrades to the newer compiler, it is faced with a compile error that it cannot address in any way (let alone a way that preserves its binary compatibility guarantees).

Instead of making this an error under future, have you considered making it a warning right away? A warning that says very clearly that it will become an error in the future? The warning could be silenced locally while the issue gets addressed in the superclass.

That's the approach we've taken in Scala.js about unsoundness issues. If there was any remote way that it could be valid in some use cases, we made it a warning in some 0.6.x version, and turned it into actual errors in 1.0.0.

@odersky
Copy link
Contributor Author

odersky commented Sep 26, 2022

It is already a deprecation warning in this PR.

Temporarily, always report an error instead of a deprecation warning to verify
that projects compile. This will be reverted in the next commit.
Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

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

For proper language design, making class parameters effectively final is the right direction to go --- for soundness, but also for semantic clarity 👍

But given that the open PR #16066 is precisely about warning about this dubious behavior we should be safe.

To clarify, the PR #16066 tries to address the same problem as this PR does: overcome problems caused by overriding of val parameters (Issue #15764).

In the issue #15764 , @olhotak and I initially tried to make val parameters final, but it turns out to cause too many migration problems from empirical study: #15764 (comment).

To make it easier for migration, #16066 allows overriding of val parameters but ensures that:

  • A class parameter may only be overridden by a class parameter.
  • If a class parameter overrides another class parameter, they should have the same value at the end of the constructor.

In particular, it makes an effort to support secondary constructors & trait parameters. However, as it's just a migration facility, I think the solution in this PR is good enough therefore we do not need #16066.

@@ -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 odersky merged commit 8d723a9 into scala:main Sep 27, 2022
@odersky odersky deleted the fix-16092 branch September 27, 2022 12:58
@Kordyjan Kordyjan added this to the 3.2.2 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unsoundness from overriding vals
5 participants