diff --git a/java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/analysis/HighlightVisitorImpl.java b/java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/analysis/HighlightVisitorImpl.java index 64f87ca1ec82..be6855c8f9cb 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/analysis/HighlightVisitorImpl.java +++ b/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)); } diff --git a/java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/analysis/JavaModuleGraphUtil.java b/java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/analysis/JavaModuleGraphUtil.java index fed781907c5c..9e271c08e503 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/analysis/JavaModuleGraphUtil.java +++ b/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 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 relations = MultiMap.create(); Set transitiveEdges = ContainerUtil.newTroveSet(); @@ -144,21 +159,28 @@ public class JavaModuleGraphUtil { private static void visit(PsiJavaModule module, MultiMap relations, Set 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 myGraph; + private final Graph myGraph; private final Set myTransitiveEdges; - public RequiresGraph(OutboundSemiGraph graph, Set transitiveEdges) { + public RequiresGraph(Graph graph, Set transitiveEdges) { myGraph = graph; myTransitiveEdges = transitiveEdges; } @@ -177,6 +199,52 @@ public class JavaModuleGraphUtil { return false; } + public Trinity findConflict(PsiJavaModule source) { + Map 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 processExports(PsiJavaModule start, BiFunction processor) { + return myGraph.getNodes().contains(start) ? processExports(start.getName(), start, 0, ContainerUtil.newHashSet(), processor) : null; + } + + private T processExports(String name, + PsiJavaModule module, + int layer, + Set visited, + BiFunction processor) { + if (visited.add(module)) { + if (layer == 1) { + for (PsiPackageAccessibilityStatement statement : module.getExports()) { + List 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 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(); } diff --git a/java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/analysis/ModuleHighlightUtil.java b/java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/analysis/ModuleHighlightUtil.java index b94293dc5b33..805992b056d7 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/analysis/ModuleHighlightUtil.java +++ b/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 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) diff --git a/java/java-psi-impl/src/com/intellij/psi/impl/light/LightJavaModule.java b/java/java-psi-impl/src/com/intellij/psi/impl/light/LightJavaModule.java index a436d8672e11..db05f9a2e037 100644 --- a/java/java-psi-impl/src/com/intellij/psi/impl/light/LightJavaModule.java +++ b/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 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 getExports() { - return Collections.emptyList(); + if (myExports == null) { + List 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 getModuleReferences() { + return Collections.emptyList(); + } + + @NotNull + @Override + public List 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); diff --git a/java/java-psi-impl/src/messages/JavaErrorMessages.properties b/java/java-psi-impl/src/messages/JavaErrorMessages.properties index 41cab2e10d97..c1be49910e57 100644 --- a/java/java-psi-impl/src/messages/JavaErrorMessages.properties +++ b/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 diff --git a/java/java-tests/testSrc/com/intellij/codeInsight/daemon/ModuleHighlightingTest.kt b/java/java-tests/testSrc/com/intellij/codeInsight/daemon/ModuleHighlightingTest.kt index 140260ea8ed3..9cc962e3f405 100644 --- a/java/java-tests/testSrc/com/intellij/codeInsight/daemon/ModuleHighlightingTest.kt +++ b/java/java-tests/testSrc/com/intellij/codeInsight/daemon/ModuleHighlightingTest.kt @@ -252,6 +252,53 @@ class ModuleHighlightingTest : LightJava9ModulesCodeInsightFixtureTestCase() { highlight("""module M { requires M2; }""") } + 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", """package pkg.collision2;""") + highlight("test2.java", """package pkg.collision4;""") + highlight("test3.java", """package pkg.collision7;""") + highlight("test4.java", """package java.util;""") + highlight("test5.java", """package pkg.lib2;""") + } + + 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(""" + module M { + 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(""" + module M { + requires M2; + requires lib.auto; + }""".trimIndent()) + } + // private fun highlight(text: String) = highlight("module-info.java", text)