From 22da3636fd531964182c4079e0e17faf2c0f38c8 Mon Sep 17 00:00:00 2001 From: Paul Phillips Date: Tue, 26 Oct 2010 04:37:09 +0000 Subject: A modifier's work is never done. 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. --- src/compiler/scala/tools/nsc/symtab/Symbols.scala | 4 +- .../scala/tools/nsc/typechecker/RefChecks.scala | 86 +++++++++++++--------- test/files/neg/java-access-neg.check | 16 ++++ test/files/neg/java-access-neg/J.java | 15 ++++ test/files/neg/java-access-neg/S2.scala | 61 +++++++++++++++ test/files/pos/java-access-pos/J.java | 15 ++++ test/files/pos/java-access-pos/S1.scala | 67 +++++++++++++++++ test/pending/neg/t3633/test/PackageProtected.java | 5 -- 8 files changed, 228 insertions(+), 41 deletions(-) create mode 100644 test/files/neg/java-access-neg.check create mode 100644 test/files/neg/java-access-neg/J.java create mode 100644 test/files/neg/java-access-neg/S2.scala create mode 100644 test/files/pos/java-access-pos/J.java create mode 100644 test/files/pos/java-access-pos/S1.scala delete mode 100644 test/pending/neg/t3633/test/PackageProtected.java diff --git a/src/compiler/scala/tools/nsc/symtab/Symbols.scala b/src/compiler/scala/tools/nsc/symtab/Symbols.scala index 18c34eb036..7092b8272c 100644 --- a/src/compiler/scala/tools/nsc/symtab/Symbols.scala +++ b/src/compiler/scala/tools/nsc/symtab/Symbols.scala @@ -670,10 +670,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 (hasAccessBoundary && !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 9fd1c69ef7..bc16d7c377 100644 --- a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala +++ b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala @@ -254,13 +254,15 @@ abstract class RefChecks extends InfoTransform { tp1 <:< tp2 } - /** Check that all conditions for overriding other by - * member of class clazz 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+ @@ -274,13 +276,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) @@ -319,23 +321,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"); @@ -555,24 +553,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 diff --git a/test/pending/neg/t3633/test/PackageProtected.java b/test/pending/neg/t3633/test/PackageProtected.java deleted file mode 100644 index f4535a55b4..0000000000 --- a/test/pending/neg/t3633/test/PackageProtected.java +++ /dev/null @@ -1,5 +0,0 @@ -package test; - -class PackageProtected { - int foo; -} -- cgit v1.2.3