Detect and allow failing for unused/undeclared dependencies #62

betafood · 2017-12-19T19:48:54Z

This adds tracking on what dependencies are referenced in the jdt compiler.

ifedorenko

Thank you for the contribution, Ben. I think this is a useful addition to takari-lifecycle and agree with implementation approach. There are few code-polish issues, but nothing major. Let me know if you have any questions.

ifedorenko · 2018-01-26T13:34:38Z

...ecycle-plugin/src/main/java/io/takari/maven/plugins/compile/javac/AbstractCompilerJavac.java

@@ -273,6 +273,11 @@ public final int compile() throws MojoExecutionException, IOException {
    return compile(files);
  }

+  @Override
+  public Set<String> getReferencedClasspathEntries() {


Don't silently ignore unsupported policy value, make it clear to the users that unusedDeclaredDependencies=error only works with JDT-based compiler backend. See how this is done for transitiveDependencyReference and privatePackageReference (or just move unusedDeclaredDependency to CompilerJdt)

ifedorenko · 2018-01-26T13:43:15Z

takari-lifecycle-plugin/src/main/java/io/takari/maven/plugins/compile/AbstractCompileMojo.java

+    List<File> processorpath = getProcessorpath();
+
+    // Find the equivalent artifact equivalent for each referenced entry
+    for (String entry : referencedEntries) {


This is N^2 loop and needs optimization. For a project with 3000 dependencies it'll run 9000000 (9 million) iterations. I'd change #getClasspath() to return Map<File,Artifact> and then do single for-each referencedEntries to match entries to classpath artifacts.

ifedorenko · 2018-01-26T13:52:01Z

takari-lifecycle-plugin/src/main/java/io/takari/maven/plugins/compile/AbstractCompileMojo.java

+      if (unusedDeclaredDependency == AccessRulesViolation.error) {
+        context.addPomMessage("The following dependencies are declared but are not used: " + unusedArtifacts, MessageSeverity.ERROR, null);
+      } else {
+        log.warn(pom.getAbsolutePath() + ": The following dependencies are declared but are not used: {}", unusedArtifacts);


ignore means ignore, don't log anything here. actually, don't execute the check at all if unusedDeclaredDependency=ignore.

ifedorenko · 2018-01-26T13:57:23Z

...-lifecycle-plugin/src/main/java/io/takari/maven/plugins/compile/jdt/classpath/Classpath.java

    NameEnvironmentAnswer suggestedAnswer = null;
    Collection<ClasspathEntry> entries = !packageName.isEmpty() ? packages.get(packageName) : this.entries;
    if (entries != null) {
      for (ClasspathEntry entry : entries) {
        NameEnvironmentAnswer answer = entry.findType(packageName, typeName);
        if (answer != null) {
+          used = entry.getEntryDescription();


entry.getEntryDescription() is for humans to read, introduce new Path entry.getLocation() to get classpath entry location for further processing.

ifedorenko · 2018-01-26T13:59:18Z

...-lifecycle-plugin/src/main/java/io/takari/maven/plugins/compile/jdt/classpath/Classpath.java

@@ -102,4 +116,8 @@ public void reset() {
  public List<ClasspathEntry> getEntries() {
    return entries;
  }
+
+  public Set<String> getReferencedEntries() {


Should return Set<Path>

ifedorenko · 2018-01-26T14:11:16Z

...ecycle-plugin/src/test/java/io/takari/maven/plugins/compile/jdt/CompileJdtClasspathTest.java

+      mojos.compile(moduleA, unused);
+      fail();
+    } catch (MojoExecutionException e) {
+      assertTrue(e.getLocalizedMessage().contains("The following dependencies are declared but are not used: [test:module-b:jar:1.0:compile]"));


Don't assert the exception, use CompileRule.assertMessages to assert expected message was created on the pom file.

ifedorenko · 2018-01-26T14:13:16Z

...cts/compile-jdt-classpath/reactor-unused/module-b/src/main/java/reactor/moduleb/ModuleB.java

@@ -0,0 +1,5 @@
+package reactor.moduleb;


remove unused test file

ifedorenko · 2018-01-26T14:13:22Z

...ile-jdt-classpath/reactor-unused/module-b/src/main/java/reactor/moduleb/ModuleB.java-comment

@@ -0,0 +1,5 @@
+package reactor.moduleb;


remove unused test file

ifedorenko · 2018-01-26T14:13:30Z

...pile-jdt-classpath/reactor-unused/module-b/src/main/java/reactor/moduleb/ModuleB.java-method

@@ -0,0 +1,8 @@
+package reactor.moduleb;


remove unused test file

ifedorenko · 2018-01-26T14:14:14Z

takari-lifecycle-plugin/src/test/projects/compile-jdt-classpath/reactor-unused/pom.xml

@@ -0,0 +1,14 @@
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"


remove unused test file

betafood

Thanks for all the comments. I've made a new commit to address them. I also added a few questions here.

betafood · 2018-01-26T22:16:05Z

...-lifecycle-plugin/src/main/java/io/takari/maven/plugins/compile/jdt/classpath/Classpath.java

@@ -117,7 +115,7 @@ public void reset() {
    return entries;
  }

-  public Set<String> getReferencedEntries() {
+  public Set<Path> getReferencedEntries() {
    return referencedentries;


Would it make sense to use File here to have parity with File in AbstractCompiler?

Currently Classpath uses Path, and CompilerJdt translates between Path and File. Beware, however, that under java 9 system libraries will not have corresponding Files

betafood · 2018-01-26T22:27:36Z

takari-lifecycle-plugin/src/main/java/io/takari/maven/plugins/compile/AbstractCompileMojo.java

          undeclaredArtifacts.add(artifact);
        }
      }
    }

-    // Log or fail the build for any referenced artifacts that are not declared as a direct dependency
+    // Fail the build for any referenced artifacts that are not declared as a direct dependency
    if (!undeclaredArtifacts.isEmpty()) {
      if (unusedDeclaredDependency == AccessRulesViolation.error) {


Looking at this again. Would it make sense to have a separate AccessRulesViolation for "undeclared" vs "unused" artifacts? To fail only on undeclared or unused rather than failing on any combination of the two. Although, I guess technically speaking, "undeclared" is almost the same thing as transitiveDependencyReference.

You are right, I missed it.

checkDependencyReferences should really be checkUnusedDependencies and only check for unused dependencies declared in project pom.xml (or inherited from the parent).

Existing transitiveDependencyReference policy already covers references to transitive/undeclared dependencies. No need duplicate functionality. I'll update review in the code too.

ifedorenko · 2018-01-26T23:10:13Z

takari-lifecycle-plugin/src/main/java/io/takari/maven/plugins/compile/AbstractCompileMojo.java

+
+    // Find the equivalent artifact equivalent for each referenced entry
+    for (File entry : allReferencedEntries) {
+      Artifact artifact = getClasspath().get(entry);


This unnecessary calculates new classpath map for each referenced entry. At least pull #getClasspath() out of the loop. Better yet, pass classpath map as a parameter, so the same map is used during compilation and when checking for unused dependencies. (which will also ensure the two classpaths are the same)

ifedorenko · 2018-01-26T23:12:12Z

takari-lifecycle-plugin/src/main/java/io/takari/maven/plugins/compile/AbstractCompileMojo.java

+
+    // Fail the build for any referenced artifacts that are not declared as a direct dependency
+    if (!undeclaredArtifacts.isEmpty()) {
+      if (unusedDeclaredDependency == AccessRulesViolation.error) {


unusedDeclaredDependency == AccessRulesViolation.error is redundant here.

Actually, remove this branch altogether. transitiveDependencyReference policy already provides mechanism to block use of transitive dependencies, and that mechanism also provides more targeted error messages.

ifedorenko · 2018-01-26T23:12:43Z

takari-lifecycle-plugin/src/main/java/io/takari/maven/plugins/compile/AbstractCompileMojo.java

-  private List<File> getClasspath() {
-    List<File> classpath = new ArrayList<File>();
+  private Map<File, Artifact> getClasspath() {
+    Map<File, Artifact> classpath = new LinkedHashMap<File, Artifact>();


Please use diamond operator.

ifedorenko · 2018-01-26T23:24:47Z

...in/src/main/java/io/takari/maven/plugins/compile/jdt/classpath/DependencyClasspathEntry.java

@@ -121,6 +122,11 @@ public String getEntryDescription() {
    return sb.toString();
  }

+  @Override
+  public Path getLocation() {
+    return file.toPath();


What branch are you using? #file has type Path in the current master.

My fork is 5 commits behind master, so I'll have to rebase the branch.

ifedorenko · 2018-01-26T23:27:00Z

takari-lifecycle-plugin/src/main/java/io/takari/maven/plugins/compile/AbstractCompileMojo.java

@@ -419,6 +431,9 @@ public void execute() throws MojoExecutionException, MojoFailureException {
        log.info("Compiling {} sources to {}", sources.size(), getOutputDirectory());
        int compiled = compiler.compile();
        log.info("Compiled {} out of {} sources ({} ms)", compiled, sources.size(), stopwatch.elapsed(TimeUnit.MILLISECONDS));
+        if (unusedDeclaredDependency != AccessRulesViolation.ignore && compiler.getReferencedClasspathEntries() != null && !compiler.getReferencedClasspathEntries().isEmpty()) {


Check compiler.getReferencedClasspathEntries() != null && !compiler.getReferencedClasspathEntries().isEmpty() is redundant here.

ifedorenko · 2018-01-26T23:27:49Z

takari-lifecycle-plugin/src/main/java/io/takari/maven/plugins/compile/AbstractCompileMojo.java

+
+    // Fail the build for any direct dependencies that are never referenced
+    if (!unusedArtifacts.isEmpty()) {
+      if (unusedDeclaredDependency == AccessRulesViolation.error) {


unusedDeclaredDependency == AccessRulesViolation.error is redundant here.

ifedorenko · 2018-01-26T23:34:36Z

takari-lifecycle-plugin/src/main/java/io/takari/maven/plugins/compile/AbstractCompiler.java

@@ -59,6 +61,8 @@

  private AccessRulesViolation privatePackageReference;

+  private AccessRulesViolation unusedDeclaredDependency;


I think unusedDeclaredDependency and corresponding getter are redundant.

ifedorenko · 2018-01-26T23:35:39Z

takari-lifecycle-plugin/src/main/java/io/takari/maven/plugins/compile/AbstractCompiler.java

+    this.unusedDeclaredDependency = unusedDeclaredDependency;
+  }
+
+  protected Set<File> getReferencedClasspathEntries() {


I'd define getReferencedClasspathEntries() as abstract here and throw UnsupportedOperationException in Javac compiler backend.

ifedorenko · 2018-01-26T23:50:10Z

takari-lifecycle-plugin/src/main/java/io/takari/maven/plugins/compile/AbstractCompileMojo.java

@@ -433,18 +448,70 @@ public void execute() throws MojoExecutionException, MojoFailureException {
    }
  }

+  private void checkDependencyReferences(Set<File> referencedEntries) throws MojoExecutionException, IOException {


Should be called checkUnusedDependencies.

ifedorenko · 2018-01-26T23:54:15Z

takari-lifecycle-plugin/src/main/java/io/takari/maven/plugins/compile/AbstractCompileMojo.java

+
+    // Fail the build for any referenced artifacts that are not declared as a direct dependency
+    if (!undeclaredArtifacts.isEmpty()) {
+      if (unusedDeclaredDependency == AccessRulesViolation.error) {


Actually, remove this branch altogether. transitiveDependencyReference policy already provides mechanism to block use of transitive dependencies, and that mechanism also provides more targeted error messages.

betafood · 2018-01-27T02:23:06Z

Thanks for the quick review and answers to my questions. Added another commit to address your comments(along with rebasing the branch on the latest master).

ifedorenko · 2018-01-27T15:01:51Z

This change fails when running the build under java 9. I haven't looked closely, but suspect CompilerJdt#getReferencedClasspathEntries() needs to disregard references to jdk system libraries. In other words, it should only return files passed to the compiler via #setClasspath(...) and ignore anything else. Note that you need to rebase on latest master to be able to build with java 9.

I also would like to see an automated test for the following scenario:

two projects, project-a and project-b, project-a depends on project-b;
project-b has single class b.B
project-a has two classes, a.A1 and a.A2, class a.A1 references class b.B from project-b.
initial full compile of project-a is a success
a.A2 is changed is some way (just don't add dependency on b.B)
incremental compile of project-a is success, and only a.A2 is recompiled.

* refactor checking logic to remove n^2 loop * setting unusedDeclaredDependency to ignore skips check logic * use Path & File over String for referenced entries * Move test to CompileTest * Remove unused test files

* removed undeclared dependency checking * remove redundant checking, calls, and methods

betafood · 2018-01-29T18:28:36Z

modules/java.base is coming in as a JrtPath which toFile throws an UnsupportedOperationException when called. Within getReferencedEntries, the toFile calls were changed to return null on UnsupportedOperationException and then an additional filter was called to filter out any null values.

I also made a change for unusedDeclaredDependencies to only check in a full build context. Writing the test you detailed had it failing for unused declared dependencies for incremental builds(since the classes that did reference the dependency did not have to be built again).

ifedorenko · 2018-01-30T03:30:32Z

takari-lifecycle-plugin/src/main/java/io/takari/maven/plugins/compile/jdt/CompilerJdt.java

+      return path.toFile();
+    } catch (UnsupportedOperationException e) {
+      // Java 9 support
+      return null;


This does not feel right, to be honest. I think it will be cleaner to remember classpath dependencies as Set<Path> and filter referenced dependencies based on membership in the set.

ifedorenko · 2018-01-30T03:36:15Z

takari-lifecycle-plugin/src/test/java/io/takari/maven/plugins/compile/CompileTest.java

+
+  @Test
+  public void testReferencedEntries() throws Exception {
+    final String javacMessage = "Compiler javac does not support unusedDeclaredDependency=error, use compilerId=jdt";


Use Assume.assumeTrue(CompilerJdt.ID.equals(compilerId)); to only run the test with JDT compiler backend. Validating javac error message pollutes the test and makes it more difficult to understand.

ifedorenko · 2018-01-30T03:39:00Z

takari-lifecycle-plugin/src/test/java/io/takari/maven/plugins/compile/CompileTest.java

+      mojos.assertBuildOutputs(parent, "a/target/classes/a/A2.class");
+    } catch (UnsupportedOperationException e) {
+      ErrorMessage.isMatch(e.getMessage(), javacMessage);
+    }


I think for completeness sake this test should also remove a.A1->b.B dependency and assert that only a.A1 got recompiled and that unused dependency error was reported.

ifedorenko · 2018-01-30T03:56:40Z

takari-lifecycle-plugin/src/main/java/io/takari/maven/plugins/compile/AbstractCompileMojo.java

@@ -419,6 +431,9 @@ public void execute() throws MojoExecutionException, MojoFailureException {
        log.info("Compiling {} sources to {}", sources.size(), getOutputDirectory());
        int compiled = compiler.compile();
        log.info("Compiled {} out of {} sources ({} ms)", compiled, sources.size(), stopwatch.elapsed(TimeUnit.MILLISECONDS));
+        if (unusedDeclaredDependency != AccessRulesViolation.ignore && context.isEscalated()) {


-1

&& context.isEscalated() here will result in errors reported during full build "magically" disappear during incremental build, only to reappear during the next full build or when modifying some sources but not others.

We need to find a way to make unusedDeclaredDependency=error work during incremental build. I think the only way to do this is to track dependencies used by each source, record this information in build context and use it during subsequent incremental builds. I don't know how easy this is to implement.

Do you see any other way to correctly support incremental build?

True. I did not consider that scenario, and I agree with what you said. I'll remove this change.

I do not see another way from what you detailed. Since there is a need of persistence of the set of dependencies referenced between builds, I believe to keep that set of dependencies accurate, each source's list of dependencies would need to be tracked.
Along those lines, I'm wondering if something like a Map<Path, Set<Artifact>> where at the end of each build, any sources that were built would update the map would work (or the reverse with each dependency listing each source that references it where any that are still empty by the end of the build are considered unused).

ifedorenko · 2018-02-01T12:34:46Z

Here is another incremental build test I'd like to see

a project has two differently named but otherwise identical dependencies. project sources use classpath from the dependencies. the second dependency is marked as unused
the first dependency is removed. incremental build is run.
the correct behaviour is to remove unused error from now the only project dependency, but I suspect the errors will not be removed

Compiler uses observable class features (like members or class modifiers) to determine how what project sources needs to be recompiled after classpath dependency change. Removing redundant dependency does not change set of available types, so nothing is recompiled. Moving classes from one dependency to another will have the same effect.

I don't have a good proposal how to solve this.

ifedorenko requested changes Jan 26, 2018

View reviewed changes

betafood commented Jan 26, 2018

View reviewed changes

ifedorenko requested changes Jan 26, 2018

View reviewed changes

betafood force-pushed the unused-undeclared-dependency branch from a9a865c to 4e2a9db Compare January 27, 2018 02:22

Benjamin added 5 commits January 29, 2018 01:22

Detect and allow failing for unused/undeclared dependencies

fff3d98

Use canonicalPath over absolutePath for comparison

bd27e63

Cleanup unusedDeclaredDependency logic

b1da3b5

* refactor checking logic to remove n^2 loop * setting unusedDeclaredDependency to ignore skips check logic * use Path & File over String for referenced entries * Move test to CompileTest * Remove unused test files

Refactor unused dependencies logic

95b36b3

* removed undeclared dependency checking * remove redundant checking, calls, and methods

Add compatability with java9 for unusedDeclaredDependencies flag

81593db

betafood force-pushed the unused-undeclared-dependency branch from 4e2a9db to 81593db Compare January 29, 2018 18:17

ifedorenko requested changes Jan 30, 2018

View reviewed changes

		@@ -0,0 +1,14 @@
		<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"

		@@ -59,6 +61,8 @@

		private AccessRulesViolation privatePackageReference;

		private AccessRulesViolation unusedDeclaredDependency;

Detect and allow failing for unused/undeclared dependencies #62

Are you sure you want to change the base?

Detect and allow failing for unused/undeclared dependencies #62

Conversation

betafood commented Dec 19, 2017

ifedorenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ifedorenko Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

ifedorenko Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

ifedorenko Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

ifedorenko Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

betafood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

betafood commented Jan 27, 2018

ifedorenko commented Jan 27, 2018

betafood commented Jan 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ifedorenko commented Feb 1, 2018

ifedorenko Jan 26, 2018 •

edited

Loading

ifedorenko Jan 26, 2018 •

edited

Loading

ifedorenko Jan 26, 2018 •

edited

Loading

ifedorenko Jan 26, 2018 •

edited

Loading