ErrorProneInjector incorrectly picks up the no-args constructor

[ksobolew]

The logic of determining the constructor of a checker class changed in commit 1c3c09f. Previously it explicitly checked for a constructor with a single parameter of type ErrorProneFlags; now it looks for a constructor with all parameters having this type. The issue is that the condition used - allMatch - also returns true if there are no parameters at all, so the no-args constructor also matches the test.

The impact is that when I have a test for a checker which needs to take additional flags, the CompilationTestHelper uses the wrong constructor to create the checker instance, making it impossible to pass flags in a test.

[Stephan202]

@ksobolew this sounds similar to something that initially puzzled me when upgrading to Error Prone 2.19+. Then I realized that just annotating the ErrorProneFlags-accepting constructors with @Inject suffices. See the changes in PicnicSupermarket/error-prone-support#621 for an example. Hope this helps!

[ksobolew]

You're right @Stephan202, there's also some support for @Inject, so I think it will work for me too. But I still think this could be fixed, perhaps even with something like this:

diff --git a/check_api/src/main/java/com/google/errorprone/scanner/ErrorProneInjector.java b/check_api/src/main/java/com/google/errorprone/scanner/ErrorProneInjector.java
index 7a3c79cee5..afd2570d87 100644
--- a/check_api/src/main/java/com/google/errorprone/scanner/ErrorProneInjector.java
+++ b/check_api/src/main/java/com/google/errorprone/scanner/ErrorProneInjector.java
@@ -18,6 +18,7 @@ package com.google.errorprone.scanner;
 
 import static com.google.common.collect.Lists.reverse;
 import static java.util.Arrays.stream;
+import static java.util.Comparator.comparingInt;
 import static java.util.stream.Collectors.joining;
 
 import com.google.common.collect.ClassToInstanceMap;
@@ -106,7 +107,7 @@ public final class ErrorProneInjector {
     return stream(clazz.getDeclaredConstructors())
         .filter(predicate)
         .map(c -> (Constructor<T>) c)
-        .findFirst();
+        .max(comparingInt(Constructor::getParameterCount));
   }
 
   private static String printPath(List<Class<?>> path) {

[Stephan202]

Check; I suppose that's something for @graememorgan to have an opinion on :)

[graememorgan]

Hah, I think I thought of that edge case when I wrote it, but decided it wouldn't matter for some reason. :)

I would expect the inject constructor to be annotated @Inject and therefore win.

The impact is that when I have a test for a checker which needs to take additional flags, the CompilationTestHelper uses the wrong constructor to create the checker instance, making it impossible to pass flags in a test.

Could you provide a minimal example which is a problem?

[Stephan202]

Could you provide a minimal example which is a problem?

IIRC ~any test that provides a non-vacuous set of flags using setArgs should trigger the issue.

One example:

git clone [email protected]:PicnicSupermarket/error-prone-support.git
cd error-prone-support
sed -i '/Inject/d' error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversion.java
mvn clean install -Dverification.skip -DskipTests
mvn test -Dtest='RedundantStringConversionTest#identificationOfCustomConversionMethod' -pl error-prone-contrib

[graememorgan]

sed -i '/Inject/d' error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversion.java

But can it be fixed by just including the @Inject? Ideally we'd like @Inject to be used for the injectable constructor.

(I'm not suggesting not fixing this to match the original behaviour, but chucking some @Injects around would be good anyway.)

[Stephan202]

Indeed, at least in our project adding @Inject to these non-nullary constructors solved the issue in each case 👍

[ksobolew]

Yes, it can be fixed this way. I originally didn't realize that, so the severity is much lower than I thought. Now it's just mostly a source of confusion :)

[Stephan202]

The warning-level InjectOnBugCheckers check should help, though many may have disabled it due to the false-positives it reported in an earlier version. Perhaps adding a hint to the release notes could also help. :)

[graememorgan]

I've mailed a change internally to favour an EPF constructor over a no-arg one.

[graememorgan]

Should be fixed now; give me a shout about any further problems.