From 3cb01d6cbd549c11f71556ee7d8a592e354afd46 Mon Sep 17 00:00:00 2001
From: Dale Wijnand <dale.wijnand@gmail.com>
Date: Tue, 7 Mar 2023 13:52:50 +0000
Subject: [PATCH 1/4] Drop incorrect super accessor in trait subclass

When a trait extends a class and accesses a protected member (while in a
different package), we would emitted a super accessor and call that
instead.  That breaks the semantics because if the protected member is
overridden, that implementation won't be called, as the call is to the
super method instead.

In addition to that, the call to the super accessor was causing a false
positive error as we don't allow super calls to bind to vals.
---
 .../tools/dotc/transform/AccessProxies.scala  |  2 +-
 .../dotc/transform/ProtectedAccessors.scala   | 12 +++------
 .../tools/dotc/transform/SuperAccessors.scala | 26 +++++++------------
 tests/{neg => pos}/i11170a.scala              |  4 +--
 tests/run/i17021.defs.scala                   | 16 ++++++++++++
 tests/run/i17021.scala                        | 18 +++++++++++++
 6 files changed, 50 insertions(+), 28 deletions(-)
 rename tests/{neg => pos}/i11170a.scala (82%)
 create mode 100644 tests/run/i17021.defs.scala
 create mode 100644 tests/run/i17021.scala

diff --git a/compiler/src/dotty/tools/dotc/transform/AccessProxies.scala b/compiler/src/dotty/tools/dotc/transform/AccessProxies.scala
index 14362260d032..3175ffceae49 100644
--- a/compiler/src/dotty/tools/dotc/transform/AccessProxies.scala
+++ b/compiler/src/dotty/tools/dotc/transform/AccessProxies.scala
@@ -71,7 +71,7 @@ abstract class AccessProxies {
     def needsAccessor(sym: Symbol)(using Context): Boolean
 
     def ifNoHost(reference: RefTree)(using Context): Tree = {
-      assert(false, "no host found for $reference with ${reference.symbol.showLocated} from ${ctx.owner}")
+      assert(false, i"no host found for $reference with ${reference.symbol.showLocated} from ${ctx.owner}")
       reference
     }
 
diff --git a/compiler/src/dotty/tools/dotc/transform/ProtectedAccessors.scala b/compiler/src/dotty/tools/dotc/transform/ProtectedAccessors.scala
index 6d8f7bdb32cb..c675938b86f7 100644
--- a/compiler/src/dotty/tools/dotc/transform/ProtectedAccessors.scala
+++ b/compiler/src/dotty/tools/dotc/transform/ProtectedAccessors.scala
@@ -33,17 +33,11 @@ object ProtectedAccessors {
       ctx.owner.isContainedIn(boundary) || ctx.owner.isContainedIn(boundary.linkedClass)
     }
 
-  /** Do we need a protected accessor if the current context's owner
-   *  is not in a subclass or subtrait of `sym`?
-   */
-  def needsAccessorIfNotInSubclass(sym: Symbol)(using Context): Boolean =
-    sym.isTerm && sym.is(Protected) &&
-    !sym.owner.is(Trait) && // trait methods need to be handled specially, are currently always public
-    !insideBoundaryOf(sym)
-
   /** Do we need a protected accessor for accessing sym from the current context's owner? */
   def needsAccessor(sym: Symbol)(using Context): Boolean =
-    needsAccessorIfNotInSubclass(sym) &&
+    sym.isTerm && sym.is(Protected) &&
+    !sym.owner.is(Trait) && // trait methods need to be handled specially, are currently always public
+    !insideBoundaryOf(sym) &&
     !ctx.owner.enclosingClass.derivesFrom(sym.owner)
 }
 
diff --git a/compiler/src/dotty/tools/dotc/transform/SuperAccessors.scala b/compiler/src/dotty/tools/dotc/transform/SuperAccessors.scala
index 2307f759b571..5d7210a59a96 100644
--- a/compiler/src/dotty/tools/dotc/transform/SuperAccessors.scala
+++ b/compiler/src/dotty/tools/dotc/transform/SuperAccessors.scala
@@ -174,26 +174,20 @@ class SuperAccessors(thisPhase: DenotTransformer) {
     val sel @ Select(qual, name) = tree: @unchecked
     val sym = sel.symbol
 
-    /** If an accesses to protected member of a class comes from a trait,
-      *  or would need a protected accessor placed in a trait, we cannot
-      *  perform the access to the protected member directly since jvm access
-      *  restrictions require the call site to be in an actual subclass and
-      *  traits don't count as subclasses in this respect. In this case
-      *  we generate a super accessor instead. See SI-2296.
-      */
     def needsSuperAccessor =
-      ProtectedAccessors.needsAccessorIfNotInSubclass(sym) &&
+      ProtectedAccessors.needsAccessor(sym) &&
       AccessProxies.hostForAccessorOf(sym).is(Trait)
     qual match {
       case _: This if needsSuperAccessor =>
-        /*
-          * A trait which extends a class and accesses a protected member
-          *  of that class cannot implement the necessary accessor method
-          *  because jvm access restrictions require the call site to be in
-          *  an actual subclass and traits don't count as subclasses in this
-          *  respect. We generate a super accessor itself, which will be fixed
-          *  by the implementing class.  See SI-2296.
-          */
+        /* Given a protected member m defined in class C,
+         * and a trait T that calls m.
+         *
+         * If T extends C, then we can access it by casting
+         * the qualifier of the select to C.
+         *
+         * Otherwise, we need to go through an accessor,
+         * which the implementing class will provide an implementation for.
+         */
         superAccessorCall(sel)
       case Super(_, mix) =>
         transformSuperSelect(sel)
diff --git a/tests/neg/i11170a.scala b/tests/pos/i11170a.scala
similarity index 82%
rename from tests/neg/i11170a.scala
rename to tests/pos/i11170a.scala
index 5268c506f33f..bbf627ce8864 100644
--- a/tests/neg/i11170a.scala
+++ b/tests/pos/i11170a.scala
@@ -23,6 +23,6 @@ package cpackage {
   import apackage._
   import bpackage._
 
-  case class C(override protected val x: Int) extends A with B  // error
+  case class C(override protected val x: Int) extends A with B
   case class C2(override val x: Int) extends A2 with B2
-}
\ No newline at end of file
+}
diff --git a/tests/run/i17021.defs.scala b/tests/run/i17021.defs.scala
new file mode 100644
index 000000000000..126759b5d268
--- /dev/null
+++ b/tests/run/i17021.defs.scala
@@ -0,0 +1,16 @@
+// Derives from run/i17021, but with defs instead of vals
+package p1:
+  class A:
+    protected def foo: Int = 1
+
+package p2:
+  trait B extends p1.A:
+    def bar: Int = foo
+
+  class C extends B:
+    override def foo: Int = 2
+
+object Test:
+  def main(args: Array[String]): Unit =
+    val n = new p2.C().bar
+    assert(n == 2, n) // was: assertion failed: 1
diff --git a/tests/run/i17021.scala b/tests/run/i17021.scala
new file mode 100644
index 000000000000..7465508e7f0a
--- /dev/null
+++ b/tests/run/i17021.scala
@@ -0,0 +1,18 @@
+package p1:
+  class A:
+    protected val foo: Int = 1
+
+package p2:
+  trait B extends p1.A:
+    def bar: Int = foo
+
+  class C extends B: // was: error: parent trait B has a super call which binds to the value p1.A.foo. Super calls can only target methods.
+    override val foo: Int = 2
+
+// Also, assert that the access continues to delegate:
+// i.e. B#bar delegates to this.foo and so C#bar returns 2,
+// not B#bar delegates to super.foo and so C#bar returns 1.
+object Test:
+  def main(args: Array[String]): Unit =
+    val n = new p2.C().bar
+    assert(n == 2, n) // was: assertion failed: 1

From 121af881cf3f66d291cb4343330c905781c91dd9 Mon Sep 17 00:00:00 2001
From: Dale Wijnand <dale.wijnand@gmail.com>
Date: Mon, 13 Mar 2023 11:22:31 +0000
Subject: [PATCH 2/4] Add a Java protected member case, which can only call
 super

---
 .../tools/dotc/transform/ProtectedAccessors.scala  |  2 +-
 tests/run/i17021.ext-java/A.java                   |  6 ++++++
 tests/run/i17021.ext-java/Test.scala               | 14 ++++++++++++++
 3 files changed, 21 insertions(+), 1 deletion(-)
 create mode 100644 tests/run/i17021.ext-java/A.java
 create mode 100644 tests/run/i17021.ext-java/Test.scala

diff --git a/compiler/src/dotty/tools/dotc/transform/ProtectedAccessors.scala b/compiler/src/dotty/tools/dotc/transform/ProtectedAccessors.scala
index c675938b86f7..2b150d321812 100644
--- a/compiler/src/dotty/tools/dotc/transform/ProtectedAccessors.scala
+++ b/compiler/src/dotty/tools/dotc/transform/ProtectedAccessors.scala
@@ -38,7 +38,7 @@ object ProtectedAccessors {
     sym.isTerm && sym.is(Protected) &&
     !sym.owner.is(Trait) && // trait methods need to be handled specially, are currently always public
     !insideBoundaryOf(sym) &&
-    !ctx.owner.enclosingClass.derivesFrom(sym.owner)
+    (sym.is(JavaDefined) || !ctx.owner.enclosingClass.derivesFrom(sym.owner))
 }
 
 class ProtectedAccessors extends MiniPhase {
diff --git a/tests/run/i17021.ext-java/A.java b/tests/run/i17021.ext-java/A.java
new file mode 100644
index 000000000000..536e9caa4a38
--- /dev/null
+++ b/tests/run/i17021.ext-java/A.java
@@ -0,0 +1,6 @@
+// Derives from run/i17021.defs, but with a Java protected member
+package p1;
+
+public class A {
+  protected int foo() { return 1; }
+}
diff --git a/tests/run/i17021.ext-java/Test.scala b/tests/run/i17021.ext-java/Test.scala
new file mode 100644
index 000000000000..80dbca9fdd5d
--- /dev/null
+++ b/tests/run/i17021.ext-java/Test.scala
@@ -0,0 +1,14 @@
+// Derives from run/i17021.defs
+// but with a Java protected member
+// which changes the behaviour
+package p2:
+  trait B extends p1.A:
+    def bar: Int = foo
+
+  class C extends B:
+    override def foo: Int = 2
+
+object Test:
+  def main(args: Array[String]): Unit =
+    val n = new p2.C().bar
+    assert(n == 1, n) // B can only call super.foo

From 0dea6c0681d632f66689eae8ee126a1910cf7ecd Mon Sep 17 00:00:00 2001
From: Dale Wijnand <dale.wijnand@gmail.com>
Date: Tue, 14 Mar 2023 11:09:49 +0000
Subject: [PATCH 3/4] Fail the Java protected member case, rather than be wierd

---
 .../tools/dotc/transform/ProtectedAccessors.scala  | 14 ++++++++++----
 .../tools/dotc/transform/SuperAccessors.scala      | 13 +++++++++++--
 tests/neg/i17021.ext-java/A.java                   |  6 ++++++
 tests/neg/i17021.ext-java/Test.scala               | 14 ++++++++++++++
 tests/run/i17021.ext-java/Test.scala               |  6 +++---
 5 files changed, 44 insertions(+), 9 deletions(-)
 create mode 100644 tests/neg/i17021.ext-java/A.java
 create mode 100644 tests/neg/i17021.ext-java/Test.scala

diff --git a/compiler/src/dotty/tools/dotc/transform/ProtectedAccessors.scala b/compiler/src/dotty/tools/dotc/transform/ProtectedAccessors.scala
index 2b150d321812..6d8f7bdb32cb 100644
--- a/compiler/src/dotty/tools/dotc/transform/ProtectedAccessors.scala
+++ b/compiler/src/dotty/tools/dotc/transform/ProtectedAccessors.scala
@@ -33,12 +33,18 @@ object ProtectedAccessors {
       ctx.owner.isContainedIn(boundary) || ctx.owner.isContainedIn(boundary.linkedClass)
     }
 
-  /** Do we need a protected accessor for accessing sym from the current context's owner? */
-  def needsAccessor(sym: Symbol)(using Context): Boolean =
+  /** Do we need a protected accessor if the current context's owner
+   *  is not in a subclass or subtrait of `sym`?
+   */
+  def needsAccessorIfNotInSubclass(sym: Symbol)(using Context): Boolean =
     sym.isTerm && sym.is(Protected) &&
     !sym.owner.is(Trait) && // trait methods need to be handled specially, are currently always public
-    !insideBoundaryOf(sym) &&
-    (sym.is(JavaDefined) || !ctx.owner.enclosingClass.derivesFrom(sym.owner))
+    !insideBoundaryOf(sym)
+
+  /** Do we need a protected accessor for accessing sym from the current context's owner? */
+  def needsAccessor(sym: Symbol)(using Context): Boolean =
+    needsAccessorIfNotInSubclass(sym) &&
+    !ctx.owner.enclosingClass.derivesFrom(sym.owner)
 }
 
 class ProtectedAccessors extends MiniPhase {
diff --git a/compiler/src/dotty/tools/dotc/transform/SuperAccessors.scala b/compiler/src/dotty/tools/dotc/transform/SuperAccessors.scala
index 5d7210a59a96..b78c75d58340 100644
--- a/compiler/src/dotty/tools/dotc/transform/SuperAccessors.scala
+++ b/compiler/src/dotty/tools/dotc/transform/SuperAccessors.scala
@@ -175,7 +175,7 @@ class SuperAccessors(thisPhase: DenotTransformer) {
     val sym = sel.symbol
 
     def needsSuperAccessor =
-      ProtectedAccessors.needsAccessor(sym) &&
+      ProtectedAccessors.needsAccessorIfNotInSubclass(sym) &&
       AccessProxies.hostForAccessorOf(sym).is(Trait)
     qual match {
       case _: This if needsSuperAccessor =>
@@ -185,10 +185,19 @@ class SuperAccessors(thisPhase: DenotTransformer) {
          * If T extends C, then we can access it by casting
          * the qualifier of the select to C.
          *
+         * That's because the protected method is actually public,
+         * so we can call it.  For truly protected methods, like from
+         * Java, we error instead of emitting the wrong code (i17021.ext-java).
+         *
          * Otherwise, we need to go through an accessor,
          * which the implementing class will provide an implementation for.
          */
-        superAccessorCall(sel)
+        if ctx.owner.enclosingClass.derivesFrom(sym.owner) then
+          if sym.is(JavaDefined) then
+            report.error(em"${ctx.owner} accesses protected $sym inside a concrete trait method: use super.${sel.name} instead", sel.srcPos)
+          sel
+        else
+          superAccessorCall(sel)
       case Super(_, mix) =>
         transformSuperSelect(sel)
       case _ =>
diff --git a/tests/neg/i17021.ext-java/A.java b/tests/neg/i17021.ext-java/A.java
new file mode 100644
index 000000000000..536e9caa4a38
--- /dev/null
+++ b/tests/neg/i17021.ext-java/A.java
@@ -0,0 +1,6 @@
+// Derives from run/i17021.defs, but with a Java protected member
+package p1;
+
+public class A {
+  protected int foo() { return 1; }
+}
diff --git a/tests/neg/i17021.ext-java/Test.scala b/tests/neg/i17021.ext-java/Test.scala
new file mode 100644
index 000000000000..c700ed8138d7
--- /dev/null
+++ b/tests/neg/i17021.ext-java/Test.scala
@@ -0,0 +1,14 @@
+// Derives from run/i17021.defs
+// but with a Java protected member
+// which leads to a compile error
+package p2:
+  trait B extends p1.A:
+    def bar: Int = foo // error: method bar accesses protected method foo inside a concrete trait method: use super.foo instead
+
+  class C extends B:
+    override def foo: Int = 2
+
+object Test:
+  def main(args: Array[String]): Unit =
+    val n = new p2.C().bar
+    assert(n == 2, n)
diff --git a/tests/run/i17021.ext-java/Test.scala b/tests/run/i17021.ext-java/Test.scala
index 80dbca9fdd5d..3741fa2dfadb 100644
--- a/tests/run/i17021.ext-java/Test.scala
+++ b/tests/run/i17021.ext-java/Test.scala
@@ -1,9 +1,9 @@
 // Derives from run/i17021.defs
 // but with a Java protected member
-// which changes the behaviour
+// and fixed calling code, that uses super
 package p2:
   trait B extends p1.A:
-    def bar: Int = foo
+    def bar: Int = super.foo
 
   class C extends B:
     override def foo: Int = 2
@@ -11,4 +11,4 @@ package p2:
 object Test:
   def main(args: Array[String]): Unit =
     val n = new p2.C().bar
-    assert(n == 1, n) // B can only call super.foo
+    assert(n == 1, n)

From f8fb548300850aebaece5c15a13632289a563c67 Mon Sep 17 00:00:00 2001
From: Dale Wijnand <dale.wijnand@gmail.com>
Date: Thu, 23 Mar 2023 10:55:28 +0100
Subject: [PATCH 4/4] Skip Java using run test when scalajs

---
 compiler/test/dotty/tools/vulpix/ParallelTesting.scala | 7 +++++--
 tests/run/i17021.ext-java/Test.scala                   | 1 +
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/compiler/test/dotty/tools/vulpix/ParallelTesting.scala b/compiler/test/dotty/tools/vulpix/ParallelTesting.scala
index 62b1b88984bc..3799a2335a78 100644
--- a/compiler/test/dotty/tools/vulpix/ParallelTesting.scala
+++ b/compiler/test/dotty/tools/vulpix/ParallelTesting.scala
@@ -751,8 +751,11 @@ trait ParallelTesting extends RunnerOrchestration { self =>
           case _ =>
         }
         case Failure(output) =>
-          echo(s"Test '${testSource.title}' failed with output:")
-          echo(output)
+          if output == "" then
+            echo(s"Test '${testSource.title}' failed with no output")
+          else
+            echo(s"Test '${testSource.title}' failed with output:")
+            echo(output)
           failTestSource(testSource)
         case Timeout =>
           echo("failed because test " + testSource.title + " timed out")
diff --git a/tests/run/i17021.ext-java/Test.scala b/tests/run/i17021.ext-java/Test.scala
index 3741fa2dfadb..2cca286c3801 100644
--- a/tests/run/i17021.ext-java/Test.scala
+++ b/tests/run/i17021.ext-java/Test.scala
@@ -1,3 +1,4 @@
+// scalajs: --skip
 // Derives from run/i17021.defs
 // but with a Java protected member
 // and fixed calling code, that uses super