Imported From: https://issues.scala-lang.org/browse/SI-4762?orig=1
Reporter: Gregor Scheidt (gregor)
Affected Versions: 2.8.1, 2.9.0, 2.9.1

@paulp said:
It's not helpful that your first code sample doesn't generate a field in Derived. There is a very serious bug here, which is related to #3194 but worse.

class A(var x: Int) {
  def increment() { x = x + 1 }
}
class B(x: Int) extends A(x) {
  def y = this.x
}

object Test {
  def main(args: Array[String]): Unit = {
    val x = new B(100)
    x.increment()
    assert(x.x == x.y)
  }
}

That assertion fails. That is very bad. It would be bad enough if "def y = x" asserted, which is what is being reported here, and which is a problem in its own right. (And of course it does.) But to have "this.x" silently using the initial constructor parameter in this potentially common scenario with no warning or anything is a correctness disaster.

Gregor Scheidt (gregor) said:
Added link to StackOverflow thread.

Gregor Scheidt (gregor) said (edited on Jul 5, 2011 9:16:12 AM UTC):
Sorry if I made a mistake in the example. But I am not entirely sure why it would not create a field in Derived. Adding a minimal modification (just to make the values distinguishable) I would give us this code:

    class Base( val x : String )
    class Derived( x : String ) extends Base( "Base's " + x ) { override def toString = x }
    println( new Derived("value").toString )    // returns "value" (expected: "Base's value")

which seems to indicate that a field Derived.x (containing "value") is generated in addition to Base.x (containing "Base's value"). Which actually seems like correct behavior, just potentially confusing. Anyway, hope filing the ticket was useful.

Gregor Scheidt (gregor) said:
OK, and just for completeness' sake the answer for the idiomatically correct workaround that Aaron Novstrup provided to my original question on StackOverflow (use an abstract field on the base class), modified a bit to match your example code above:

    abstract class A { var x: Int; def increment() { x+=1 } }
    class B (var x: Int) extends A { override def toString = x.toString }
    val b = new B(0)
    assert( b.x == 0 )
    b.increment()
    assert( b.x == 1 )

@paulp said:
Fancier example for a more visceral horror show.

// In A, x and y are -1.
class A(val x: Int) {
 val y: Int = -1
}

// In B, x and y are 99 and private[this], implicitly so
// in the case of x.
class B(x: Int) extends A(-1) {
 private[this] def y: Int = 99

 // Three distinct results.
 def f = List(
   /* (99,99) */  (this.x, this.y),
   /* (-1,99) */  ((this: B).x, (this: B).y),
   /* (-1,-1) */  ((this: A).x, (this: A).y)
 )

 // The 99s tell us we are reading the private[this]
 // data of a different instance.
 def g(b: B) = List(
   /* (-1,99) */  (b.x, b.y),
   /* (-1,99) */  ((b: B).x, (b: B).y),
   /* (-1,-1) */  ((b: A).x, (b: A).y)
 )
}

object Test {
 def f(x: A)  = /* -2 */  x.x + x.y
 def g1(x: B) = /* -2 */  (x: A).x + (x: A).y
 def g2(x: B) = (x: B).x + (x: B).y
 // java.lang.IllegalAccessError: tried to access method B.y()I from
class Test$

 def main(args: Array[String]): Unit = {
   val b = new B(99)
   b.f foreach println
   b.g(new B(99)) foreach println

   println(f(b))
   println(g1(b))
   println(g2(b))
 }
}

Gregor Scheidt (gregor) said:
:-)

@odersky said:
Don't panic! The only real problem I can see here is that the private[this] def x in B should not override the inherited x yet it does. I believe the same error is in order here as if x had been declared private def x. For private[this] val x (which is far more common) the test is not necessary because fields don't override on the JVM. That's a relief because adding the test would probably cause lots of programs to break.

@odersky said:
[y/x] in my comment above :-)

Commit Message Bot (anonymous) said:
(extempore in r25875) Warn about surprising shadowing.

It's hidden behind -Xlint and pretty specific, but makes me feel
better anyway. References #4762, no review.

@Blaisorblade said:
Nobody commented on runtime overhead of field duplication, which is annoying to avoid.

class Base( val x : String )
class Derived( x : String ) extends Base( x ) { override def toString = x }

The above code, for instance, will produce two 'x' fields in \code{Derived}, apparently. I found that variations of this code, however, will not, but I have been unable to gain control of the issue other than by trial and error.

@paulp said:
Allowing the reading of private[this] fields in different instances is in direct violation of the spec. If that part is wontfix, I would like to see the specification updated before the ticket is closed.

DaveScala (davescala) said:
Suggestion:
Add warning when fields are shadowed (directly by programmer or indirectly by compiler) and a language feature shadowedFields to turn this warning off.

DaveScala (davescala) said:
Manifestation of this problem:
https://groups.google.com/forum/?hl=en&fromgroups#!topic/scala-user/tjrnxpuzfNA%5B1-25%5D

Erik Allik (eallik) said:
Would this be considered a manifestation of the same underlying problem: http://stackoverflow.com/questions/21580309/in-scala-what-is-the-reasoning-behind-allowing-parameter-shadowing ?

@som-snytt said:
Erik asks about locals shadowing parameters. Might be nice to -Ywarn-unused if an unused param is also shadowed; and -Ywarn-unused-parameter unconditionally. That would need a ticket, I think.