Two fixes for mixing in Java interfaces that refine a member's type #9615

lrytz · 2021-05-06T12:17:44Z

The first commit refines #9525 to skip access boundry checks between two Java members.

This PR fixes two bugs when mixing in a Java interface that refines the type of a member

The override check was incorrectly skipped, allowing the concrete method to have a weaker type than requried by the interface
For correct types, the generated bridge method AST didn't have a Type assigned, leading to an NPE

Reverts the second commit of #8643

Follow-up in #9629

PR 9525 added access boundary checks when overriding a protected Java member. This check should only be done if the overriding member is defined in Scala, not if the (Scala) class inherits two members both defined in Java.

The shortcut that's removed here was added in PR 8643 but turned out to be incorrect. The bug fixed by that PR remains fixed, because the shortcut was only added as a "second layer", the underlying bug was fixed as well. Note that RefChecks only runs on Scala classes, so we only perform override checks of two Java-defined members if a Scala class inherits them. These checks are not optional, as shown by the test cases added here.

The Select and This trees generated for bridge methods before only had a Symbol assigned, but no Type. This lead to an NPE in the attached test case.

smarter · 2021-05-06T17:00:54Z

src/compiler/scala/tools/nsc/transform/OverridingPairs.scala

@@ -53,9 +53,6 @@ abstract class OverridingPairs extends SymbolPairs {
      && !exclude(low)                 // this admits private, as one can't have a private member that matches a less-private member.
      && (lowMemberType matches (self memberType high))
    ) // TODO we don't call exclude(high), should we?
-
-    override def skipOwnerPair(lowClass: Symbol, highClass: Symbol): Boolean =
-      lowClass.isJavaDefined && highClass.isJavaDefined // javac is already checking this better than we could


In Dotty I ended up with a similar check but restricted to the case where both symbols are actual classes and not interfaces: https://github.com/lampepfl/dotty/blob/69c800b22c0ae6c5eb6fa6e7c2142c18a8278ff1/compiler/src/dotty/tools/dotc/typer/RefChecks.scala#L506 since one can't extend two Java classes without one extending the other, but I'm not sure if it's completely correct either.

lrytz · 2021-05-06T18:58:05Z

@SethTisue if we see any overriding checking errors in the community build, this PR will likely be the cause.

SethTisue · 2021-05-11T17:52:27Z

@lrytz https://scala-ci.typesafe.com/job/scala-2.13.x-jdk11-integrate-community-build/2725/artifact/logs/sttp-build.log

[sttp] [error] /home/jenkins/workspace/scala-2.13.x-jdk11-integrate-community-build/target-0.9.17/project-builds/sttp-7e54fbfc42d10ff169ef66b31619abe9bb140930/armeria-backend/src/main/scala/sttp/client3/armeria/HttpDecodingClient.scala:14:13: cannot override final member:
[sttp] [error] final def as[U](type: Class[U]): U (defined in class AbstractUnwrappable)
[sttp] [error]   with <defaultmethod> def as[T](type: Class[T]): T (defined in trait Client);
[sttp] [error]  other members with override errors are: unwrap
[sttp] [error] final class HttpDecodingClient(delegate: HttpClient) extends SimpleDecoratingHttpClient(delegate) {
[sttp] [error]             ^

the problem is reproducible locally outside of dbuild if you publishLocal kind-projector for the Scala version you want to test; that's how I bisected it. (I didn't continue bisecting to discover which of the 3 commits in this PR was the culprit.)

sttp commit tested is softwaremill/sttp@d85db52 and the command to test is armeriaBackend/compile

lrytz · 2021-05-12T11:40:33Z

Thanks Seth. See scala/bug#12394 and #9629.

lrytz requested a review from smarter May 6, 2021 12:17

scala-jenkins added this to the 2.13.6 milestone May 6, 2021

This comment has been minimized.

Sign in to view

lrytz added 3 commits May 6, 2021 15:38

No access boundary check between two protected Java members

e4a11fa

PR 9525 added access boundary checks when overriding a protected Java member. This check should only be done if the overriding member is defined in Scala, not if the (Scala) class inherits two members both defined in Java.

Generate attributed trees when creating bridge methods

5ad3b05

The Select and This trees generated for bridge methods before only had a Symbol assigned, but no Type. This lead to an NPE in the attached test case.

lrytz force-pushed the t12380 branch from f5adf52 to 5ad3b05 Compare May 6, 2021 13:39

smarter approved these changes May 6, 2021

View reviewed changes

lrytz merged commit 45b129e into scala:2.13.x May 6, 2021

SethTisue added the java interop label May 6, 2021

lrytz mentioned this pull request May 12, 2021

Too much vs too little override checking for Java-defined inherited members scala/bug#12394

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Two fixes for mixing in Java interfaces that refine a member's type #9615

Two fixes for mixing in Java interfaces that refine a member's type #9615

lrytz commented May 6, 2021 •

edited

This comment has been minimized.

smarter May 6, 2021

lrytz commented May 6, 2021

SethTisue commented May 11, 2021 •

edited

lrytz commented May 12, 2021

Two fixes for mixing in Java interfaces that refine a member's type #9615

Two fixes for mixing in Java interfaces that refine a member's type #9615

Conversation

lrytz commented May 6, 2021 • edited

This comment has been minimized.

smarter May 6, 2021

Choose a reason for hiding this comment

lrytz commented May 6, 2021

SethTisue commented May 11, 2021 • edited

lrytz commented May 12, 2021

lrytz commented May 6, 2021 •

edited

SethTisue commented May 11, 2021 •

edited