SI-9111 / SI-10107 resolution of static references in Java code #5606

lrytz · 2016-12-19T21:11:00Z

Scala puts static members of Java classes into the companion
object symbol. When parsing Java code, references to static
members need to be looked up in these companions.

In 2.11.x, a synthetic import A._ was added to Java classes
having static members, which lead to some bugs (SI-9111). The
scheme was changed in 4e822d7 (2.12.x only): when a lookup
of T / A.T fails, the typer searches in the companion of the
current class.

This fixes some issues, but not all

In Java, A.T can refer to a static member defined in a
parent of A
If a static A.T exists, but a parent of A defines a
non-static member class T, the non-static one would be
chosen (wrongly)

This commit pushes the logic to consider companion objects into
FindMembers and makes sure that for ever class in the base type
sequence, the companion is considered before moving to the next
base type.

This version changes the mode for mixed compilation, which used to be 1. scalac *.java *.scala -d o 2. javac *.java -d o -cp o 3. scalac *.scala -d o -cp o Now the third step is skipped. This required some adjustments to existing tests. - t7014 is split in two groups, the fix is for separate compilation. - t7582 is also split. It tests inliner warnings when inling code that accesses Java-defined package-private code. Inlining from Java only works in separate compilation (no bytecode available in mixed compilation). - Java compiler warnings of "run" tests were not reported in the old scheme, now they are. Deprecation / unchecked warnings were removed from t6240, t8786, varargs. - t4788 required a .check file update to pass, which hints at a bug. I will re-open SI-4788 and investigate later.

The logic to resolve static nested classes changed 4e822d7, so the description in the bug report (wrong imports) is no longer accurate. The first example in the ticket is fixed, but not the second. This commit adds tests for the status quo. Removes a method that's no longer used since 4e822d7.

adriaanm

Great! I have a few small suggestions.

adriaanm · 2016-12-21T01:04:06Z

src/compiler/scala/tools/nsc/typechecker/Contexts.scala

+        val searchCompanions = isPlainClassInJavaUnit(pre.typeSymbol)
+        val sym = pre.findMember(name, excludedFlags = Flags.BridgeFlags, requiredFlags = 0, stableOnly = false, searchCompanions).filter(qualifies)
+        if (searchCompanions && sym.owner.isModuleClass)
+          pre = sym.owner.typeOfThis


Much neater than companionSymbolOf!

Could we ever get here while completing a lazy type for sym.owner? Maybe that's not possible because of the Java-specific code path here.

sym is a member, and sym.owner is a module class. I think when completing a lazy type of a class, its members' types are not completed.

adriaanm · 2016-12-21T01:08:17Z

src/compiler/scala/tools/nsc/typechecker/Contexts.scala

@@ -984,6 +985,10 @@ trait Contexts { self: Analyzer =>

    def isNameInScope(name: Name) = lookupSymbol(name, _ => true).isSuccess

+    def isPlainClassInJavaUnit(sym: Symbol): Boolean =


Would it be possible/make sense to get the unit from sym, so that we can encapsulate this logic in findMember instead of computing searchCompanions at its call sites?

There are many uses of findMember other than the ones updated in this PR. This PR only searches in companions when type-checking Idents or Selects while type-checking a Java source file. For example, given

public class A { public static class T } public class B extends A

A reference B.T resolves to A.T when it appears inside a Java source file, but should (?) give an error when inside a Scala source file. So we need to check context.unit.isJava, not only sym.isJavaDefined.

There's no compilationUnit on symbols, only sourceFile, but that's says where a symbol is defined. The context.unit.isJava check is about where the symbol is referenced.

How about:

class HasStatic { static void foo() {} } class Client { void test() { ScalaSubclass.foo(); } }

class ScalaSubclass extends HasStatic

This shows that a Java client can access a Java static selected from a Scala subclass of the owner of the static member.

I think we should drop the check for sym.isJavaDefined here, and instead only use this to filter out companions in FindMember.

I agree with your point of moving the sym.isJavaDefined check into FindMember. However, your example is not going to work, and it will be very hard to support it:

// A.java public class A { public static class C { public static class I { } } public static class E extends D.I { } }

// Test.scala class D extends A.C object Test { new A.E() }

In mixed compilation:

A.java:5: error: not found: value D public static class E extends D.I { } ^ one error found

It works if D has a companion object. It also works in separate compilation.

Thinking more about it, we should support D.I in a Java source even if D is defined in Scala and not a value. This would be very similar as the special case being implemented in this PR (allow A.T if A is a java class).

To make it a little trickier:

// Java public class A { public static class I { } } // Scala class B { // class I // (1) } class C extends A object C extends B { // class I // (2) }

What is C.I in a Java source? In separate compilation (mixed compilation should behave the same):

With 1 and 2 commented, C.I (in a Java source) resolves to A$I

Un-commenting 1 doesn't change anything: class B is not a parent of the class C - it's a parent of the module class C$. If a Java source references C.I, the Java compiler doesn't look in C$.

Un-commenting 2 triggers some compat-mode in the InnerClass attribute and marks I as being a static member class of C (not of C$), so a C.I reference in Java resolves to C$I.

So when we are in a Java compilation unit and look for a member of a module class C we should

first check the C module class (but not any of its parents)

then the C class and its parents, and also all parents module classes

If we also start type checking Java terms (in the future) things would get more complicated. Right now, in findMember, we don't know whether we're looking for a static or instance member. A type selection Outer.Inner, it can resolve to a static or non-static inner class, the same syntax is used for both. For terms, A.foo should not return an instance method.

Related older fix I wasn't aware of: scala/bug#3120. When type-checking a selection A.foo fails in a Java compilation unit, a synthetic SelectFromTypeTree tree will be type-checked as second try.

adriaanm · 2016-12-21T01:52:18Z

src/reflect/scala/reflect/internal/tpe/FindMembers.scala

+      // For Java compilation units, a `T` or `A.T` can refer to a static inner class `T`. Static inner
+      // classes are placed in the companion object. Under `searchCompanions`, we always check the companion
+      // before proceeding to the next base class.
+      if (searchCompanions) bcs.flatMap(bc => List(bc, bc.companionModule.moduleClass))


Could you add some more details from your commit message here? Maybe some variation of:

In Java, static members are inherited, so A.T can refer to a static member defined in a parent of A. In Scala we model static members as members of the class's companion object. In addition to the inheritance class hierarchy, we must be careful to follow the implied parallel hierarchy of the companion objects.

Yes, will do

Fixed a while ago, according to comments on the issue.

Scala puts static members of Java classes into the companion object symbol. When parsing Java code, references to static members need to be looked up in these companions. In 2.11.x, a synthetic `import A._` was added to Java classes having static members, which lead to some bugs (SI-9111). The scheme was changed in 4e822d7 (2.12.x only): when a lookup of `T` / `A.T` fails, the typer searches in the companion of the current class. This fixes some issues, but not all - In Java, `A.T` can refer to a static member defined in a parent of `A` - If a static `A.T` exists, but a parent of `A` defines a non-static member class `T`, the non-static one would be chosen (wrongly) This commit pushes the logic to consider companion objects into `FindMembers` and makes sure that for ever class in the base type sequence, the companion is considered before moving to the next base type.

SI-4788 implemented support for `@Retention` annotations of Java annotation classes: if a Java annotation class is itself annotated `@Retention(value=SOURCE)`, it is not emitted to the annotated (Scala) classfile. However, the Java parser currently skips annotations altogether, so if a Java annotation is passed to the Scala compiler in mixed compilation, its `@Retention` not respected. Instead, uses of the annotation class will be written to the classfile. The existing tests for t4788 basically both tested separate compilation, because partest's "mixed" compilation used to compile Scala sources twice (the second time with the Java classfiles on the classpath). This was fixed recently, uncovering the difference. Additionally, the actual `t4788-separate-compilation` did not work as intended because the filename groups were wrong (the `.scala` files should all have been in the `_2` group.).

adriaanm · 2016-12-21T20:07:40Z

src/compiler/scala/tools/nsc/typechecker/Contexts.scala

@@ -984,6 +985,10 @@ trait Contexts { self: Analyzer =>

    def isNameInScope(name: Name) = lookupSymbol(name, _ => true).isSuccess

+    def isPlainClassInJavaUnit(sym: Symbol): Boolean =
+      unit.isJava && sym.isJavaDefined && sym.isClass && !sym.isModuleClass && !sym.isPackageClass


Why exclude sym.isModuleClass? I'm trying to get this scenario to work (using scaladoc to more eagerly force infos):

public class A { public static class T {} } public class B extends A { public static class U extends T { } // ^ // error: not found: type T }

adriaanm · 2016-12-21T20:09:09Z

src/reflect/scala/reflect/internal/tpe/FindMembers.scala

+      // members: `A.T` can resolve to a static member defined in a parent of `A`.
+      //   class C { class T }; class B extends C { static class T }; class A extends B
+      // Here, `A.T` resolves to the static class `B.T`, so we need to check B's companion before proceeding to C.
+      if (searchCompanions) bcs.flatMap(bc => List(bc, bc.companionModule.moduleClass))


Based on my example above, I think we also need to support the case where we're looking for the base classes of a module class. The companion modules of its companion class's base classes should also be included.

retronym · 2016-12-22T06:58:30Z

Improvements in this area make it easier for us to typecheck more Java code, which we need to do (robustly) to improve support for annotations in jointly compiled sources (e.g. for the SI-4788 limitation noted above), or to typecheck the RHS of constant expressions.

I like your approach of addressing this at its core, in findMembers.

However, we'll have to be quite careful in making membership dependent on the context of the unit containing the reference. Will this break any caches? I can't find any problems right now. e.g. ModuleClassSymbol#implicitMembersCacheValue looks okay.

lrytz · 2017-01-06T13:35:39Z

I'm going to put this on hold, busy with other issues for now. Hope to pick it up for 2.12.2.

ghik · 2017-07-11T14:01:41Z

Hello,

Is there anything I can do to help so that this gets fixed in, say, 2.12.4? (I assume it's already too late for 2.12.3). It really makes me frustrated, because it's a regression, blocking us from migration to Scala 2.12, significant work had already been put into it and now it seems like it's stuck in a limbo.

lrytz · 2017-07-11T14:09:50Z

I'm sorry this slipped, I really need to pick it up again for 2.12.4. I have it on my list, but it just keeps getting bumped. I will take a look, keeping in mind that perfect is the enemy of good.

guizmaii · 2017-07-19T08:55:53Z

Hi,

Where can we track the evolution of this bug ?

Thanks

lrytz · 2017-07-19T09:02:40Z

At scala/bug#9111

guizmaii · 2017-07-19T14:27:22Z

Thanks

lrytz added 2 commits December 19, 2016 13:31

scala-jenkins added this to the 2.12.2 milestone Dec 19, 2016

lrytz changed the title ~~SI-10107 / SI-9111 WIP~~ SI-9111 / SI-10107 resolution of static references in Java code Dec 20, 2016

lrytz requested a review from adriaanm December 20, 2016 12:25

adriaanm reviewed Dec 21, 2016

View reviewed changes

lrytz added 2 commits December 21, 2016 10:16

SI-7386 test case

d29d5a9

Fixed a while ago, according to comments on the issue.

lrytz force-pushed the t10107 branch from 656a47a to cfe98b3 Compare December 21, 2016 09:36

adriaanm reviewed Dec 21, 2016

View reviewed changes

adriaanm self-assigned this Dec 21, 2016

lrytz closed this Jan 6, 2017

SethTisue removed this from the 2.12.2 milestone Feb 22, 2017

guizmaii mentioned this pull request Jul 17, 2017

[WIP] Update Scala version to 2.12.3 lagom/lagom#887

Closed

6 tasks

SethTisue mentioned this pull request May 1, 2018

Issue finding Java classes defined in interfaces when compiling Scala code referencing them scala/bug#10859

Closed

		@@ -984,6 +985,10 @@ trait Contexts { self: Analyzer =>

		def isNameInScope(name: Name) = lookupSymbol(name, _ => true).isSuccess

		def isPlainClassInJavaUnit(sym: Symbol): Boolean =

SI-9111 / SI-10107 resolution of static references in Java code #5606

SI-9111 / SI-10107 resolution of static references in Java code #5606

Uh oh!

Conversation

lrytz commented Dec 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adriaanm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lrytz Dec 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lrytz Dec 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lrytz Dec 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adriaanm Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adriaanm Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

retronym commented Dec 22, 2016

Uh oh!

lrytz commented Jan 6, 2017

Uh oh!

ghik commented Jul 11, 2017

Uh oh!

lrytz commented Jul 11, 2017

Uh oh!

guizmaii commented Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lrytz commented Jul 19, 2017

Uh oh!

guizmaii commented Jul 19, 2017

Uh oh!

Uh oh!

lrytz commented Dec 19, 2016 •

edited

Loading

lrytz Dec 23, 2016 •

edited

Loading

lrytz Dec 23, 2016 •

edited

Loading

lrytz Dec 23, 2016 •

edited

Loading

adriaanm Dec 21, 2016 •

edited

Loading

adriaanm Dec 21, 2016 •

edited

Loading

guizmaii commented Jul 19, 2017 •

edited

Loading