unnecessary module dependency: support exported dependencies

don't report dependencies through which required modules or libraries come (IDEA-185022; IDEA-183789)

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<Set<Module>> 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<Module> onModules = getAllPossibleWhatModules(what);
+      if (onModules != null && fromModule != null){
         final RefModule refModule = myManager.getRefModule(fromModule);
         if (refModule != null) {
-          HashSet<Module> modules = new HashSet<>();
-          modules.add(onModule);
+          HashSet<Module> 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<PsiClass> superClasses = new LinkedHashSet<>();
     InheritanceUtil.getSuperClasses(currentClass, superClasses, false);
     for (PsiClass superClass : superClasses) {
-      ContainerUtil.addIfNotNull(modules, ModuleUtilCore.findModuleForPsiElement(superClass));
+      Set<Module> 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<Module> 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<OrderEntry> orderEntries = fileIndex.getOrderEntriesForFile(vFile);
+      if (orderEntries.isEmpty()) {
+        return null;
+      }
+      Set<Module> 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<Module> getModules(RefModule refModule) {
     Set<Module> modules = refModule.getUserData(DEPENDENCIES);
     if (modules == null){

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<Graph<Module>> 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<CommonProblemDescriptor> descriptors = new ArrayList<>();
+      final List<CommonProblemDescriptor> descriptors = new ArrayList<>();
       final Set<Module> modules = refModule.getUserData(UnnecessaryModuleDependencyAnnotator.DEPENDENCIES);
-      Graph<Module> graph = myGraph.get();
-      if (graph == null) {
-        graph = ModuleManager.getInstance(globalContext.getProject()).moduleGraph();
-        myGraph = new SoftReference<>(graph);
-      }
-
-      final RefManager refManager = globalContext.getRefManager();
-      currentDependencies:
+      final List<Module> 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<Module> 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<Module> 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<Module> 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;

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"))