SI-8143 Regressions with override checks, private members

These regressed in e609f1f20b, which excluded all private methods from
overriding checks. We should only exclude private[this] members on the
low end of a pair, as was done before that commit, and, we must also
exclude private members on the high side.

Why? Warning: reverse engineered intuition follows.

We need to report an error when if a private method in a subclass
has matches a less-private method in the super class and report an
error, lest the user be fooled into thinking it might be invoked
virtually. On the other hand, adding a private method to a super
class shouldn't invalidate the choice names of public members in
its superclasses.

I've removed the test case added by that commit and will lodge a
reworked version of it that Paul provided as a new issue. That shows
a bug with qualified private + inheritance.

In addition, the expectation of `neg/accesses.check` is reverted
to its 2.10.3 version, which I believe is correct. When it was
changed in e609f1f20b it sprouted a variation, `neg/accesses-2`,
which has now changed behaviour. The intent of that test will
be captured in the aforementioned issue covering qualified private
inheritance.

9 files changed, 46 insertions, 29 deletions

diff --git a/src/compiler/scala/tools/nsc/transform/Erasure.scala b/src/compiler/scala/tools/nsc/transform/Erasure.scala
index 6732900ef2..54b5b399ac 100644
--- a/src/compiler/scala/tools/nsc/transform/Erasure.scala
+++ b/src/compiler/scala/tools/nsc/transform/Erasure.scala
@@ -760,7 +760,7 @@ abstract class Erasure extends AddInterfaces
           || super.exclude(sym)
           || !sym.hasTypeAt(currentRun.refchecksPhase.id)
         )
-        override def matches(sym1: Symbol, sym2: Symbol) = true
+        override def matches(lo: Symbol, high: Symbol) = true
       }
       def isErasureDoubleDef(pair: SymbolPair) = {
         import pair._
diff --git a/src/compiler/scala/tools/nsc/transform/OverridingPairs.scala b/src/compiler/scala/tools/nsc/transform/OverridingPairs.scala
index 4222c4d8c8..870eafbf20 100644
--- a/src/compiler/scala/tools/nsc/transform/OverridingPairs.scala
+++ b/src/compiler/scala/tools/nsc/transform/OverridingPairs.scala
@@ -24,15 +24,16 @@ abstract class OverridingPairs extends SymbolPairs {
     /** Symbols to exclude: Here these are constructors and private/artifact symbols,
      *  including bridges. But it may be refined in subclasses.
      */
-    override protected def exclude(sym: Symbol) = (sym hasFlag PRIVATE | ARTIFACT) || sym.isConstructor
+    override protected def exclude(sym: Symbol) = sym.isPrivateLocal || sym.isArtifact || sym.isConstructor
 
     /** Types always match. Term symbols match if their member types
      *  relative to `self` match.
      */
-    override protected def matches(sym1: Symbol, sym2: Symbol) = sym1.isType || (
-         (sym1.owner != sym2.owner)
-      && !exclude(sym2)
-      && relatively.matches(sym1, sym2)
-    )
+    override protected def matches(lo: Symbol, high: Symbol) = lo.isType || (
+         (lo.owner != high.owner)     // don't try to form pairs from overloaded members
+      && !high.isPrivate              // private or private[this] members never are overriden
+      && !exclude(lo)                 // this admits private, as one can't have a private member that matches a less-private member.
+      && relatively.matches(lo, high)
+    ) // TODO we don't call exclude(high), should we?
   }
 }
diff --git a/src/compiler/scala/tools/nsc/typechecker/Checkable.scala b/src/compiler/scala/tools/nsc/typechecker/Checkable.scala
index 94f8f509fc..b899cd8994 100644
--- a/src/compiler/scala/tools/nsc/typechecker/Checkable.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/Checkable.scala
@@ -186,7 +186,7 @@ trait Checkable {
      *  additional conditions holds:
      *   - either A or B is effectively final
      *   - neither A nor B is a trait (i.e. both are actual classes, not eligible for mixin)
-     *   - both A and B are sealed, and every possible pairing of their children is irreconcilable
+     *   - both A and B are sealed/final, and every possible pairing of their children is irreconcilable
      *
      *  TODO: the last two conditions of the last possibility (that the symbols are not of
      *  classes being compiled in the current run) are because this currently runs too early,
@@ -198,8 +198,9 @@ trait Checkable {
          isEffectivelyFinal(sym1) // initialization important
       || isEffectivelyFinal(sym2)
       || !sym1.isTrait && !sym2.isTrait
-      || sym1.isSealed && sym2.isSealed && allChildrenAreIrreconcilable(sym1, sym2) && !currentRun.compiles(sym1) && !currentRun.compiles(sym2)
+      || isSealedOrFinal(sym1) && isSealedOrFinal(sym2) && allChildrenAreIrreconcilable(sym1, sym2) && !currentRun.compiles(sym1) && !currentRun.compiles(sym2)
     )
+    private def isSealedOrFinal(sym: Symbol) = sym.isSealed || sym.isFinal
     private def isEffectivelyFinal(sym: Symbol): Boolean = (
       // initialization important
       sym.initialize.isEffectivelyFinal || (
diff --git a/src/reflect/scala/reflect/internal/SymbolPairs.scala b/src/reflect/scala/reflect/internal/SymbolPairs.scala
index b538648b36..c088e8f57c 100644
--- a/src/reflect/scala/reflect/internal/SymbolPairs.scala
+++ b/src/reflect/scala/reflect/internal/SymbolPairs.scala
@@ -125,7 +125,7 @@ abstract class SymbolPairs {
      *  considered as a (lo, high) pair? Types always match. Term symbols
      *  match if their member types relative to `self` match.
      */
-    protected def matches(sym1: Symbol, sym2: Symbol): Boolean
+    protected def matches(lo: Symbol, high: Symbol): Boolean
 
     /** The parents and base classes of `base`.  Can be refined in subclasses.
      */
diff --git a/test/files/neg/accesses.check b/test/files/neg/accesses.check
index 5a5e03233e..db58af12ce 100644
--- a/test/files/neg/accesses.check
+++ b/test/files/neg/accesses.check
@@ -1,3 +1,7 @@
+accesses.scala:23: error: overriding method f2 in class A of type ()Unit;
+ method f2 has weaker access privileges; it should not be private
+  private def f2(): Unit = ()
+              ^
 accesses.scala:24: error: overriding method f3 in class A of type ()Unit;
  method f3 has weaker access privileges; it should be at least protected
   private[p2] def f3(): Unit = ()
@@ -10,4 +14,4 @@ accesses.scala:26: error: overriding method f5 in class A of type ()Unit;
  method f5 has weaker access privileges; it should be at least protected[p1]
   protected[p2] def f5(): Unit
                     ^
-three errors found
+four errors found
diff --git a/test/files/neg/accesses2.check b/test/files/neg/accesses2.check
index 554a7b4c81..66cf9a116e 100644
--- a/test/files/neg/accesses2.check
+++ b/test/files/neg/accesses2.check
@@ -1,4 +1,12 @@
+accesses2.scala:6: error: overriding method f2 in class A of type ()Int;
+ method f2 has weaker access privileges; it should not be private
+    private def f2(): Int = 1
+                ^
 accesses2.scala:5: error: class B1 needs to be abstract, since method f2 in class A of type ()Int is not defined
   class B1 extends A {
         ^
-one error found
+accesses2.scala:9: error: overriding method f2 in class A of type ()Int;
+ method f2 has weaker access privileges; it should not be private
+    private def f2(): Int = 1
+                ^
+three errors found
diff --git a/test/files/neg/t8143a.check b/test/files/neg/t8143a.check
new file mode 100644
index 0000000000..4e11000a2a
--- /dev/null
+++ b/test/files/neg/t8143a.check
@@ -0,0 +1,5 @@
+t8143a.scala:2: error: overriding method f in class Foo of type => Int;
+ method f has weaker access privileges; it should not be private
+class Bar extends Foo { private def f = 10 }
+                                    ^
+one error found
diff --git a/test/files/neg/t8143a.scala b/test/files/neg/t8143a.scala
new file mode 100644
index 0000000000..4ec539e671
--- /dev/null
+++ b/test/files/neg/t8143a.scala
@@ -0,0 +1,15 @@
+class Foo { def f = 5 }
+class Bar extends Foo { private def f = 10 }
+
+
+class Foo1 { private def f = 5 }
+class Bar1 extends Foo1 { def f = 10 } // okay
+
+class Foo2 { private def f = 5 }
+class Bar2 extends Foo2 { private def f = 10 } // okay
+
+class Foo3 { private[this] def f = 5 }
+class Bar3 extends Foo3 { private def f = 10 } // okay
+
+class Foo4 { private def f = 5 }
+class Bar4 extends Foo4 { private[this] def f = 10 } // okay
\ No newline at end of file
diff --git a/test/files/run/private-override.scala b/test/files/run/private-override.scala
deleted file mode 100644
index 0a3f57f97c..0000000000
--- a/test/files/run/private-override.scala
+++ /dev/null
@@ -1,17 +0,0 @@
-package test.p1.p2 {
-  abstract class A {
-    private[p2] def f2(): Int = 1
-  }
-  abstract class Other extends A {
-    // It's a private method - not a private[p2] method. Not a failed
-    // "weaker access privileges" override, a different namespace.
-    private def f2(): Int = super.f2() + 2
-    def go() = f2()
-  }
-}
-
-object Test extends test.p1.p2.Other {
-  def main(args: Array[String]): Unit = {
-    println(go())
-  }
-}