[java] highlights package conflicts in Java modular projects (IDEA-169255)

java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/analysis/HighlightVisitorImpl.java

@@ -984,7 +984,7 @@ public class HighlightVisitorImpl extends JavaElementVisitor implements Highligh
     super.visitPackageStatement(statement);
     myHolder.add(AnnotationsHighlightUtil.checkPackageAnnotationContainingFile(statement, myFile));
     if (myLanguageLevel.isAtLeast(LanguageLevel.JDK_1_9)) {
-      if (!myHolder.hasErrorResults()) myHolder.add(ModuleHighlightUtil.checkPackageStatement(statement, myFile));
+      if (!myHolder.hasErrorResults()) myHolder.add(ModuleHighlightUtil.checkPackageStatement(statement, myFile, myJavaModule));
     }
   }
 
@@ -1373,7 +1373,7 @@ public class HighlightVisitorImpl extends JavaElementVisitor implements Highligh
       if (errorMessage != null) {
         final HighlightInfo info =
           HighlightInfo.newHighlightInfo(HighlightInfoType.ERROR).range(expression).descriptionAndTooltip(errorMessage).create();
-        if (method instanceof PsiMethod && !((PsiMethod)method).isConstructor() && 
+        if (method instanceof PsiMethod && !((PsiMethod)method).isConstructor() &&
             !((PsiMethod)method).hasModifierProperty(PsiModifier.ABSTRACT)) {
           final boolean shouldHave = !((PsiMethod)method).hasModifierProperty(PsiModifier.STATIC);
           final LocalQuickFixAndIntentionActionOnPsiElement fixStaticModifier =
@@ -1677,6 +1677,7 @@ public class HighlightVisitorImpl extends JavaElementVisitor implements Highligh
     if (!myHolder.hasErrorResults()) myHolder.add(ModuleHighlightUtil.checkFileName(module, myFile));
     if (!myHolder.hasErrorResults()) myHolder.add(ModuleHighlightUtil.checkFileDuplicates(module, myFile));
     if (!myHolder.hasErrorResults()) myHolder.addAll(ModuleHighlightUtil.checkDuplicateStatements(module));
+    if (!myHolder.hasErrorResults()) myHolder.add(ModuleHighlightUtil.checkClashingReads(module));
     if (!myHolder.hasErrorResults()) myHolder.addAll(ModuleHighlightUtil.checkUnusedServices(module));
     if (!myHolder.hasErrorResults()) myHolder.add(ModuleHighlightUtil.checkFileLocation(module, myFile));
   }

java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/analysis/JavaModuleGraphUtil.java

@@ -18,8 +18,10 @@ package com.intellij.codeInsight.daemon.impl.analysis;
 import com.intellij.openapi.module.Module;
 import com.intellij.openapi.module.ModuleManager;
 import com.intellij.openapi.project.Project;
+import com.intellij.openapi.util.Trinity;
 import com.intellij.openapi.vfs.VirtualFile;
 import com.intellij.psi.*;
+import com.intellij.psi.impl.light.LightJavaModule;
 import com.intellij.psi.impl.source.PsiJavaModuleReference;
 import com.intellij.psi.search.FilenameIndex;
 import com.intellij.psi.util.CachedValueProvider.Result;
@@ -29,12 +31,12 @@ import com.intellij.util.containers.MultiMap;
 import com.intellij.util.graph.DFSTBuilder;
 import com.intellij.util.graph.Graph;
 import com.intellij.util.graph.GraphGenerator;
-import com.intellij.util.graph.OutboundSemiGraph;
 import gnu.trove.THashSet;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
 import java.util.*;
+import java.util.function.BiFunction;
 import java.util.stream.Collectors;
 
 import static com.intellij.psi.PsiJavaModule.MODULE_INFO_FILE;
@@ -70,10 +72,17 @@ public class JavaModuleGraphUtil {
   }
 
   public static boolean reads(@NotNull PsiJavaModule source, @NotNull PsiJavaModule destination) {
-    Project project = source.getProject();
-    RequiresGraph graph = CachedValuesManager.getManager(project).getCachedValue(project, () ->
-      Result.create(buildRequiresGraph(project), OUT_OF_CODE_BLOCK_MODIFICATION_COUNT));
-    return graph.reads(source, destination);
+    return getRequiresGraph(source).reads(source, destination);
+  }
+
+  @Nullable
+  public static Trinity<String, PsiJavaModule, PsiJavaModule> findConflict(@NotNull PsiJavaModule module) {
+    return getRequiresGraph(module).findConflict(module);
+  }
+
+  @Nullable
+  public static PsiJavaModule findOrigin(@NotNull PsiJavaModule module, @NotNull String packageName) {
+    return getRequiresGraph(module).findOrigin(module, packageName);
   }
 
   // Looks for cycles between Java modules in the project sources.
@@ -124,8 +133,14 @@ public class JavaModuleGraphUtil {
     return map;
   }
 
+  private static RequiresGraph getRequiresGraph(PsiJavaModule module) {
+    Project project = module.getProject();
+    return CachedValuesManager.getManager(project).getCachedValue(project, () ->
+      Result.create(buildRequiresGraph(project), OUT_OF_CODE_BLOCK_MODIFICATION_COUNT));
+  }
+
   // Starting from source modules, collects all module dependencies in the project.
-  // The resulting graph is used for tracing readability.
+  // The resulting graph is used for tracing readability and checking package conflicts.
   private static RequiresGraph buildRequiresGraph(Project project) {
     MultiMap<PsiJavaModule, PsiJavaModule> relations = MultiMap.create();
     Set<String> transitiveEdges = ContainerUtil.newTroveSet();
@@ -144,21 +159,28 @@ public class JavaModuleGraphUtil {
   private static void visit(PsiJavaModule module, MultiMap<PsiJavaModule, PsiJavaModule> relations, Set<String> transitiveEdges) {
     if (!relations.containsKey(module)) {
       relations.putValues(module, Collections.emptyList());
+      boolean explicitJavaBase = false;
       for (PsiRequiresStatement statement : module.getRequires()) {
-        for (PsiJavaModule dependency : PsiJavaModuleReference.multiResolve(statement, statement.getModuleName(), false)) {
+        String moduleName = statement.getModuleName();
+        if (PsiJavaModule.JAVA_BASE.equals(moduleName)) explicitJavaBase = true;
+        for (PsiJavaModule dependency : PsiJavaModuleReference.multiResolve(statement, moduleName, false)) {
           relations.putValue(module, dependency);
           if (statement.hasModifierProperty(PsiModifier.TRANSITIVE)) transitiveEdges.add(RequiresGraph.key(dependency, module));
           visit(dependency, relations, transitiveEdges);
         }
       }
+      if (!explicitJavaBase && !(module instanceof LightJavaModule)) {
+        PsiJavaModule javaBase = PsiJavaModuleReference.resolve(module, PsiJavaModule.JAVA_BASE, false);
+        if (javaBase != null) relations.putValue(module, javaBase);
+      }
     }
   }
 
   private static class RequiresGraph {
-    private final OutboundSemiGraph<PsiJavaModule> myGraph;
+    private final Graph<PsiJavaModule> myGraph;
     private final Set<String> myTransitiveEdges;
 
-    public RequiresGraph(OutboundSemiGraph<PsiJavaModule> graph, Set<String> transitiveEdges) {
+    public RequiresGraph(Graph<PsiJavaModule> graph, Set<String> transitiveEdges) {
       myGraph = graph;
       myTransitiveEdges = transitiveEdges;
     }
@@ -177,6 +199,52 @@ public class JavaModuleGraphUtil {
       return false;
     }
 
+    public Trinity<String, PsiJavaModule, PsiJavaModule> findConflict(PsiJavaModule source) {
+      Map<String, PsiJavaModule> exports = ContainerUtil.newHashMap();
+      return processExports(source, (pkg, m) -> {
+        PsiJavaModule existing = exports.put(pkg, m);
+        return existing != null ? new Trinity<>(pkg, existing, m) : null;
+      });
+    }
+
+    public PsiJavaModule findOrigin(PsiJavaModule module, String packageName) {
+      return processExports(module, (pkg, m) -> packageName.equals(pkg) ? m : null);
+    }
+
+    private <T> T processExports(PsiJavaModule start, BiFunction<String, PsiJavaModule, T> processor) {
+      return myGraph.getNodes().contains(start) ? processExports(start.getName(), start, 0, ContainerUtil.newHashSet(), processor) : null;
+    }
+
+    private <T> T processExports(String name,
+                                 PsiJavaModule module,
+                                 int layer,
+                                 Set<PsiJavaModule> visited,
+                                 BiFunction<String, PsiJavaModule, T> processor) {
+      if (visited.add(module)) {
+        if (layer == 1) {
+          for (PsiPackageAccessibilityStatement statement : module.getExports()) {
+            List<String> exportTargets = statement.getModuleNames();
+            if (exportTargets.isEmpty() || exportTargets.contains(name)) {
+              T result = processor.apply(statement.getPackageName(), module);
+              if (result != null) return result;
+            }
+          }
+        }
+        if (layer < 2) {
+          Iterator<PsiJavaModule> iterator = myGraph.getIn(module);
+          while (iterator.hasNext()) {
+            PsiJavaModule dependency = iterator.next();
+            if (layer == 0 || myTransitiveEdges.contains(key(dependency, module))) {
+              T result = processExports(name, dependency, 1, visited, processor);
+              if (result != null) return result;
+            }
+          }
+        }
+      }
+
+      return null;
+    }
+
     public static String key(PsiJavaModule module, PsiJavaModule exporter) {
       return module.getName() + '/' + exporter.getName();
     }

java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/analysis/ModuleHighlightUtil.java

@@ -27,6 +27,7 @@ import com.intellij.openapi.module.ModuleUtilCore;
 import com.intellij.openapi.project.Project;
 import com.intellij.openapi.roots.ProjectFileIndex;
 import com.intellij.openapi.util.TextRange;
+import com.intellij.openapi.util.Trinity;
 import com.intellij.openapi.util.text.StringUtil;
 import com.intellij.openapi.vfs.JarFileSystem;
 import com.intellij.openapi.vfs.VirtualFile;
@@ -94,7 +95,7 @@ public class ModuleHighlightUtil {
       .orElse(null);
   }
 
-  static HighlightInfo checkPackageStatement(@NotNull PsiPackageStatement statement, @NotNull PsiFile file) {
+  static HighlightInfo checkPackageStatement(@NotNull PsiPackageStatement statement, @NotNull PsiFile file, @Nullable PsiJavaModule module) {
     if (PsiUtil.isModuleFile(file)) {
       String message = JavaErrorMessages.message("module.no.package");
       HighlightInfo info = HighlightInfo.newHighlightInfo(HighlightInfoType.ERROR).range(statement).descriptionAndTooltip(message).create();
@@ -102,6 +103,17 @@ public class ModuleHighlightUtil {
       return info;
     }
 
+    if (module != null) {
+      String packageName = statement.getPackageName();
+      if (packageName != null) {
+        PsiJavaModule origin = JavaModuleGraphUtil.findOrigin(module, packageName);
+        if (origin != null) {
+          String message = JavaErrorMessages.message("module.conflicting.packages", packageName, origin.getName());
+          return HighlightInfo.newHighlightInfo(HighlightInfoType.ERROR).range(statement).descriptionAndTooltip(message).create();
+        }
+      }
+    }
+
     return null;
   }
 
@@ -468,6 +480,18 @@ public class ModuleHighlightUtil {
     return null;
   }
 
+  @Nullable
+  static HighlightInfo checkClashingReads(@NotNull PsiJavaModule module) {
+    Trinity<String, PsiJavaModule, PsiJavaModule> conflict = JavaModuleGraphUtil.findConflict(module);
+    if (conflict != null) {
+      String message = JavaErrorMessages.message(
+        "module.conflicting.reads", module.getName(), conflict.first, conflict.second.getName(), conflict.third.getName());
+      return HighlightInfo.newHighlightInfo(HighlightInfoType.ERROR).range(range(module)).descriptionAndTooltip(message).create();
+    }
+
+    return null;
+  }
+
   private static Module findModule(PsiElement element) {
     return Optional.ofNullable(element.getContainingFile())
       .map(PsiFile::getVirtualFile)

java/java-psi-impl/src/com/intellij/psi/impl/light/LightJavaModule.java

@@ -19,16 +19,21 @@ import com.intellij.lang.java.JavaLanguage;
 import com.intellij.navigation.ItemPresentation;
 import com.intellij.navigation.ItemPresentationProviders;
 import com.intellij.openapi.util.text.StringUtil;
+import com.intellij.openapi.vfs.VfsUtilCore;
 import com.intellij.openapi.vfs.VirtualFile;
+import com.intellij.openapi.vfs.VirtualFileVisitor;
 import com.intellij.psi.*;
 import com.intellij.psi.javadoc.PsiDocComment;
 import com.intellij.psi.util.CachedValueProvider;
 import com.intellij.psi.util.CachedValuesManager;
+import com.intellij.psi.util.PsiUtil;
 import com.intellij.util.IncorrectOperationException;
+import com.intellij.util.containers.ContainerUtil;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
 import java.util.Collections;
+import java.util.List;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
@@ -37,6 +42,7 @@ import static com.intellij.util.ObjectUtils.notNull;
 public class LightJavaModule extends LightElement implements PsiJavaModule {
   private final LightJavaModuleReferenceElement myRefElement;
   private final VirtualFile myJarRoot;
+  private List<PsiPackageAccessibilityStatement> myExports = null;
 
   private LightJavaModule(@NotNull PsiManager manager, @NotNull VirtualFile jarRoot) {
     super(manager, JavaLanguage.INSTANCE);
@@ -64,7 +70,34 @@ public class LightJavaModule extends LightElement implements PsiJavaModule {
   @NotNull
   @Override
   public Iterable<PsiPackageAccessibilityStatement> getExports() {
-    return Collections.emptyList();
+    if (myExports == null) {
+      List<PsiPackageAccessibilityStatement> exports = ContainerUtil.newArrayList();
+
+      VfsUtilCore.visitChildrenRecursively(myJarRoot, new VirtualFileVisitor() {
+        private JavaDirectoryService service = JavaDirectoryService.getInstance();
+
+        @Override
+        public boolean visitFile(@NotNull VirtualFile file) {
+          if (file.isDirectory() && !myJarRoot.equals(file)) {
+            PsiDirectory directory = myManager.findDirectory(file);
+            if (directory != null) {
+              PsiPackage pkg = service.getPackage(directory);
+              if (pkg != null) {
+                String packageName = pkg.getQualifiedName();
+                if (!packageName.isEmpty() && !PsiUtil.isPackageEmpty(new PsiDirectory[]{directory}, packageName)) {
+                  exports.add(new LightPackageAccessibilityStatement(myManager, packageName));
+                }
+              }
+            }
+          }
+          return true;
+        }
+      });
+
+      myExports = exports;
+    }
+
+    return myExports;
   }
 
   @NotNull
@@ -164,6 +197,50 @@ public class LightJavaModule extends LightElement implements PsiJavaModule {
     }
   }
 
+  private static class LightPackageAccessibilityStatement extends LightElement implements PsiPackageAccessibilityStatement {
+    private final String myPackageName;
+
+    public LightPackageAccessibilityStatement(@NotNull PsiManager manager, @NotNull String packageName) {
+      super(manager, JavaLanguage.INSTANCE);
+      myPackageName = packageName;
+    }
+
+    @NotNull
+    @Override
+    public Role getRole() {
+      return Role.EXPORTS;
+    }
+
+    @Nullable
+    @Override
+    public PsiJavaCodeReferenceElement getPackageReference() {
+      return null;
+    }
+
+    @Nullable
+    @Override
+    public String getPackageName() {
+      return myPackageName;
+    }
+
+    @NotNull
+    @Override
+    public Iterable<PsiJavaModuleReferenceElement> getModuleReferences() {
+      return Collections.emptyList();
+    }
+
+    @NotNull
+    @Override
+    public List<String> getModuleNames() {
+      return Collections.emptyList();
+    }
+
+    @Override
+    public String toString() {
+      return "PsiPackageAccessibilityStatement";
+    }
+  }
+
   @NotNull
   public static LightJavaModule getModule(@NotNull final PsiManager manager, @NotNull final VirtualFile jarRoot) {
     final PsiDirectory directory = manager.findDirectory(jarRoot);

java/java-psi-impl/src/messages/JavaErrorMessages.properties

@@ -425,6 +425,8 @@ module.service.unused=Service interface provided but not exported or used
 module.package.not.exported=The module ''{0}'' does not export the package ''{1}'' to the module ''{2}''
 module.package.on.classpath=A named module cannot access packages of an unnamed one
 module.not.in.requirements=The module ''{0}'' does not have the module ''{1}'' in requirements
+module.conflicting.reads=Module ''{0}'' reads package ''{1}'' from both ''{2}'' and ''{3}''
+module.conflicting.packages=Package ''{0}'' exists in another module: {1}
 
 feature.generics=Generics
 feature.annotations=Annotations

java/java-tests/testSrc/com/intellij/codeInsight/daemon/ModuleHighlightingTest.kt

@@ -252,6 +252,53 @@ class ModuleHighlightingTest : LightJava9ModulesCodeInsightFixtureTestCase() {
     highlight("""module M { requires <warning descr="'M2' is deprecated">M2</warning>; }""")
   }
 
+  fun testPackageConflicts() {
+    addFile("pkg/collision2/C2.java", "package pkg.collision2;\npublic class C2 { }", M2)
+    addFile("pkg/collision4/C4.java", "package pkg.collision4;\npublic class C4 { }", M4)
+    addFile("pkg/collision7/C7.java", "package pkg.collision7;\npublic class C7 { }", M7)
+    addFile("module-info.java", "module M2 { exports pkg.collision2; }", M2)
+    addFile("module-info.java", "module M4 { exports pkg.collision4 to M88; }", M4)
+    addFile("module-info.java", "module M6 { requires transitive M7; }", M6)
+    addFile("module-info.java", "module M7 { exports pkg.collision7 to M6; }", M7)
+    addFile("module-info.java", "module M { requires M2; requires M4; requires M6; requires lib.auto; }")
+    highlight("test1.java", """<error descr="Package 'pkg.collision2' exists in another module: M2">package pkg.collision2;</error>""")
+    highlight("test2.java", """package pkg.collision4;""")
+    highlight("test3.java", """package pkg.collision7;""")
+    highlight("test4.java", """<error descr="Package 'java.util' exists in another module: java.base">package java.util;</error>""")
+    highlight("test5.java", """<error descr="Package 'pkg.lib2' exists in another module: lib.auto">package pkg.lib2;</error>""")
+  }
+
+  fun testClashingReads1() {
+    addFile("pkg/collision/C2.java", "package pkg.collision;\npublic class C2 { }", M2)
+    addFile("pkg/collision/C7.java", "package pkg.collision;\npublic class C7 { }", M7)
+    addFile("module-info.java", "module M2 { exports pkg.collision; }", M2)
+    addFile("module-info.java", "module M6 { requires transitive M7; }", M6)
+    addFile("module-info.java", "module M7 { exports pkg.collision; }", M7)
+    highlight("""
+        <error descr="Module 'M' reads package 'pkg.collision' from both 'M2' and 'M7'">module M</error> {
+          requires M2;
+          requires M6;
+        }""".trimIndent())
+  }
+
+  fun testClashingReads2() {
+    addFile("pkg/collision/C2.java", "package pkg.collision;\npublic class C2 { }", M2)
+    addFile("pkg/collision/C4.java", "package pkg.collision;\npublic class C4 { }", M4)
+    addFile("module-info.java", "module M2 { exports pkg.collision; }", M2)
+    addFile("module-info.java", "module M4 { exports pkg.collision to somewhere; }", M4)
+    highlight("module M { requires M2; requires M4; }")
+  }
+
+  fun testClashingReads3() {
+    addFile("pkg/lib2/C2.java", "package pkg.lib2;\npublic class C2 { }", M2)
+    addFile("module-info.java", "module M2 { exports pkg.lib2; }", M2)
+    highlight("""
+        <error descr="Module 'M' reads package 'pkg.lib2' from both 'M2' and 'lib.auto'">module M</error> {
+          requires M2;
+          requires <warning descr="Ambiguous module reference: lib.auto">lib.auto</warning>;
+        }""".trimIndent())
+  }
+
   //<editor-fold desc="Helpers.">
   private fun highlight(text: String) = highlight("module-info.java", text)