RFR: 8264258: Unknown lookups in the java package give misleading compilation errors

[Jan Lahoda]

On Tue, 20 Apr 2021 06:42:45 GMT, Srikanth Adayapalam <sadayapalam at openjdk.org> wrote:

>> Unknown lookups in the java package give misleading compilation errors
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java line 4064:
> 
>> 4062:             if (!location.name.isEmpty()) {
>> 4063:                 if (location.kind == PCK && !site.tsym.exists() && location.name != names.java) {
>> 4064:                     return diags.create(dkind, log.currentSource(), pos,
> 
> This works, but I am not sure it is the fix we want. I would be curious to find out why site.tsym.exists() returns false for the java PackageSymbol. What I find with some preliminary debugging is that we end up creating a PackageSymbol for `java' that is distinct from the PackageSymbol java that is the owner of the PackageSymbol java.lang.
> 
> In this block of code
> 
> // Import-on-demand java.lang.
>                 PackageSymbol javaLang = syms.enterPackage(syms.java_base, names.java_lang);
>                 if (javaLang.members().isEmpty() && !javaLang.exists())
>                     throw new FatalError(diags.fragment(Fragments.FatalErrNoJavaLang));
>                 importAll(make.at(tree.pos()).Import(make.QualIdent(javaLang), false), javaLang, env);
> 
> com.sun.tools.javac.comp.TypeEnter.ImportsPhase#resolveImports
> 
> at the importAll call if you have a breakpoint and query the javaLang.exists() and javaLang.owner.exists() they both answer true.
> 
> So it looks fishy to me that we somehow end up with a distinct PackageSymbol for java that answers false for exists() even as there exist other symbols that answer true.
> 
> If you debug through javac as it attributes:
> 
> Object o1 = java.lang.String.class;
> 
> while attributing the FieldSelect node you can see that the PackageSymbol for java.lang has a owner that is distinct from the PackageSymbol for siteSym - so we do right thing by coalescing the symbols somehow - how is what needs further analysis.

There is an issue with the PackageSymbol for `java`: it is not clear what is the correct PackageSymbol. There is no module that would export package `java`. There are multiple modules that export direct sub-packages of `java` (e.g. java.base and java.desktop). In case of package `java`, we could say the PackageSymbol should be from java.base, as that is the most base module that contains subpackages of `java`, but that does not work generally. Consider, e.g., three modules: m1 exporting api.a, m2 exporting api.b (with no relation between m1 and m2), and m3 using both m1 and m2. There is no way to have a PackageSymbol for `api` in `m3` such that it would be the same time the owner of `api.a` in `m1` and `api.b` in `m2`. So, what javac is doing is that it synthesizes a new PackageSymbol in the current module (for `java` or `api`). Most of the few places where this caused issues were hopefully fixed, so this may be one of the remaining. 

One thing we could try is to look into the `visiblePackages` of a reasonable module and see if any of them is existing and is a sub-package of the current package (there may be a nicer way to do this, tough):

diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java
index db416413588..6e4db4bc49f 100644
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java
@@ -4060,7 +4060,8 @@ public class Resolve {
                 location = site.tsym;
             }
             if (!location.name.isEmpty()) {
-                if (location.kind == PCK && !site.tsym.exists()) {
+                if (location.kind == PCK && !site.tsym.exists() &&
+                    location.packge().modle.visiblePackages.values().stream().filter(ps -> ps.exists()).flatMap(this::owners).noneMatch(ps -> ps.flatName() == site.tsym.flatName())) {
                     return diags.create(dkind, log.currentSource(), pos,
                         "doesnt.exist", location);
                 }
@@ -4087,6 +4088,23 @@ public class Resolve {
         private Object args(List<Type> args) {
             return args.isEmpty() ? args : methodArguments(args);
         }
+        private Stream<Symbol> owners(PackageSymbol ps) {
+            Iterable<Symbol> ownersIt = () -> new Iterator<>() {
+                private Symbol current = ps.owner;
+                @Override
+                public boolean hasNext() {
+                    return !current.name.isEmpty();
+                }
+
+                @Override
+                public Symbol next() {
+                    Symbol result = current;
+                    current = current.owner;
+                    return result;
+                }
+            };
+            return StreamSupport.stream(ownersIt.spliterator(), false);
+        }
 
         private String getErrorKey(KindName kindname, boolean hasTypeArgs, boolean hasLocation) {
             String key = "cant.resolve";

-------------

PR: https://git.openjdk.java.net/jdk/pull/3428