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

Inadvertent shadowing of base class fields in derived classes, Warning desirable #4762

Open
scabug opened this issue Jul 4, 2011 · 16 comments
Open
Assignees
Labels

Comments

@scabug
Copy link

@scabug scabug commented Jul 4, 2011

Issue arises whenever (a) a class parameter in a derived class uses the same
symbol as a field or function in a base class and (b) that base class symbol
is subsequently accessed in the derived class. The derived class parameter
causes the compiler to auto-generate a field of the same name that shadows
the base class symbol. Any reference to that symbol in the Derived class,
intended to refer to the base class field, then inadvertently (a) causes
duplicate definition of the field and (b) unexpectedly (but correctly) refers
to the auto-generated field in the derived class.

Code example:

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

Since the 'Base' class has 'val' to generate the field and the derived class
does not, the developer clearly intends to use the 'Derived' 'x' only for
pass-through to Base, and therefore expects the reference in 'toString' to
yield the Base value. Instead, the reference in 'toString' causes the compiler
to automatically generate a field 'Derived.x' that shadows the 'Base.x' field,
resulting in an error. The compiler behaves correctly, but the result is a
programming error.

Usage scenario (with var fields for clearer illustration of the risks):

class Base( var x : Int ) { def increment() { x = x + 1 } }
class Derived( x : Int ) extends Base( x ) { override def toString = x.toString }

val derived = new Derived( 1 )
println( derived.toString )     // yields '1', as expected
derived.increment()
println( derived.toString )     // still '1', probably unexpected

Since this issue arises whenever anyone uses the default way to initialize
base class fields from a derived class in Scala, the scenario would appear to
be extremely common and result in lots of programming errors (easily fixed,
but still) for newer users.

An easy work-around for this issue exists (use differing names for the derived
class parameter, like '_x', 'theX', 'initialX', etc.), but this introduces
unwanted extra symbols.

Solution A (Minimal): issue a warning whenever the compiler infers that a
class parameter requires an auto-generated field in a derived class that
would shadow a symbol already defined in a base class.

Solution B: the work-around, still required with Solution A, is to come up
with a new symbol name each time one initializes a base class field. This
scenario comes up all the time and polluting the namespace with workaround
field names like '_x' and 'theX' seems undesirable. Instead, it might be nice
to have a way to suppress automatic field generation if the developer
determines that the derived class symbols would otherwise end up hiding a base
class symbol (e.g., following the warning of Solution A). Maybe a useful
addition to Scala would be a modifier like 'noval' (or 'passthrough' or
'temp', or whatever - in addition to 'val' and 'var'), like this:

class Base( var x : Int ) { def increment() { x = x + 1 } }
class Derived( noval x : Int ) extends Base( x ) { override def toString = x.toString }

val derived = new Derived( 1 )
println( derived.toString )     // yields '1', as expected
derived.increment()
println( derived.toString )     // still '2', as expected

BTW, this ticket was posted in response to a suggestion by @jsuereth in the related thread on StackOverflow.com, which can be found here: http://stackoverflow.com/questions/6515931/idiomatic-scala-way-to-deal-with-base-vs-derived-class-field-names

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 4, 2011

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

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 4, 2011

@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.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 5, 2011

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

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 5, 2011

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.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 5, 2011

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 )
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Oct 15, 2011

@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))
 }
}
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Oct 15, 2011

Gregor Scheidt (gregor) said:
:-)

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Oct 17, 2011

@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.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Oct 17, 2011

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

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Oct 22, 2011

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.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Mar 25, 2012

@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.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jun 11, 2012

@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.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Aug 11, 2012

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.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Aug 11, 2012

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

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Feb 5, 2014

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 ?

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Mar 13, 2014

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.