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

Unsoundness from overriding vals #16092

Closed
howtonotwin opened this issue Sep 24, 2022 · 3 comments · Fixed by #16096
Closed

Unsoundness from overriding vals #16092

howtonotwin opened this issue Sep 24, 2022 · 3 comments · Fixed by #16096
Labels
area:typer itype:bug itype:soundness Soundness bug (it lets us compile code that crashes at runtime with a ClassCastException)
Milestone

Comments

@howtonotwin
Copy link

Tested on Dotty 3.2.0 and 3.2.1-RC2 (both via Scastie). What it says on the tin: overriding a val basically makes paths containing that val unstable. I searched the Dotty issue tracker for this issue and didn't find it, but I was expecting that a (serious?) soundness failure would be there. So this might be a duplicate.

Code

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 // 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)
}
new Evil(x1, x2, 42).process() // BOOM: basically did x2.process(42)

Output

No compiler error, and a ClassCastException when run.

Expectation

Compiler error of some kind.

@howtonotwin howtonotwin added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Sep 24, 2022
@odersky
Copy link
Contributor

odersky commented Sep 24, 2022

Good point. val overriding has been eyed with unease for a long time, but the objections were mostly about the weird initialization behavior. This might be the final straw to disallow it altogether.

@odersky
Copy link
Contributor

odersky commented Sep 24, 2022

I tried to construct a similar unsoundness example using just member val overriding (no val parameters) but failed. It would be good to find out whether that's just my lack of imagination or indeed it's necessary to have an overridden val parameter to exhibit the unsoundness.

odersky added a commit to dotty-staging/dotty that referenced this issue Sep 25, 2022
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.
@jchyb jchyb added area:typer itype:soundness Soundness bug (it lets us compile code that crashes at runtime with a ClassCastException) and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Sep 25, 2022
@abgruszecki
Copy link
Contributor

For what it's worth, I took a look at this issue and I also can't find an example where a similar issue occurs without using val parameters. It seems that the real problem is allowing Evil to initialize x twice.

odersky added a commit that referenced this issue Sep 27, 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 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
mpollmeier pushed a commit to mpollmeier/dotty that referenced this issue Oct 16, 2022
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.
@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
area:typer itype:bug itype:soundness Soundness bug (it lets us compile code that crashes at runtime with a ClassCastException)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants