diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/unnecessaryModuleDependency/UnnecessaryModuleDependencyAnnotator.java b/java/java-analysis-impl/src/com/intellij/codeInspection/unnecessaryModuleDependency/UnnecessaryModuleDependencyAnnotator.java index cff2224914d6..29632554b6fd 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/unnecessaryModuleDependency/UnnecessaryModuleDependencyAnnotator.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/unnecessaryModuleDependency/UnnecessaryModuleDependencyAnnotator.java @@ -6,15 +6,18 @@ import com.intellij.codeInspection.reference.RefManager; import com.intellij.codeInspection.reference.RefModule; import com.intellij.openapi.module.Module; import com.intellij.openapi.module.ModuleUtilCore; +import com.intellij.openapi.project.Project; +import com.intellij.openapi.roots.OrderEntry; +import com.intellij.openapi.roots.ProjectFileIndex; import com.intellij.openapi.util.Key; +import com.intellij.openapi.vfs.VirtualFile; import com.intellij.psi.*; import com.intellij.psi.util.InheritanceUtil; import com.intellij.psi.util.PsiUtil; -import com.intellij.util.containers.ContainerUtil; +import com.intellij.psi.util.PsiUtilCore; +import org.jetbrains.annotations.NotNull; -import java.util.HashSet; -import java.util.LinkedHashSet; -import java.util.Set; +import java.util.*; public class UnnecessaryModuleDependencyAnnotator extends RefGraphAnnotator { public static final Key> DEPENDENCIES = Key.create("inspection.dependencies"); @@ -28,13 +31,13 @@ public class UnnecessaryModuleDependencyAnnotator extends RefGraphAnnotator { @Override public void onMarkReferenced(PsiElement what, PsiElement from, boolean referencedFromClassInitializer) { if (what != null && from != null){ - final Module onModule = ModuleUtilCore.findModuleForPsiElement(what); - final Module fromModule = ModuleUtilCore.findModuleForPsiElement(from); - if (onModule != null && fromModule != null){ + //from should be always in sources + final Module fromModule = ModuleUtilCore.findModuleForFile(from.getContainingFile()); + final Set onModules = getAllPossibleWhatModules(what); + if (onModules != null && fromModule != null){ final RefModule refModule = myManager.getRefModule(fromModule); if (refModule != null) { - HashSet modules = new HashSet<>(); - modules.add(onModule); + HashSet modules = new HashSet<>(onModules); collectRequiredModulesInHierarchy(what, modules); modules.remove(fromModule); getModules(refModule).addAll(modules); @@ -45,6 +48,7 @@ public class UnnecessaryModuleDependencyAnnotator extends RefGraphAnnotator { @Override public void onMarkReferenced(RefElement refWhat, RefElement refFrom, boolean referencedFromClassInitializer) { + //case when both from and what are located in the scope, no library dependency expected RefModule fromModule = refFrom.getModule(); RefModule whatModule = refWhat.getModule(); if (fromModule != null && whatModule != null) { @@ -103,10 +107,34 @@ public class UnnecessaryModuleDependencyAnnotator extends RefGraphAnnotator { LinkedHashSet superClasses = new LinkedHashSet<>(); InheritanceUtil.getSuperClasses(currentClass, superClasses, false); for (PsiClass superClass : superClasses) { - ContainerUtil.addIfNotNull(modules, ModuleUtilCore.findModuleForPsiElement(superClass)); + Set onModules = getAllPossibleWhatModules(superClass); + if (onModules != null) modules.addAll(onModules); } } + /** + * Returns all owner modules for a library or single module set for a source outside of the inspecting scope + */ + private static Set getAllPossibleWhatModules(@NotNull PsiElement what) { + VirtualFile vFile = PsiUtilCore.getVirtualFile(what); + if (vFile == null) return null; + Project project = what.getProject(); + final ProjectFileIndex fileIndex = ProjectFileIndex.SERVICE.getInstance(project); + if (fileIndex.isInLibrarySource(vFile) || fileIndex.isInLibraryClasses(vFile)) { + final List orderEntries = fileIndex.getOrderEntriesForFile(vFile); + if (orderEntries.isEmpty()) { + return null; + } + Set modules = new HashSet<>(); + for (OrderEntry orderEntry : orderEntries) { + modules.add(orderEntry.getOwnerModule()); + } + return modules; + } + Module module = ModuleUtilCore.findModuleForFile(vFile, project); + return module != null ? Collections.singleton(module) : null; + } + private static Set getModules(RefModule refModule) { Set modules = refModule.getUserData(DEPENDENCIES); if (modules == null){ diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/unnecessaryModuleDependency/UnnecessaryModuleDependencyInspection.java b/java/java-analysis-impl/src/com/intellij/codeInspection/unnecessaryModuleDependency/UnnecessaryModuleDependencyInspection.java index 20ced4eefafe..50f6fcd3c415 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/unnecessaryModuleDependency/UnnecessaryModuleDependencyInspection.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/unnecessaryModuleDependency/UnnecessaryModuleDependencyInspection.java @@ -8,26 +8,20 @@ import com.intellij.codeInspection.reference.RefGraphAnnotator; import com.intellij.codeInspection.reference.RefManager; import com.intellij.codeInspection.reference.RefModule; import com.intellij.openapi.module.Module; -import com.intellij.openapi.module.ModuleManager; import com.intellij.openapi.project.Project; import com.intellij.openapi.roots.*; import com.intellij.openapi.util.Comparing; -import com.intellij.reference.SoftReference; -import com.intellij.util.ArrayUtil; -import com.intellij.util.graph.Graph; +import com.intellij.util.containers.ContainerUtil; import org.jetbrains.annotations.NonNls; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import java.util.ArrayList; -import java.util.Iterator; +import java.util.HashSet; import java.util.List; import java.util.Set; public class UnnecessaryModuleDependencyInspection extends GlobalInspectionTool { - - private SoftReference> myGraph = new SoftReference<>(null); - @Override public RefGraphAnnotator getAnnotator(@NotNull final RefManager refManager) { return new UnnecessaryModuleDependencyAnnotator(refManager); @@ -40,54 +34,39 @@ public class UnnecessaryModuleDependencyInspection extends GlobalInspectionTool final Module module = refModule.getModule(); final ModuleRootManager moduleRootManager = ModuleRootManager.getInstance(module); final OrderEntry[] declaredDependencies = moduleRootManager.getOrderEntries(); - final Module[] declaredModuleDependencies = moduleRootManager.getDependencies(); - List descriptors = new ArrayList<>(); + final List descriptors = new ArrayList<>(); final Set modules = refModule.getUserData(UnnecessaryModuleDependencyAnnotator.DEPENDENCIES); - Graph graph = myGraph.get(); - if (graph == null) { - graph = ModuleManager.getInstance(globalContext.getProject()).moduleGraph(); - myGraph = new SoftReference<>(graph); - } - - final RefManager refManager = globalContext.getRefManager(); - currentDependencies: + final List candidates = new ArrayList<>(); for (final OrderEntry entry : declaredDependencies) { - if (entry instanceof ModuleOrderEntry && ((ModuleOrderEntry)entry).getScope() != DependencyScope.RUNTIME) { - final Module dependency = ((ModuleOrderEntry)entry).getModule(); - if (dependency != null) { - if (modules == null || !modules.contains(dependency)) { - if (((ModuleOrderEntry)entry).isExported()) { - final Iterator iterator = graph.getOut(module); - while (iterator.hasNext()) { - final Module dep = iterator.next(); - if (!scope.containsModule(dep)) continue currentDependencies; - final RefModule depRefModule = refManager.getRefModule(dep); - if (depRefModule != null) { - final Set neededModules = depRefModule.getUserData(UnnecessaryModuleDependencyAnnotator.DEPENDENCIES); - if (neededModules != null && neededModules.contains(dependency)) { - continue currentDependencies; - } - } - } - } - if (modules != null) { - final OrderEntry[] dependenciesOfDependencies = ModuleRootManager.getInstance(dependency).getOrderEntries(); - for (OrderEntry secondDependency : dependenciesOfDependencies) { - if (secondDependency instanceof ModuleOrderEntry && ((ModuleOrderEntry)secondDependency).isExported()) { - final Module mod = ((ModuleOrderEntry)secondDependency).getModule(); - if (mod != null && modules.contains(mod) && ArrayUtil.find(declaredModuleDependencies, mod) < 0) { - continue currentDependencies; - } - } - } - } + if (entry instanceof ModuleOrderEntry && + ((ModuleOrderEntry)entry).getScope() != DependencyScope.RUNTIME && + !((ModuleOrderEntry)entry).isExported()) { - descriptors.add(createDescriptor(scope, manager, module, dependency)); - } + final Module dependency = ((ModuleOrderEntry)entry).getModule(); + if (dependency == null || modules != null && modules.remove(dependency)) { + continue; } + + candidates.add(dependency); } } + + for (Module dependency : candidates) { + if (modules != null) { + HashSet outs = new HashSet<>(); + OrderEnumerator.orderEntries(dependency) + .withoutSdk() + .exportedOnly() + .recursively() + .forEachModule(outs::add); + + if (ContainerUtil.intersects(modules, outs)) continue; + } + + descriptors.add(createDescriptor(scope, manager, module, dependency)); + } + return descriptors.isEmpty() ? null : descriptors.toArray(CommonProblemDescriptor.EMPTY_ARRAY); } return null; diff --git a/java/java-tests/testSrc/com/intellij/java/codeInspection/UnnecessaryModuleDependencyInspectionTest.kt b/java/java-tests/testSrc/com/intellij/java/codeInspection/UnnecessaryModuleDependencyInspectionTest.kt index f2bbfd8d7195..7aaa20ad3030 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/UnnecessaryModuleDependencyInspectionTest.kt +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/UnnecessaryModuleDependencyInspectionTest.kt @@ -11,16 +11,15 @@ import com.intellij.openapi.module.JavaModuleType import com.intellij.openapi.module.Module import com.intellij.openapi.roots.DependencyScope import com.intellij.openapi.roots.ModuleRootModificationUtil +import com.intellij.project.IntelliJProjectConfiguration import com.intellij.testFramework.InspectionTestUtil import com.intellij.testFramework.PsiTestUtil import com.intellij.testFramework.createGlobalContextForTool import com.intellij.testFramework.fixtures.JavaCodeInsightFixtureTestCase import org.junit.Assert -import java.io.IOException class UnnecessaryModuleDependencyInspectionTest : JavaCodeInsightFixtureTestCase() { - @Throws(Exception::class) fun testRequireSuperClassInDependencies() { addModuleDependencies() @@ -31,7 +30,6 @@ class UnnecessaryModuleDependencyInspectionTest : JavaCodeInsightFixtureTestCase assertInspectionProducesZeroResults() } - @Throws(Exception::class) fun testRequireSuperClassInUnusedReturnTypeOfFactory() { addModuleDependencies() @@ -43,7 +41,28 @@ class UnnecessaryModuleDependencyInspectionTest : JavaCodeInsightFixtureTestCase assertInspectionProducesZeroResults() } - @Throws(Exception::class) + fun testExportedLibraryThroughModuleDependency() { + val mod1 = PsiTestUtil.addModule(project, JavaModuleType.getModuleType(), "mod1", myFixture.tempDirFixture.findOrCreateDir("mod1")) + val lib = IntelliJProjectConfiguration.getProjectLibrary("JUnit4") + ModuleRootModificationUtil.addModuleLibrary(myModule, "JUnit4", lib.classesUrls, lib.sourcesUrls, emptyList(), DependencyScope.COMPILE, true) + ModuleRootModificationUtil.addDependency(mod1, myModule) + + myFixture.addFileToProject("mod1/MyTest1.java", "public class MyTest1 {@org.junit.Test public void test() {}}") + assertInspectionProducesZeroResults() + } + + fun testDeepExportedLibraryThroughModuleDependency() { + val mod1 = PsiTestUtil.addModule(project, JavaModuleType.getModuleType(), "mod1", myFixture.tempDirFixture.findOrCreateDir("mod1")) + val mod2 = PsiTestUtil.addModule(project, JavaModuleType.getModuleType(), "mod2", myFixture.tempDirFixture.findOrCreateDir("mod2")) + val lib = IntelliJProjectConfiguration.getProjectLibrary("JUnit4") + ModuleRootModificationUtil.addModuleLibrary(myModule, "JUnit4", lib.classesUrls, lib.sourcesUrls, emptyList(), DependencyScope.COMPILE, true) + ModuleRootModificationUtil.addDependency(mod1, myModule, DependencyScope.COMPILE, true) + ModuleRootModificationUtil.addDependency(mod2, mod1) + + myFixture.addFileToProject("mod2/MyTest2.java", "public class MyTest2 {@org.junit.Test public void test() {}}") + assertInspectionProducesZeroResults() + } + fun testRequireSuperClassInUnusedReturnType() { val mod1 = PsiTestUtil.addModule(project, JavaModuleType.getModuleType(), "mod1", myFixture.tempDirFixture.findOrCreateDir("mod1")) val mod2 = PsiTestUtil.addModule(project, JavaModuleType.getModuleType(), "mod2", myFixture.tempDirFixture.findOrCreateDir("mod2")) @@ -63,7 +82,6 @@ class UnnecessaryModuleDependencyInspectionTest : JavaCodeInsightFixtureTestCase assertInspectionProducesZeroResults() } - @Throws(Exception::class) fun testExportedDependencies() { val mod1 = PsiTestUtil.addModule(project, JavaModuleType.getModuleType(), "mod1", myFixture.tempDirFixture.findOrCreateDir("mod1")) val mod2 = PsiTestUtil.addModule(project, JavaModuleType.getModuleType(), "mod2", myFixture.tempDirFixture.findOrCreateDir("mod2")) @@ -75,6 +93,40 @@ class UnnecessaryModuleDependencyInspectionTest : JavaCodeInsightFixtureTestCase assertInspectionProducesZeroResults() } + fun testDeepExportedDependenciesWithDirectDependency() { + val topModule = deepDepends() + ModuleRootModificationUtil.addDependency(topModule, myModule) + val toolWrapper: InspectionToolWrapper<*, *> = GlobalInspectionToolWrapper(UnnecessaryModuleDependencyInspection()) + val scope = AnalysisScope(project) + val globalContext = createGlobalContextForTool(scope, project, listOf(toolWrapper)) + InspectionTestUtil.runTool(toolWrapper, scope, globalContext) + val presentation = globalContext.getPresentation(toolWrapper) + presentation.updateContent() + Assert.assertTrue(presentation.problemDescriptors.joinToString { problem -> problem.descriptionTemplate }, + presentation.hasReportedProblems()) + Assert.assertEquals("Module 'mod3' sources do not depend on module 'mod2' sources", + presentation.problemDescriptors.joinToString { problem -> problem.descriptionTemplate }) + } + + fun testDeepExportedDependenciesNoDirectDependency() { + deepDepends() + assertInspectionProducesZeroResults() + } + + private fun deepDepends() : Module { + val mod1 = PsiTestUtil.addModule(project, JavaModuleType.getModuleType(), "mod1", myFixture.tempDirFixture.findOrCreateDir("mod1")) + val mod2 = PsiTestUtil.addModule(project, JavaModuleType.getModuleType(), "mod2", myFixture.tempDirFixture.findOrCreateDir("mod2")) + val mod3 = PsiTestUtil.addModule(project, JavaModuleType.getModuleType(), "mod3", myFixture.tempDirFixture.findOrCreateDir("mod3")) + + ModuleRootModificationUtil.addDependency(mod3, mod2) + ModuleRootModificationUtil.addDependency(mod2, mod1, DependencyScope.COMPILE, true) + ModuleRootModificationUtil.addDependency(mod1, myModule, DependencyScope.COMPILE, true) + + myFixture.addClass("public class Class0 {}") + myFixture.addFileToProject("mod3/Class3.java", "public class Class3 extends Class0 {}") + return mod3 + } + private fun assertInspectionProducesZeroResults() { val toolWrapper: InspectionToolWrapper<*, *> = GlobalInspectionToolWrapper(UnnecessaryModuleDependencyInspection()) val scope = AnalysisScope(project) @@ -86,7 +138,6 @@ class UnnecessaryModuleDependencyInspectionTest : JavaCodeInsightFixtureTestCase presentation.hasReportedProblems()) } - @Throws(IOException::class) private fun addModuleDependencies(): Module { val mod1 = PsiTestUtil.addModule(project, JavaModuleType.getModuleType(), "mod1", myFixture.tempDirFixture.findOrCreateDir("mod1")) val mod2 = PsiTestUtil.addModule(project, JavaModuleType.getModuleType(), "mod2", myFixture.tempDirFixture.findOrCreateDir("mod2"))