Merged revisions 23355 via svnmerge from

https://lampsvn.epfl.ch/svn-repos/scala/scala/trunk

........
  r23355 | extempore | 2010-10-26 06:37:09 +0200 (Tue, 26 Oct 2010) | 4 lines

  A modifier's work is never done. Another mortal embrace with protected
and its bevy of corner cases. Closes #3939, #3947. This patch is
intended for both trunk and 2.8.1. Already reviewed and co-authored by
moors, and review by oderksy. ........

7 files changed, 228 insertions, 36 deletions

diff --git a/src/compiler/scala/tools/nsc/symtab/Symbols.scala b/src/compiler/scala/tools/nsc/symtab/Symbols.scala
index 8d02a120d3..3fabf766f8 100644
--- a/src/compiler/scala/tools/nsc/symtab/Symbols.scala
+++ b/src/compiler/scala/tools/nsc/symtab/Symbols.scala
@@ -674,10 +674,10 @@ trait Symbols extends reflect.generic.Symbols { self: SymbolTable =>
     final def resetFlags { rawflags = rawflags & TopLevelCreationFlags }
 
     /** The class or term up to which this symbol is accessible,
-     *  or RootClass if it is public
+     *  or RootClass if it is public.
      */
     def accessBoundary(base: Symbol): Symbol = {
-      if (hasFlag(PRIVATE) || owner.isTerm) owner
+      if (hasFlag(PRIVATE) || isLocal) owner
       else if (privateWithin != NoSymbol && !phase.erasedTypes) privateWithin
       else if (hasFlag(PROTECTED)) base
       else RootClass
diff --git a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
index 5c448349cb..7b533fd5e1 100644
--- a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
@@ -241,13 +241,15 @@ abstract class RefChecks extends InfoTransform {
           tp1 <:< tp2
       }
 
-      /** Check that all conditions for overriding <code>other</code> by
-       *  <code>member</code> of class <code>clazz</code> are met.
+      /** Check that all conditions for overriding `other` by `member`
+       *  of class `clazz` are met.
        */
       def checkOverride(clazz: Symbol, member: Symbol, other: Symbol) {
+        def noErrorType = other.tpe != ErrorType && member.tpe != ErrorType
+        def isRootOrNone(sym: Symbol) = sym == RootClass || sym == NoSymbol
 
         def overrideError(msg: String) {
-          if (other.tpe != ErrorType && member.tpe != ErrorType) {
+          if (noErrorType) {
             val fullmsg =
               "overriding "+infoStringWithLocation(other)+";\n "+
               infoString(member)+" "+msg+
@@ -261,13 +263,13 @@ abstract class RefChecks extends InfoTransform {
         }
 
         def overrideTypeError() {
-          if (other.tpe != ErrorType && member.tpe != ErrorType) {
+          if (noErrorType) {
             overrideError("has incompatible type")
           }
         }
 
         def accessFlagsToString(sym: Symbol)
-          = flagsToString(sym getFlag (PRIVATE | PROTECTED), if (sym.privateWithin eq NoSymbol) "" else sym.privateWithin.name.toString)
+          = flagsToString(sym getFlag (PRIVATE | PROTECTED), if (!sym.hasAccessBoundary) "" else sym.privateWithin.name.toString)
 
         def overrideAccessError() {
           val otherAccess = accessFlagsToString(other)
@@ -306,23 +308,19 @@ abstract class RefChecks extends InfoTransform {
           // ^-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)
+          if (member.isPrivate) // (1.1)
             overrideError("has weaker access privileges; it should not be private")
 
-          @inline def definedIn(sym: Symbol, in: Symbol) = sym != RootClass && sym != NoSymbol && sym.hasTransOwner(in)
-
+          // todo: align accessibility implication checking with isAccessible in Contexts
           val ob = other.accessBoundary(member.owner)
           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()
+          def isOverrideAccessOK = member.isPublic || {     // member is public, definitely same or relaxed access
+            (!other.isProtected || member.isProtected) &&   // if o is protected, so is m
+            (!isRootOrNone(ob) && ob.hasTransOwner(mb))     // m relaxes o's access boundary
           }
-          else if (other.isClass || other.isModule) {
+          if (!isOverrideAccessOK) {
+            overrideAccessError()
+          } else if (other.isClass || other.isModule) {
             overrideError("cannot be used here - classes and objects cannot be overridden");
           } else if (!other.isDeferred && (member.isClass || member.isModule)) {
             overrideError("cannot be used here - classes and objects can only override abstract types");
@@ -542,24 +540,44 @@ abstract class RefChecks extends InfoTransform {
        *  (which must be different from `clazz`) whose name and type
        *  seen as a member of `class.thisType` matches `member`'s.
        */
-      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 || {
-              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
+      def hasMatchingSym(inclazz: Symbol, member: Symbol): Boolean = {
+        val isVarargs = hasRepeatedParam(member.tpe)
+        lazy val varargsType = toJavaRepeatedParam(member.tpe)
+
+        def isSignatureMatch(sym: Symbol) = !sym.isTerm || {
+          val symtpe            = clazz.thisType memberType sym
+          def matches(tp: Type) = tp matches symtpe
+
+          matches(member.tpe) || (isVarargs && matches(varargsType))
         }
+        /** The rules for accessing members which have an access boundary are more
+         *  restrictive in java than scala.  Since java has no concept of package nesting,
+         *  a member with "default" (package-level) access can only be accessed by members
+         *  in the exact same package.  Example:
+         *
+         *    package a.b;
+         *    public class JavaClass { void foo() { } }
+         *
+         *  The member foo() can be accessed only from members of package a.b, and not
+         *  nested packages like a.b.c.  In the analogous scala class:
+         *
+         *    package a.b
+         *    class ScalaClass { private[b] def foo() = () }
+         *
+         *  The member IS accessible to classes in package a.b.c.  The javaAccessCheck logic
+         *  is restricting the set of matching signatures according to the above semantics.
+         */
+        def javaAccessCheck(sym: Symbol) = (
+             !inclazz.isJavaDefined                             // not a java defined member
+          || !sym.hasAccessBoundary                             // no access boundary
+          || sym.isProtected                                    // marked protected in java, thus accessible to subclasses
+          || sym.privateWithin == member.enclosingPackageClass  // exact package match
+        )
+        def classDecls   = inclazz.info.nonPrivateDecl(member.name)
+        def matchingSyms = classDecls filter (sym => isSignatureMatch(sym) && javaAccessCheck(sym))
+
+        (inclazz != clazz) && (matchingSyms != NoSymbol)
+      }
 
       // 4. Check that every defined member with an `override' modifier overrides some other member.
       for (member <- clazz.info.decls.toList)
diff --git a/test/files/neg/java-access-neg.check b/test/files/neg/java-access-neg.check
new file mode 100644
index 0000000000..af2812b579
--- /dev/null
+++ b/test/files/neg/java-access-neg.check
@@ -0,0 +1,16 @@
+S2.scala:12: error: method packageAbstract overrides nothing
+  override private[b] def packageAbstract() = ()      // fail
+                          ^
+S2.scala:16: error: method packageConcrete overrides nothing
+  override private[b] def packageConcrete() = ()      // fail
+                          ^
+S2.scala:36: error: method packageConcrete overrides nothing
+  override protected[b] def packageConcrete() = ()    // fail
+                            ^
+S2.scala:47: error: method packageConcrete overrides nothing
+  override private[a] def packageConcrete() = ()        // fail
+                          ^
+S2.scala:58: error: method packageConcrete overrides nothing
+  override def packageConcrete() = ()   // fail
+               ^
+5 errors found
diff --git a/test/files/neg/java-access-neg/J.java b/test/files/neg/java-access-neg/J.java
new file mode 100644
index 0000000000..b6bc3363a1
--- /dev/null
+++ b/test/files/neg/java-access-neg/J.java
@@ -0,0 +1,15 @@
+package a.b;
+
+public abstract class J {
+  public J() { }
+  J(int x1) { }
+  protected J(int x1, int x2) { }
+
+  abstract void packageAbstract();
+  protected abstract void protectedAbstract();
+  public abstract void publicAbstract();
+
+  void packageConcrete() { return; }
+  protected void protectedConcrete() { return; }
+  public void publicConcrete() { return; }
+}
diff --git a/test/files/neg/java-access-neg/S2.scala b/test/files/neg/java-access-neg/S2.scala
new file mode 100644
index 0000000000..b082bb7174
--- /dev/null
+++ b/test/files/neg/java-access-neg/S2.scala
@@ -0,0 +1,61 @@
+package a.b
+package c
+
+import a.b.J
+
+/** Variations of java-access-pos with us in a nested package.
+ */
+
+/** Declaring "override" all the time.
+ */
+class S1 extends J {
+  override private[b] def packageAbstract() = ()      // fail
+  override protected[b] def protectedAbstract() = ()
+  override def publicAbstract() = ()
+
+  override private[b] def packageConcrete() = ()      // fail
+  override protected[b] def protectedConcrete() = ()
+  override def publicConcrete() = ()
+}
+
+/** Implementing abstracts.
+ */
+class S2 extends J {
+  private[b] def packageAbstract() = ()     // fail
+  protected[b] def protectedAbstract() = ()
+  def publicAbstract() = ()
+}
+
+/** Widening access.
+ */
+class S3 extends J {
+  protected[b] def packageAbstract() = ()   // fail
+  protected[b] def protectedAbstract() = ()
+  def publicAbstract() = ()
+
+  override protected[b] def packageConcrete() = ()    // fail
+  override protected[b] def protectedConcrete() = ()
+  override def publicConcrete() = ()
+}
+/** More widening.
+ */
+class S4 extends J {
+  private[a] def packageAbstract() = ()         // fail
+  protected[a] def protectedAbstract() = ()
+  def publicAbstract() = ()
+
+  override private[a] def packageConcrete() = ()        // fail
+  override protected[a] def protectedConcrete() = ()
+  override def publicConcrete() = ()
+}
+/** Yet more widening.
+ */
+class S5 extends J {
+  def packageAbstract() = ()      // fail
+  def protectedAbstract() = ()
+  def publicAbstract() = ()
+
+  override def packageConcrete() = ()   // fail
+  override def protectedConcrete() = ()
+  override def publicConcrete() = ()
+}
diff --git a/test/files/pos/java-access-pos/J.java b/test/files/pos/java-access-pos/J.java
new file mode 100644
index 0000000000..b6bc3363a1
--- /dev/null
+++ b/test/files/pos/java-access-pos/J.java
@@ -0,0 +1,15 @@
+package a.b;
+
+public abstract class J {
+  public J() { }
+  J(int x1) { }
+  protected J(int x1, int x2) { }
+
+  abstract void packageAbstract();
+  protected abstract void protectedAbstract();
+  public abstract void publicAbstract();
+
+  void packageConcrete() { return; }
+  protected void protectedConcrete() { return; }
+  public void publicConcrete() { return; }
+}
diff --git a/test/files/pos/java-access-pos/S1.scala b/test/files/pos/java-access-pos/S1.scala
new file mode 100644
index 0000000000..10730e3a70
--- /dev/null
+++ b/test/files/pos/java-access-pos/S1.scala
@@ -0,0 +1,67 @@
+package a.b
+
+/** Declaring "override" all the time.
+ */
+class S1 extends J {
+  override private[b] def packageAbstract() = ()
+  override protected[b] def protectedAbstract() = ()
+  override def publicAbstract() = ()
+
+  override private[b] def packageConcrete() = ()
+  override protected[b] def protectedConcrete() = ()
+  override def publicConcrete() = ()
+}
+
+/** Implementing abstracts.
+ */
+class S2 extends J {
+  private[b] def packageAbstract() = ()
+  protected[b] def protectedAbstract() = ()
+  def publicAbstract() = ()
+}
+
+/** Widening access.
+ */
+class S3 extends J {
+  protected[b] def packageAbstract() = ()
+  protected[b] def protectedAbstract() = ()
+  def publicAbstract() = ()
+
+  override protected[b] def packageConcrete() = ()
+  override protected[b] def protectedConcrete() = ()
+  override def publicConcrete() = ()
+}
+/** More widening.
+ */
+class S4 extends J {
+  private[a] def packageAbstract() = ()
+  protected[a] def protectedAbstract() = ()
+  def publicAbstract() = ()
+
+  override private[a] def packageConcrete() = ()
+  override protected[a] def protectedConcrete() = ()
+  override def publicConcrete() = ()
+}
+/** Yet more widening.
+ */
+class S5 extends J {
+  def packageAbstract() = ()
+  def protectedAbstract() = ()
+  def publicAbstract() = ()
+
+  override def packageConcrete() = ()
+  override def protectedConcrete() = ()
+  override def publicConcrete() = ()
+}
+/** Constructors.
+ */
+class S6 extends J(1) {
+  def packageAbstract() = ()
+  def protectedAbstract() = ()
+  def publicAbstract() = ()
+}
+class S7 extends J(1, 2) {
+  def packageAbstract() = ()
+  def protectedAbstract() = ()
+  def publicAbstract() = ()
+}
\ No newline at end of file