[wip] Adding to the unused dependency rule

src/main/groovy/com/netflix/nebula/lint/plugin/GradleLintCorrectionTask.groovy

@@ -98,7 +98,7 @@ class GradleLintCorrectionTask extends DefaultTask {
                             correctedViolations++
                         } else if (v.addition) {
                             textOutput.withStyle(StyledTextOutput.Style.UserInput).println("adding:")
-                            textOutput.print(v.addition)
+                            textOutput.println(v.addition)
                             correctedViolations++
                         }
                     } else {

src/main/groovy/com/netflix/nebula/lint/rule/dependency/DependencyClassVisitor.groovy

@@ -1,19 +1,19 @@
 package com.netflix.nebula.lint.rule.dependency
 
-import org.gradle.api.artifacts.ModuleVersionIdentifier
+import org.gradle.api.artifacts.ResolvedDependency
 import org.objectweb.asm.*
 import org.objectweb.asm.signature.SignatureReader
 import org.objectweb.asm.signature.SignatureVisitor
 import org.slf4j.Logger
 
 class DependencyClassVisitor extends ClassVisitor {
-    private Map<String, Set<ModuleVersionIdentifier>> classOwners
+    private Map<String, Set<ResolvedDependency>> classOwners
     private String className
     private Logger logger
 
-    Set<ModuleVersionIdentifier> references = new HashSet()
+    Set<ResolvedDependency> references = new HashSet()
 
-    DependencyClassVisitor(Map<String, Set<ModuleVersionIdentifier>> classOwners, Logger logger) {
+    DependencyClassVisitor(Map<String, Set<ResolvedDependency>> classOwners, Logger logger) {
         super(Opcodes.ASM5)
         this.classOwners = classOwners
         this.logger = logger
@@ -125,6 +125,7 @@ class DependencyClassVisitor extends ClassVisitor {
 
         @Override
         void visitMethodInsn(int opcode, String owner, String name, String desc, boolean itf) {
+            readType(owner)
             readType(Type.getReturnType(desc).descriptor)
             Type.getArgumentTypes(desc).collect { readType(it.descriptor) }
         }
@@ -156,6 +157,12 @@ class DependencyClassVisitor extends ClassVisitor {
             return new DependencyAnnotationVisitor()
         }
 
+        @Override
+        AnnotationVisitor visitAnnotation(String desc, boolean visible) {
+            readType(desc)
+            return super.visitAnnotation(desc, visible)
+        }
+
         @Override
         void visitTryCatchBlock(Label start, Label end, Label handler, String type) {
             readType(type)

src/main/groovy/com/netflix/nebula/lint/rule/dependency/UnusedDependencyRule.groovy

@@ -4,8 +4,8 @@ import com.netflix.nebula.lint.rule.GradleDependency
 import com.netflix.nebula.lint.rule.GradleLintRule
 import com.netflix.nebula.lint.rule.GradleModelAware
 import org.codehaus.groovy.ast.expr.MethodCallExpression
-import org.gradle.api.artifacts.ModuleVersionIdentifier
 import org.gradle.api.artifacts.ResolvedDependency
+import org.gradle.api.internal.artifacts.ivyservice.ivyresolve.strategy.DefaultVersionComparator
 import org.gradle.api.tasks.compile.AbstractCompile
 import org.objectweb.asm.ClassReader
 import org.slf4j.Logger
@@ -20,7 +20,7 @@ import java.util.jar.JarEntry
 import java.util.jar.JarFile
 
 class UnusedDependencyRule extends GradleLintRule implements GradleModelAware {
-    def unusedDependencies
+    Collection<ResolvedDependency> firstLevelDependencies
 
     Logger logger = LoggerFactory.getLogger(UnusedDependencyRule)
 
@@ -45,49 +45,131 @@ class UnusedDependencyRule extends GradleLintRule implements GradleModelAware {
         return definedClasses
     }
 
-    Map<String, Set<ModuleVersionIdentifier>> firstOrderDependencyClassOwners() {
-        def classOwners = new HashMap<String, Set<ModuleVersionIdentifier>>().withDefault {[] as Set}
+    Map<String, Set<ResolvedDependency>> classOwners() {
+        def classOwners = new HashMap<String, Set<ResolvedDependency>>().withDefault {[] as Set}
         def mvidsAlreadySeen = [] as Set
 
-        project.configurations*.resolvedConfiguration*.firstLevelModuleDependencies*.each { ResolvedDependency d ->
-            if (mvidsAlreadySeen.add(d.module.id)) {
-                for(clazz in classSet(d))
-                    classOwners[clazz].add(d.module.id)
+
+        def recurseFindClassOwnersSingle
+        recurseFindClassOwnersSingle = { ResolvedDependency d ->
+            for (clazz in classSet(d)) {
+                logger.debug('Class {} found in module {}', clazz, d.module.id)
+                classOwners[clazz].add(d)
+            }
+
+            d.children.each {
+                recurseFindClassOwnersSingle(it)
             }
         }
 
+        def recurseFindClassOwners
+        recurseFindClassOwners = { Collection<ResolvedDependency> ds ->
+            if(ds.isEmpty()) return
+
+            def notYetSeen = ds.findAll { d -> mvidsAlreadySeen.add(d.module.id) }
+
+            notYetSeen.each { d ->
+                for (clazz in classSet(d)) {
+                    logger.debug('Class {} found in module {}', clazz, d.module.id)
+                    classOwners[clazz].add(d)
+                }
+            }
+
+            recurseFindClassOwners(notYetSeen*.children.flatten())
+        }
+
+        recurseFindClassOwners(firstLevelDependencies)
+
         return classOwners
     }
 
+    boolean unusedDependenciesCalculated = false
+    Set<ResolvedDependency> firstOrderDependenciesToRemove = new HashSet()
+    Set<ResolvedDependency> transitiveDependenciesToAddAsFirstOrder = new HashSet()
+    Map<ResolvedDependency, String> firstOrderDependenciesWhoseConfigurationNeedsToChange = [:]
+
+    Collection<String> configurations(ResolvedDependency d) {
+        return project.configurations.findAll { it.resolvedConfiguration.firstLevelModuleDependencies.contains(d) }*.name
+    }
+
     void calculateUnusedDependenciesIfNecessary() {
-        if(!unusedDependencies) {
-            def classOwners = firstOrderDependencyClassOwners()
-
-            unusedDependencies = project
-                    .configurations*.resolvedConfiguration*.firstLevelModuleDependencies*.collect { it.module.id }
-                    .flatten()
-                    .unique()
-                    .collect { ModuleVersionIdentifier mvid -> "$mvid.group:$mvid.name".toString() }
-
-            def referencedDependencies = project.tasks.findAll { it instanceof AbstractCompile }
-                    .collect {
-                        logger.debug('Looking for output dir for task {}', it.name)
-                        it as AbstractCompile
+        if(unusedDependenciesCalculated)
+            return
+
+        firstLevelDependencies = project.configurations*.resolvedConfiguration*.firstLevelModuleDependencies
+                .flatten().unique() as Collection<ResolvedDependency>
+
+        // Unused first order dependencies
+        Collection<ResolvedDependency> unusedDependencies = firstLevelDependencies.clone()
+
+        // Used dependencies, both first order and transitive
+        def classOwners = classOwners()
+        Collection<ResolvedDependency> usedDependencies = project.tasks.findAll { it instanceof AbstractCompile }
+                .collect { it as AbstractCompile }
+                .collect { it.destinationDir }
+                .unique()
+                .findAll { it.exists() }
+                .collect { dependencyReferences(it, classOwners) }
+                .flatten()
+                .unique { it.module.id } as Collection<ResolvedDependency>
+
+        unusedDependencies.removeAll(usedDependencies)
+
+        for(d in firstLevelDependencies) {
+            def confs = configurations(d)
+            if(unusedDependencies.contains(d)) {
+                if(!confs.contains('compile') && !confs.contains('testCompile'))
+                    continue
+
+                firstOrderDependenciesToRemove.add(d)
+
+                def inUse = transitivesInUse(d, usedDependencies)
+                if(!inUse.isEmpty()) {
+                    inUse.each { used ->
+                        def matching = firstLevelDependencies.find {
+                            it.module.id.group == used.moduleGroup && it.module.id.name == used.moduleName
+                        }
+
+                        if(!matching) {
+                            transitiveDependenciesToAddAsFirstOrder.add(used)
+                        }
+                        else if(matching.configuration == 'runtime') {
+                            firstOrderDependenciesWhoseConfigurationNeedsToChange.put(matching, 'compile')
+                        }
                     }
-                    .collect { it.destinationDir }
-                    .unique()
-                    .findAll { it.exists() }
-                    .collect { dependencyReferences(it, classOwners) }
-                    .flatten()
-                    .unique()
-                    .collect { ModuleVersionIdentifier mvid -> "$mvid.group:$mvid.name".toString() }
-
-            unusedDependencies.removeAll(referencedDependencies)
+                }
+            }
+            else {
+                if(!confs.contains('compile') && !confs.contains('testCompile')) {
+                    firstOrderDependenciesWhoseConfigurationNeedsToChange.put(d, 'compile')
+                }
+            }
         }
+
+        // only keep the highest version of each transitive module that we need to add as a first order dependency
+        def versionComparator = new DefaultVersionComparator().asStringComparator()
+        transitiveDependenciesToAddAsFirstOrder = transitiveDependenciesToAddAsFirstOrder
+                .groupBy { "$it.moduleGroup:$it.moduleName".toString() }
+                .values()
+                .collect {
+                    it.sort { d1, d2 -> versionComparator.compare(d2.moduleVersion, d1.moduleVersion) }.first()
+                }
+                .toSet()
+
+        unusedDependenciesCalculated = true
     }
 
-    Set<ModuleVersionIdentifier> dependencyReferences(File classesDir, Map<String, Set<ModuleVersionIdentifier>> classOwners) {
-        def references = new HashSet<ModuleVersionIdentifier>()
+    private Set<ResolvedDependency> transitivesInUse(ResolvedDependency d, Collection<ResolvedDependency> usedDependencies) {
+        def childrenInUse = d.children.collect { transitivesInUse(it, usedDependencies) }.flatten() as Collection<ResolvedDependency>
+        if(usedDependencies.contains(d)) {
+            return childrenInUse.plus(d).toSet()
+        }
+        else
+            return childrenInUse
+    }
+
+    Set<ResolvedDependency> dependencyReferences(File classesDir, Map<String, Set<ResolvedDependency>> classOwners) {
+        def references = new HashSet<ResolvedDependency>()
 
         logger.debug('Looking for classes to examine in {}', classesDir)
 
@@ -111,7 +193,38 @@ class UnusedDependencyRule extends GradleLintRule implements GradleModelAware {
     @Override
     void visitGradleDependency(MethodCallExpression call, String conf, GradleDependency dep) {
         calculateUnusedDependenciesIfNecessary()
-        if(unusedDependencies.contains("$dep.group:$dep.name".toString()))
+
+        def matchesGradleDep = { ResolvedDependency d -> d.module.id.group == dep.group && d.module.id.name == dep.name }
+        def match
+
+        if(firstOrderDependenciesToRemove.find(matchesGradleDep)) {
             addViolationToDelete(call, 'this dependency is unused and can be removed')
+        }
+        else if((match = firstOrderDependenciesWhoseConfigurationNeedsToChange.keySet().find(matchesGradleDep))) {
+            def toConf = firstOrderDependenciesWhoseConfigurationNeedsToChange[match]
+            addViolationWithReplacement(call, 'this dependency is required at compile time',
+                    "$toConf '$match.module.id")
+        }
+    }
+
+    @Override
+    void visitMethodCallExpression(MethodCallExpression call) {
+        if(call.methodAsString == 'dependencies') {
+            // TODO match indentation of surroundings
+            def indentation = ''.padLeft(call.columnNumber + 3)
+            def transitiveSize = transitiveDependenciesToAddAsFirstOrder.size()
+
+            if(transitiveSize == 1) {
+                def d = transitiveDependenciesToAddAsFirstOrder.first()
+                addViolationInsert(call, 'one or more classes in this transitive dependency are required by your code directly',
+                        "\n${indentation}compile '$d.module.id'")
+            }
+            else if(transitiveSize > 1) {
+                addViolationInsert(call, 'one or more classes in these transitive dependencies are required by your code directly',
+                        transitiveDependenciesToAddAsFirstOrder.inject('') {
+                            deps, d -> deps + "\n${indentation}compile '$d.module.id'"
+                        })
+            }
+        }
     }
 }

src/test/groovy/com/netflix/nebula/lint/rule/dependency/DependencyClassVisitorSpec.groovy

@@ -1,6 +1,8 @@
 package com.netflix.nebula.lint.rule.dependency
 
-import org.gradle.api.artifacts.ModuleVersionIdentifier
+import org.gradle.api.artifacts.ResolvedDependency
+import org.gradle.api.internal.artifacts.DefaultModuleVersionIdentifier
+import org.gradle.api.internal.artifacts.DefaultResolvedDependency
 import spock.lang.Specification
 import spock.lang.Unroll
 
@@ -12,6 +14,7 @@ class DependencyClassVisitorSpec extends Specification {
             package a;
             public class A {
                 public static A create() { return new A(); }
+                public static void sideEffect() { }
             }
         ''')
 
@@ -139,6 +142,7 @@ class DependencyClassVisitorSpec extends Specification {
         'A[] a = new A[0]'              | true
         'A a = A.create()'              | true
         'A.create()'                    | true
+        'A.sideEffect()'                | true
 
         // type erasure prevents the following two references from being observable from ASM
         'List<A> a = new ArrayList()'   | false
@@ -149,14 +153,16 @@ class DependencyClassVisitorSpec extends Specification {
         'A a = null'                    | false
     }
 
-    def 'references are found on annotations'() {
+    @Unroll
+    def 'references are found on #type annotations'() {
         setup:
         java.compile('''
             package a;
             import java.lang.annotation.*;
 
             @Retention(RetentionPolicy.RUNTIME)
-            @Target({ElementType.TYPE})
+            @Target({ElementType.TYPE, ElementType.FIELD, ElementType.METHOD, ElementType.PARAMETER,
+                ElementType.CONSTRUCTOR, ElementType.LOCAL_VARIABLE})
             public @interface AAnnot {
             }
         ''')
@@ -166,15 +172,26 @@ class DependencyClassVisitorSpec extends Specification {
             package b;
             import a.*;
 
-            @AAnnot
-            public class B {
-            }
+            $annotation
         """)
 
         def a1 = gav('netflix', 'a', '1')
 
+        println(java.traceClass('b.B'))
+
         then:
         java.containsReferenceTo('b.B', ['a/AAnnot': [a1].toSet()], a1)
+
+        where:
+        annotation                                                                          | type
+        '''@AAnnot public class B { }'''                                                    | 'class'
+        '''public class B { @AAnnot public void foo() {} }'''                               | 'method'
+        '''public class B { @AAnnot Object field; }'''                                      | 'field'
+        '''public class B { public void foo(@AAnnot Object p) {} }'''                       | 'param'
+        '''public class B { @AAnnot public B() {} }'''                                      | 'constructor'
+
+        // Local variable annotations don't appear to be stored in the bytecode
+//        '''public class B { public void foo() { @AAnnot Object o = new Object(); } }'''     | 'local field'
     }
 
     def 'references are found on throws'() {
@@ -202,5 +219,7 @@ class DependencyClassVisitorSpec extends Specification {
         java.containsReferenceTo('b.B', ['a/AException': [a1].toSet()], a1)
     }
 
-    ModuleVersionIdentifier gav(String g, String a, String v) { [version: v, group: g, name: a] as ModuleVersionIdentifier }
+    ResolvedDependency gav(String g, String a, String v) {
+        new DefaultResolvedDependency(new DefaultModuleVersionIdentifier(g, a, v), 'compile')
+    }
 }

src/test/groovy/com/netflix/nebula/lint/rule/dependency/JavaFixture.groovy

@@ -1,6 +1,6 @@
 package com.netflix.nebula.lint.rule.dependency
 
-import org.gradle.api.artifacts.ModuleVersionIdentifier
+import org.gradle.api.artifacts.ResolvedDependency
 import org.gradle.api.logging.Logger
 import org.objectweb.asm.ClassReader
 import org.objectweb.asm.util.TraceClassVisitor
@@ -47,8 +47,8 @@ class JavaFixture {
                 new TraceClassVisitor(new PrintWriter(System.out)), ClassReader.SKIP_DEBUG)
     }
 
-    boolean containsReferenceTo(String source, Map<String, Set<ModuleVersionIdentifier>> referenceMap,
-                                ModuleVersionIdentifier refersTo) {
+    boolean containsReferenceTo(String source, Map<String, Set<ResolvedDependency>> referenceMap,
+                                ResolvedDependency refersTo) {
         def visitor = new DependencyClassVisitor(referenceMap, [isDebugEnabled: { true }, debug: { d -> println d }] as Logger)
         new ClassReader(inMemoryClassFileManager.classBytes(source) as byte[]).accept(visitor, ClassReader.SKIP_DEBUG)
         return visitor.references.contains(refersTo)