closes #3575.

cloneSymbol now preserves privateWithin -- need to reset it explicitly now when before it was assumed to be not to be carried over

rewrote accessibility in overriding checks so they're more readable, but
hopefully with same semantics

review by odersky

9 files changed, 62 insertions, 30 deletions

diff --git a/src/compiler/scala/tools/nsc/backend/opt/ClosureElimination.scala b/src/compiler/scala/tools/nsc/backend/opt/ClosureElimination.scala
index 9470078507..28aadda14d 100644
--- a/src/compiler/scala/tools/nsc/backend/opt/ClosureElimination.scala
+++ b/src/compiler/scala/tools/nsc/backend/opt/ClosureElimination.scala
@@ -196,10 +196,7 @@ abstract class ClosureElimination extends SubComponent {
 
     /** is field 'f' accessible from method 'm'? */
     def accessible(f: Symbol, m: Symbol): Boolean =
-      f.isPublic || (f.hasFlag(Flags.PROTECTED) && (enclPackage(f) == enclPackage(m)))
-
-    private def enclPackage(sym: Symbol): Symbol =
-      if ((sym == NoSymbol) || sym.isPackageClass) sym else enclPackage(sym.owner)
+      f.isPublic || (f.hasFlag(Flags.PROTECTED) && (f.enclosingPackageClass == m.enclosingPackageClass))
 
   } /* class ClosureElim */
 
diff --git a/src/compiler/scala/tools/nsc/symtab/Symbols.scala b/src/compiler/scala/tools/nsc/symtab/Symbols.scala
index c7d198b638..eb4e56a535 100644
--- a/src/compiler/scala/tools/nsc/symtab/Symbols.scala
+++ b/src/compiler/scala/tools/nsc/symtab/Symbols.scala
@@ -1074,7 +1074,7 @@ trait Symbols extends reflect.generic.Symbols { self: SymbolTable =>
     /** A clone of this symbol, but with given owner */
     final def cloneSymbol(owner: Symbol): Symbol = {
       val newSym = cloneSymbolImpl(owner)
-      // newSym.privateWithin = privateWithin // ?
+      newSym.privateWithin = privateWithin
       newSym.setInfo(info.cloneInfo(newSym))
         .setFlag(this.rawflags).setAnnotations(this.annotations)
     }
@@ -1194,19 +1194,25 @@ trait Symbols extends reflect.generic.Symbols { self: SymbolTable =>
      */
     def ancestors: List[Symbol] = info.baseClasses drop 1
 
-    /** The package containing this symbol, or NoSymbol if there
+    /** The package class containing this symbol, or NoSymbol if there
      *  is not one. */
-    def enclosingPackage: Symbol =
+    def enclosingPackageClass: Symbol =
       if (this == NoSymbol) this else {
         var packSym = this.owner
         while ((packSym != NoSymbol)
                && !packSym.isPackageClass)
           packSym = packSym.owner
-        if (packSym != NoSymbol)
-          packSym = packSym.companionModule
         packSym
       }
 
+    /** The package containing this symbol, or NoSymbol if there
+     *  is not one. */
+    def enclosingPackage: Symbol = {
+      val packSym = enclosingPackageClass
+      if (packSym != NoSymbol) packSym.companionModule
+      else packSym
+    }
+
     /** The top-level class containing this symbol */
     def toplevelClass: Symbol =
       if (owner.isPackageClass) {
diff --git a/src/compiler/scala/tools/nsc/typechecker/Namers.scala b/src/compiler/scala/tools/nsc/typechecker/Namers.scala
index 91d7b1d12c..cd5de7cb7b 100644
--- a/src/compiler/scala/tools/nsc/typechecker/Namers.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/Namers.scala
@@ -375,7 +375,7 @@ trait Namers { self: Analyzer =>
                  context.unit.isJava) &&
                  !mods.isLazy) {
               val vsym = owner.newValue(tree.pos, name).setFlag(mods.flags);
-              if(context.unit.isJava) setPrivateWithin(tree, vsym, mods) // #3663
+              if(context.unit.isJava) setPrivateWithin(tree, vsym, mods) // #3663 -- for Scala fields we assume private[this]
               tree.symbol = enterInScope(vsym)
               finish
             } else {
diff --git a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
index 5c577d8af4..b63f8b0add 100644
--- a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
@@ -266,12 +266,12 @@ abstract class RefChecks extends InfoTransform {
           }
         }
 
+        def accessFlagsToString(sym: Symbol)
+          = flagsToString(sym getFlag (PRIVATE | PROTECTED), if (sym.privateWithin eq NoSymbol) "" else sym.privateWithin.name.toString)
+
         def overrideAccessError() {
-          val pwString = if (other.privateWithin == NoSymbol) ""
-                         else other.privateWithin.name.toString
-          val otherAccess = flagsToString(other getFlag (PRIVATE | PROTECTED), pwString)
-          overrideError("has weaker access privileges; it should be "+
-                        (if (otherAccess == "") "public" else "at least "+otherAccess))
+          val otherAccess = accessFlagsToString(other)
+          overrideError("has weaker access privileges; it should be "+ (if (otherAccess == "") "public" else "at least "+otherAccess))
         }
 
         //Console.println(infoString(member) + " overrides " + infoString(other) + " in " + clazz);//DEBUG
@@ -302,14 +302,24 @@ abstract class RefChecks extends InfoTransform {
 
         if (typesOnly) checkOverrideTypes()
         else {
-          if (member hasFlag PRIVATE) { // (1.1)
+          // o: public | protected        | package-protected  (aka java's default access)
+          // ^-may be overridden by member with access privileges-v
+          // m: public | public/protected | public/protected/package-protected-in-same-package-as-o
+
+          if (member hasFlag PRIVATE) // (1.1)
             overrideError("has weaker access privileges; it should not be private")
-          }
-          val mb = member.accessBoundary(member.owner)
+
+          @inline def definedIn(sym: Symbol, in: Symbol) = sym != RootClass && sym != NoSymbol && sym.hasTransOwner(in)
+
           val ob = other.accessBoundary(member.owner)
-          if (mb != RootClass && mb != NoSymbol && // todo: change
-              (ob == RootClass || ob == NoSymbol || !ob.hasTransOwner(mb) ||
-               (other hasFlag PROTECTED) && !(member hasFlag PROTECTED))) {
+          val mb = member.accessBoundary(member.owner)
+          // println("checking override in "+ clazz +"\n  other: "+ infoString(other) +" ab: "+ ob.ownerChain +" flags: "+ accessFlagsToString(other))
+          // println("  overriding member: "+ infoString(member) +" ab: "+ mb.ownerChain +" flags: "+ accessFlagsToString(member))
+          // todo: align accessibility implication checking with isAccessible in Contexts
+          if (!(  mb == RootClass // m is public, definitely relaxes o's access restrictions (unless o.isJavaDefined, see below)
+               || mb == NoSymbol  // AM: what does this check?? accessBoundary does not ever seem to return NoSymbol (unless member.owner were NoSymbol)
+               || ((!(other hasFlag PROTECTED) || (member hasFlag PROTECTED)) && definedIn(ob, mb)) // (if o isProtected, so is m) and m relaxes o's access boundary
+               )) {
             overrideAccessError()
           }
           else if (other.isClass || other.isModule) {
@@ -379,7 +389,7 @@ abstract class RefChecks extends InfoTransform {
             // this overlaps somewhat with validateVariance
             if(member.isAliasType) {
               val kindErrors = typer.infer.checkKindBounds(List(member), List(memberTp.normalize), self, member.owner)
-2
+
               if(!kindErrors.isEmpty)
                 unit.error(member.pos,
                   "The kind of the right-hand side "+memberTp.normalize+" of "+member.keyString+" "+
@@ -534,11 +544,19 @@ abstract class RefChecks extends InfoTransform {
        */
       def hasMatchingSym(inclazz: Symbol, member: Symbol): Boolean =
         inclazz != clazz && {
+          lazy val memberEnclPackageCls = member.enclosingPackageClass
+
           val isVarargs = hasRepeatedParam(member.tpe)
           inclazz.info.nonPrivateDecl(member.name).filter { sym =>
-            !sym.isTerm || {
+            (!sym.isTerm || {
               val symtpe = clazz.thisType.memberType(sym)
               (member.tpe matches symtpe) || isVarargs && (toJavaRepeatedParam(member.tpe) matches symtpe)
+            }) && {
+              // http://java.sun.com/docs/books/jls/third_edition/html/names.html#6.6.5:
+              // If a public class has a [member] with default access, then this [member] is not accessible to,
+              // or inherited by a subclass declared outside this package.
+              // (sym is a java member with default access in pkg P) implies (member's enclosing package == P)
+              !(inclazz.isJavaDefined && sym.privateWithin == sym.enclosingPackageClass) || memberEnclPackageCls == sym.privateWithin
             }
           } != NoSymbol
         }
diff --git a/src/compiler/scala/tools/nsc/typechecker/SuperAccessors.scala b/src/compiler/scala/tools/nsc/typechecker/SuperAccessors.scala
index 7662e0a670..a4911de3bd 100644
--- a/src/compiler/scala/tools/nsc/typechecker/SuperAccessors.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/SuperAccessors.scala
@@ -433,8 +433,8 @@ abstract class SuperAccessors extends transform.Transform with transform.TypingT
        && sym.hasFlag(JAVA)
        && !sym.owner.isPackageClass
        && !accessibleThroughSubclassing
-       && (enclPackage(sym.owner) != enclPackage(currentOwner))
-       && (enclPackage(sym.owner) == enclPackage(sym.accessBoundary(sym.owner))))
+       && (sym.owner.enclosingPackageClass != currentOwner.enclosingPackageClass)
+       && (sym.owner.enclosingPackageClass == sym.accessBoundary(sym.owner.enclosingPackageClass)))
 
       if (res) {
         val host = hostForAccessorOf(sym, currentOwner.enclClass)
@@ -452,10 +452,6 @@ abstract class SuperAccessors extends transform.Transform with transform.TypingT
       } else res
     }
 
-    /** Return the enclosing package of the given symbol. */
-    private def enclPackage(sym: Symbol): Symbol =
-      if ((sym == NoSymbol) || sym.isPackageClass) sym else enclPackage(sym.owner)
-
     /** Return the innermost enclosing class C of referencingClass for which either
      *  of the following holds:
      *     - C is a subclass of sym.owner or
@@ -464,7 +460,7 @@ abstract class SuperAccessors extends transform.Transform with transform.TypingT
     private def hostForAccessorOf(sym: Symbol, referencingClass: Symbol): Symbol = {
       if (referencingClass.isSubClass(sym.owner.enclClass)
           || referencingClass.thisSym.isSubClass(sym.owner.enclClass)
-          || enclPackage(referencingClass) == enclPackage(sym.owner)) {
+          || referencingClass.enclosingPackageClass == sym.owner.enclosingPackageClass) {
         assert(referencingClass.isClass)
         referencingClass
       } else if(referencingClass.owner.enclClass != NoSymbol)
diff --git a/src/compiler/scala/tools/nsc/typechecker/SyntheticMethods.scala b/src/compiler/scala/tools/nsc/typechecker/SyntheticMethods.scala
index 0da00f5b83..60374b3a55 100644
--- a/src/compiler/scala/tools/nsc/typechecker/SyntheticMethods.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/SyntheticMethods.scala
@@ -227,6 +227,7 @@ trait SyntheticMethods extends ast.TreeDSL {
         var newAcc = tree.symbol.cloneSymbol
         newAcc.name = context.unit.fresh.newName(tree.symbol.pos.focus, tree.symbol.name + "$")
         newAcc setFlag SYNTHETIC resetFlag (ACCESSOR | PARAMACCESSOR | PRIVATE)
+        newAcc.privateWithin = NoSymbol
         newAcc = newAcc.owner.info.decls enter newAcc
         val result = typer typed { DEF(newAcc) === rhs.duplicate }
         log("new accessor method " + result)
diff --git a/test/files/neg/t3757.check b/test/files/neg/t3757.check
new file mode 100644
index 0000000000..1507df8c4f
--- /dev/null
+++ b/test/files/neg/t3757.check
@@ -0,0 +1,4 @@
+B.scala:4: error: method foo overrides nothing
+  override def foo = "B"
+               ^
+one error found
diff --git a/test/files/neg/t3757/A.java b/test/files/neg/t3757/A.java
new file mode 100644
index 0000000000..37da86fe15
--- /dev/null
+++ b/test/files/neg/t3757/A.java
@@ -0,0 +1,5 @@
+package a;
+
+public abstract class A {
+  abstract String foo(); // package protected!
+}
\ No newline at end of file
diff --git a/test/files/neg/t3757/B.scala b/test/files/neg/t3757/B.scala
new file mode 100644
index 0000000000..7c78fb634e
--- /dev/null
+++ b/test/files/neg/t3757/B.scala
@@ -0,0 +1,5 @@
+package b
+
+class B extends a.A {
+  override def foo = "B"
+}
\ No newline at end of file