From 9d12f0163828eebcf19275fea25ebc50db1445da Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Tue, 14 Jul 2020 15:12:48 +0200 Subject: [PATCH 1/3] Erasure#typedSelect: Handle inaccessible qualifier Previously, the code would have gone into an infinite loop, instead we now emit an error like in Scala 2. --- .../src/dotty/tools/dotc/transform/Erasure.scala | 14 ++++++++++++-- tests/neg/java-trait-access/A.java | 5 +++++ tests/neg/java-trait-access/B.scala | 13 +++++++++++++ 3 files changed, 30 insertions(+), 2 deletions(-) create mode 100644 tests/neg/java-trait-access/A.java create mode 100644 tests/neg/java-trait-access/B.scala diff --git a/compiler/src/dotty/tools/dotc/transform/Erasure.scala b/compiler/src/dotty/tools/dotc/transform/Erasure.scala index 3fc92aa5e4ca..d53690a6cdcb 100644 --- a/compiler/src/dotty/tools/dotc/transform/Erasure.scala +++ b/compiler/src/dotty/tools/dotc/transform/Erasure.scala @@ -20,6 +20,7 @@ import core.Annotations.BodyAnnotation import typer.{NoChecking, LiftErased} import typer.Inliner import typer.ProtoTypes._ +import typer.ErrorReporting.errorTree import core.TypeErasure._ import core.Decorators._ import dotty.tools.dotc.ast.{tpd, untpd} @@ -700,8 +701,17 @@ object Erasure { else val castTarget = // Avoid inaccessible cast targets, see i8661 if sym.owner.isAccessibleFrom(qual1.tpe)(using preErasureCtx) - then sym.owner.typeRef - else erasure(tree.qualifier.typeOpt.widen) + then + sym.owner.typeRef + else + // If the owner is inaccessible, try going through the qualifier, + // but be careful to not go in an infinite loop in case that doesn't + // work either. + val tp = erasure(tree.qualifier.typeOpt.widen) + if tp =:= qual1.tpe.widen then + return errorTree(qual1, + ex"Unable to emit reference to ${sym.showLocated}, ${sym.owner} is not accessible in ${ctx.owner.enclosingClass}") + tp recur(cast(qual1, castTarget)) } } diff --git a/tests/neg/java-trait-access/A.java b/tests/neg/java-trait-access/A.java new file mode 100644 index 000000000000..827a1a0c4a8e --- /dev/null +++ b/tests/neg/java-trait-access/A.java @@ -0,0 +1,5 @@ +package pkg; + +class A { + public void foo() {} +} diff --git a/tests/neg/java-trait-access/B.scala b/tests/neg/java-trait-access/B.scala new file mode 100644 index 000000000000..9cdfbdcaffd7 --- /dev/null +++ b/tests/neg/java-trait-access/B.scala @@ -0,0 +1,13 @@ +package pkg { + trait B extends A + class C extends B +} + +object Test { + def test1: Unit = { + val c = new pkg.C + c.foo() // OK + val b: pkg.B = c + b.foo() // error: Unable to emit reference to method foo in class A, class A is not accessible in object Test + } +} From aeafe2da55a1215627e0f0b1b32e1fc5fc826dd9 Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Tue, 14 Jul 2020 15:43:35 +0200 Subject: [PATCH 2/3] Erasure#typedSelect: Use correct type as cast target The missing `.finalResultType` meant we could end up trying to cast to a MethodType. --- compiler/src/dotty/tools/dotc/transform/Erasure.scala | 8 ++++++-- tests/neg/java-trait-access/B.scala | 5 +++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/transform/Erasure.scala b/compiler/src/dotty/tools/dotc/transform/Erasure.scala index d53690a6cdcb..bc6b04ed4baf 100644 --- a/compiler/src/dotty/tools/dotc/transform/Erasure.scala +++ b/compiler/src/dotty/tools/dotc/transform/Erasure.scala @@ -688,12 +688,16 @@ object Erasure { def recur(qual: Tree): Tree = { val qualIsPrimitive = qual.tpe.widen.isPrimitiveValueType val symIsPrimitive = sym.owner.isPrimitiveValueClass + + def originalQual: Type = + erasure(tree.qualifier.typeOpt.widen.finalResultType) + if (qualIsPrimitive && !symIsPrimitive || qual.tpe.widenDealias.isErasedValueType) recur(box(qual)) else if (!qualIsPrimitive && symIsPrimitive) recur(unbox(qual, sym.owner.typeRef)) else if (sym.owner eq defn.ArrayClass) - selectArrayMember(qual, erasure(tree.qualifier.typeOpt.widen.finalResultType)) + selectArrayMember(qual, originalQual) else { val qual1 = adaptIfSuper(qual) if (qual1.tpe.derivesFrom(sym.owner) || qual1.isInstanceOf[Super]) @@ -707,7 +711,7 @@ object Erasure { // If the owner is inaccessible, try going through the qualifier, // but be careful to not go in an infinite loop in case that doesn't // work either. - val tp = erasure(tree.qualifier.typeOpt.widen) + val tp = originalQual if tp =:= qual1.tpe.widen then return errorTree(qual1, ex"Unable to emit reference to ${sym.showLocated}, ${sym.owner} is not accessible in ${ctx.owner.enclosingClass}") diff --git a/tests/neg/java-trait-access/B.scala b/tests/neg/java-trait-access/B.scala index 9cdfbdcaffd7..4c932888f124 100644 --- a/tests/neg/java-trait-access/B.scala +++ b/tests/neg/java-trait-access/B.scala @@ -10,4 +10,9 @@ object Test { val b: pkg.B = c b.foo() // error: Unable to emit reference to method foo in class A, class A is not accessible in object Test } + + val c2 = new pkg.C + c2.foo() // OK + val b2: pkg.B = c2 + b2.foo() // error: Unable to emit reference to method foo in class A, class A is not accessible in object Test } From d3ce551b3a74c03cb28a1c77cd6ed1b0517bcb6a Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Tue, 14 Jul 2020 15:34:06 +0200 Subject: [PATCH 3/3] Erasure#typedSelect: Proper accessibility checking We only need to avoid things which are inaccessible from the JVM point of view, so `c.a` should typecheck because `protected[foo] trait A` will be emitted as a public interface. --- .../dotty/tools/dotc/transform/Erasure.scala | 17 ++++++++++++++++- tests/pos/trait-access.scala | 13 +++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 tests/pos/trait-access.scala diff --git a/compiler/src/dotty/tools/dotc/transform/Erasure.scala b/compiler/src/dotty/tools/dotc/transform/Erasure.scala index bc6b04ed4baf..5fa75d7633d6 100644 --- a/compiler/src/dotty/tools/dotc/transform/Erasure.scala +++ b/compiler/src/dotty/tools/dotc/transform/Erasure.scala @@ -685,6 +685,21 @@ object Erasure { qual } + /** Can we safely use `cls` as a qualifier without getting a runtime error on + * the JVM due to its accessibility checks? + */ + def isJvmAccessible(cls: Symbol): Boolean = + // Scala classes are always emitted as public, unless the + // `private` modifier is used, but a non-private class can never + // extend a private class, so such a class will never be a cast target. + !cls.is(Flags.JavaDefined) || { + // We can't rely on `isContainedWith` here because packages are + // not nested from the JVM point of view. + val boundary = cls.accessBoundary(cls.owner)(using preErasureCtx) + (boundary eq defn.RootClass) || + (ctx.owner.enclosingPackageClass eq boundary) + } + def recur(qual: Tree): Tree = { val qualIsPrimitive = qual.tpe.widen.isPrimitiveValueType val symIsPrimitive = sym.owner.isPrimitiveValueClass @@ -704,7 +719,7 @@ object Erasure { select(qual1, sym) else val castTarget = // Avoid inaccessible cast targets, see i8661 - if sym.owner.isAccessibleFrom(qual1.tpe)(using preErasureCtx) + if isJvmAccessible(sym.owner) then sym.owner.typeRef else diff --git a/tests/pos/trait-access.scala b/tests/pos/trait-access.scala new file mode 100644 index 000000000000..3498d1f0f58d --- /dev/null +++ b/tests/pos/trait-access.scala @@ -0,0 +1,13 @@ +package foo { + protected[foo] trait A { + def a: Unit = {} + } + class B extends A +} +trait C extends foo.B +object Test { + def test: Unit = { + val c = new C {} + c.a + } +}