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

Private member shadows inherited member #7475

Closed
scabug opened this Issue May 12, 2013 · 21 comments

Comments

Projects
None yet
2 participants
@scabug
Copy link

scabug commented May 12, 2013

Depending on order of traits, private members can shadow inherited members and prevent compilation. This does not happen with separate compilation.

scala> :pa
// Entering paste mode (ctrl-D to finish)

trait A { private val x = 1 }
trait B { val x = 2 }
trait C extends B with A { println(x) }

// Exiting paste mode, now interpreting.

<console>:9: error: value x in trait A cannot be accessed in C
       trait C extends B with A { println(x) }
                                          ^

scala> :reset
Resetting interpreter state.

scala> trait A { private val x = 1 }
defined trait A

scala> trait B { val x = 2 }
defined trait B

scala> trait C extends B with A { println(x) }
defined trait C

Expected behavior (to me anyway) is that private members (which are not inherited) should not shadow inherited members when resolving symbols in subclasses. But in either case behavior should be consistent between combined and separate compilation.

Edit: damn this editor/formatter.

@scabug

This comment has been minimized.

Copy link

scabug commented May 12, 2013

Imported From: https://issues.scala-lang.org/browse/SI-7475?orig=1
Reporter: Rob Norris (rnorris)
Affected Versions: 2.10.1
See #6794, #2568, #261

@scabug

This comment has been minimized.

Copy link

scabug commented May 12, 2013

@paulp said:
The differing behavior is some sort of repl artifact. It doesn't compile together or separately outside the repl.

Here's a test. This should compile and when run print 2 twice.

trait A { private val x = 1 }
trait B { val x = 2 }
trait C1 extends B with A { println(x) }
trait C2 extends A with B { println(x) }

object Test {
  def main(args: Array[String]): Unit = {
    new C1 { }
    new C2 { }
  }
}
@scabug

This comment has been minimized.

Copy link

scabug commented May 13, 2013

Rob Norris (rnorris) said:
Note the same problem occurs if A is a self-type.

trait C3 extends B { self: A => println(x) }

Not sure if this is surprising or not. Another data point.

@scabug

This comment has been minimized.

Copy link

scabug commented Jun 6, 2013

@retronym said:
The name lookup for x searches the base classes C and B.

In regular compilation (joint or separate); the member A::x is found, but is inaccessible, which leads into:

      def searchPrefix = {
        cx = cx.enclClass
        val found0 = lookupInPrefix(name)
        val found1 = found0 filter accessibleInPrefix
        if (found0.exists && !found1.exists && inaccessible == null)
          inaccessible = LookupInaccessible(found0, analyzer.lastAccessCheckDetails)

        found1
      }

In the resident compilation context (e.g. REPL with classes defined line-by-line), the name mangling of A::x is visible:

(baseClasses(1), baseClasses(1).info.decls.toList(1), baseClasses(1).info.decls.toList(1).name)
result = {scala.Tuple3@7304}"(trait A#45776,value x#45779,$line4$$read$$iw$$iw$A$$x)"

The lookup walks further up the base class sequence, finds B::x and happily compiles.

We're all to well aware of the proliferation of this sort of subtle bug in resident compilers; that's why we aren't supporting FSC anymore. But what do to with the beloved REPL. Could we rig it up to reload symbols from classfiles after they have compiled, rather than hanging onto the renmants of the later phases of the original run?

Back to the question of whether the error is valid or not under regular conditions. I hope not.

The private modifier can be used with any definition or declaration in a template. Such members can be accessed only from within the directly enclosing template and its companion module or companion class (§5.4). They are not inherited by subclasses and they may not override definitions in parent classes.

So the following is a bug, no? B should not have inherited a.

scala> class A {
     |   private val a: Int = 0
     |   class B extends A {
     |     def foo(a: A) = a.a // okay
     |     println(this.a)     // wait, what?
     |   }
     | }
defined class A
@scabug

This comment has been minimized.

Copy link

scabug commented Jun 18, 2013

@retronym said:
Here's a similar example distilled from Akka + JDK8 (which introduced an private field into PriorityBlockingQueue).

trait AbstractPublic {
  def queue: Any
}
trait ConcretePrivate {
  private val queue: Any = ()
}

abstract class Mix
  extends ConcretePrivate with AbstractPublic {
  final def queue: Any = ()
}
@scabug

This comment has been minimized.

@scabug

This comment has been minimized.

Copy link

scabug commented Jun 25, 2013

@retronym said:
Martin noted that in the early days of findMember, it used to exclude private members beyond the first base class. This was removed in 88a54be3877. He can't remember why. Theories: 1) to make sure that private members are intersected in refinement types (see example below that doesn't compile under my patch), or 2) to make sure private members are visible in classes with a self type.

trait T {
  type TT = T with Any
  private val priv = 0
  (??? : TT).priv // we currently allow this, and should continue to do so.
}

Collateral work: I just noticed that findMember is duplicated with findMembers. It was an attempted performance optimization: 04f0b659efe8. Needless to say, they have started to drift apart.

@scabug

This comment has been minimized.

Copy link

scabug commented Jun 25, 2013

@paulp said:
If only it were needless to say. That copy-paste job is only a year old.

@scabug

This comment has been minimized.

Copy link

scabug commented Jul 28, 2013

@paulp said:
See also recently reopened #2568, where it was clearly pointed out that private members were inherited in violation of the specification, and it was closed wontfix anyway.

@scabug

This comment has been minimized.

Copy link

scabug commented Oct 15, 2013

@gkossakowski said:
Unassigning and rescheduling to M7 as previous deadline was missed.

@scabug

This comment has been minimized.

Copy link

scabug commented Nov 19, 2013

@som-snytt said:
I haven't seen #261 mentioned as an early duplicate, though bugs, like features, evolve over time.

@scabug

This comment has been minimized.

Copy link

scabug commented Jan 16, 2014

@retronym said:
Another one that shouldn't be allowed, but it:

class A {
  private val foo = 1
  (new B).foo // refchecks, but should not
}

class B extends A
@scabug

This comment has been minimized.

Copy link

scabug commented Jan 29, 2014

@retronym said (edited by @adriaanm on Feb 10, 2014 12:46:50 AM UTC):
scala/scala#3440

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 14, 2014

@retronym said:
followup discussion regarding the interaction of this change with pattern typing: https://groups.google.com/forum/#!topic/scala-internals/h8ztMqw-3Xg

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 14, 2014

@adriaanm said:
Optimization == evil.root. I'm happy to submit a PR to remove it from inferTypedPattern and just use intersectionType.

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 14, 2014

@retronym said:
I just checked and it does leak into error messages.

% git diff
diff --git a/test/files/pos/t7475f.scala b/test/files/pos/t7475f.scala
index 45a4000..275d997 100644
--- a/test/files/pos/t7475f.scala
+++ b/test/files/pos/t7475f.scala
@@ -6,6 +6,7 @@ class C {
   private def foo: Any = 0
   this match {
     case d: D =>
+      d: String
       d.foo
   }
 }

% qbin/scalac test/files/pos/t7475f.scala
test/files/pos/t7475f.scala:9: error: type mismatch;
 found   : C with D
 required: String
      d: String

But that's less of a WTF than not being able to access the private member of the refined type of a binder, so if we're after a simple change that's okay with me.

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 14, 2014

@adriaanm said:
As always, there's another catch: test/files/run/t2755.scala CCEs if you don't eliminate subtypes… it's probably related to another issue with Array.
If we take the glb instead of the intersection, this problem doesn't arise.

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 14, 2014

@adriaanm said:
We already do this for extractor patterns btw:

      def makeTypedUnApply() = {
        // the union of the expected type and the inferred type of the argument to unapply
        val glbType        = glb(ensureFullyDefined(pt) :: unapplyArg.tpe_* :: Nil)
@scabug

This comment has been minimized.

Copy link

scabug commented Feb 14, 2014

@adriaanm said:
well, duh -- glb removes the upper type as well of course

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 15, 2014

@adriaanm said:
Shooting at dusk: scala/scala#3535

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 15, 2014

@adriaanm said:
I think this issue can be closed, opening a new one for the pattern typing. That will have to wait until 2.12, unfortunately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment