Add checkerframework and errorprone - #2941
Conversation
|
@vlsi Any feedback about the 2 tools and how I added them in the build (even if I was inspired by what you did on pg-driver)? @krmahadevan WDYT? They highlight a lot of minor issues but some potential problems too. |
|
@juherr - This is definitely a good addition. I am yet to explore both these tools. I have already seen error prone being used by jhipster when it scaffolds a new project based on my inputs. Couple of questions:
|
|
Thanks.
As I understand, yes. It provides extra data in bytecode thanks to the annotations.
To be honest, I hoped there were a bit less. That's why I sent the draft before fixing them all. |
|
"nullability" is not trivial, so it might take some time to properly annotate and identify nullable types. |
| plugins.withId("org.checkerframework") { | ||
| configure<CheckerFrameworkExtension> { |
There was a problem hiding this comment.
plugins { id("org.checkerframework") } adds checkerframework plugin, so plugins.withId(..) is not needed here.
Something like checkerframework {...} should do.
| */ | ||
| public static void assertEquals(Object[] actual, Object[] expected, String message) { | ||
| public static void assertEquals( | ||
| @Nullable Object[] actual, @Nullable Object[] expected, @Nullable String message) { |
There was a problem hiding this comment.
I believe it should be @Nullable Object @Nullable [] actual: nullable array of nullable elements
| */ | ||
| @Deprecated | ||
| default Object newInstance(Class<?> cls) { | ||
| default Object newInstance(@NonNull Class<?> cls) { |
There was a problem hiding this comment.
It would be better to use package-level "default not null" annotation rather than @NotNull: https://github.com/pgjdbc/pgjdbc/blob/aff581fe806f1c1cdc8bcd9e5a981e37691b44b5/pgjdbc/src/main/java/org/postgresql/package-info.java#L6-L8
See explanation in https://checkerframework.org/manual/#climb-to-top
|
In my experience, checkerframework nullability verification takes time, so I tend to activate it only when See https://github.com/apache/jmeter/blob/c94db9741f0547235f9a67b1ece40389add6b742/build-logic/build-parameters/build.gradle.kts#L77 , and https://github.com/apache/jmeter/blob/c94db9741f0547235f9a67b1ece40389add6b742/build-logic/verification/src/main/kotlin/build-logic.style.gradle.kts#L42-L43 (conditional activation of |
|
Thanks for the feedbacks @vlsi ❤️ |
|
Updated according to reviews. All issues are not fixed yet. Still 2 gradle issues: not succeeding in using the parameters plugin + not finding the way to merge package-info |
| * @param message the assertion error message | ||
| */ | ||
| public static void assertEquals(Object actual, Object expected, String message) { | ||
| public static void assertEquals(Object actual, Object expected, @Nullable String message) { |
There was a problem hiding this comment.
| public static void assertEquals(Object actual, Object expected, @Nullable String message) { | |
| public static void assertEquals(@Nullable Object actual, @Nullable Object expected, @Nullable String message) { |
| * @param message the assertion error message | ||
| */ | ||
| public static void assertEquals(Double actual, Double expected, String message) { | ||
| public static void assertEquals(Double actual, Double expected, @Nullable String message) { |
There was a problem hiding this comment.
| public static void assertEquals(Double actual, Double expected, @Nullable String message) { | |
| public static void assertEquals(@Nullable Double actual, @Nullable Double expected, @Nullable String message) { |
| * @param message the assertion error message | ||
| */ | ||
| public static void assertEquals(Float actual, Float expected, String message) { | ||
| public static void assertEquals(Float actual, Float expected, @Nullable String message) { |
There was a problem hiding this comment.
| public static void assertEquals(Float actual, Float expected, @Nullable String message) { | |
| public static void assertEquals(@Nullable Float actual, @Nullable Float expected, @Nullable String message) { |
| * @param expected the expected value | ||
| */ | ||
| public static void assertEquals(Iterator<?> actual, Iterator<?> expected) { | ||
| public static void assertEquals(@Nullable Iterator<?> actual, @Nullable Iterator<?> expected) { |
There was a problem hiding this comment.
| public static void assertEquals(@Nullable Iterator<?> actual, @Nullable Iterator<?> expected) { | |
| public static void assertEquals(@Nullable Iterator<@Nullable ?> actual, @Nullable Iterator<@Nullable ?> expected) { |
| */ | ||
| static boolean wasFailureDueToTimeout(ITestResult result) { | ||
| Throwable cause = result.getThrowable(); | ||
| @Nullable Throwable cause = result.getThrowable(); |
There was a problem hiding this comment.
Technically speaking, @Nullable should be automatically inferred for local variables, so this could probably be omitted
| @Nullable Throwable cause = result.getThrowable(); | |
| Throwable cause = result.getThrowable(); |
There was a problem hiding this comment.
I suppose you're right.
It means I should not use TypeUseLocation.ALL in @DefaultQualifier(value = NonNull.class, locations = TypeUseLocation.ALL)
Any other option I should exclude? https://checkerframework.org/api/org/checkerframework/framework/qual/TypeUseLocation.html
| static List<ClassLoader> appendContextualClassLoaders(List<ClassLoader> currentLoaders) { | ||
| List<ClassLoader> allClassLoaders = Lists.newArrayList(); | ||
| ClassLoader contextClassLoader = Thread.currentThread().getContextClassLoader(); | ||
| @Nullable ClassLoader contextClassLoader = Thread.currentThread().getContextClassLoader(); |
There was a problem hiding this comment.
| @Nullable ClassLoader contextClassLoader = Thread.currentThread().getContextClassLoader(); | |
| ClassLoader contextClassLoader = Thread.currentThread().getContextClassLoader(); |
?
| } else { | ||
| m.addAll(extractedMethod.getValue()); | ||
| } | ||
| @Nullable Class<?> parent = clazz.getSuperclass(); |
There was a problem hiding this comment.
| @Nullable Class<?> parent = clazz.getSuperclass(); | |
| Class<?> parent = clazz.getSuperclass(); |
|
|
||
| public ConstructorOrMethod(Method m) { | ||
| m_method = m; | ||
| m_constructor = null; |
There was a problem hiding this comment.
Default value is null anyway, so does it make sense to assign it with null?
|
I would suggest merging |
|
@vlsi Thanks for the review. I think your suggestion is a good strategy and I will make another PR with errorprone only. Any idea why I'm not able to use build-parameters? The generated plugin is not found here https://github.com/testng-team/testng/pull/2941/files#diff-5bb00f6e0b0be0ec62ea3824c3888e27921683447cea9fb16bdd9f859f7881acR5 |
TestNG has no nullness checking, so whether a method may return null is only discoverable by reading it. NullAway plugs into the Error Prone pass that already runs on every module, and OnlyNullMarked confines it to code declaring JSpecify's @NullMarked. No package declares it yet, so this commit touches no Java source and reports nothing. That scoping is the point. Nullness can now be adopted package by package, each step green on its own, instead of as a single sweep over the whole API. The earlier attempt in testng-team#2941 used the Checker Framework, which analyses every file of the compilation unit regardless of what the package opted into, and stalled on the size of the resulting diff. @NullMarked applies to a package rather than to a source set, and nine test packages share a name with a main one, so NullAway stays off for test compilation: marking org.testng or org.testng.internal would otherwise sweep in their test halves. JSpecify ships as a regular api dependency rather than compileOnly: its annotations are runtime-retained, so hiding them would strip nullness information from the published artifact instead of passing it to consumers.
TestNG has no nullness checking, so whether a method may return null is only discoverable by reading it. NullAway plugs into the Error Prone pass that already runs on every module, and OnlyNullMarked confines it to code declaring JSpecify's @NullMarked. No package declares it yet, so this commit touches no Java source and reports nothing. That scoping is the point. Nullness can now be adopted package by package, each step green on its own, instead of as a single sweep over the whole API. The earlier attempt in testng-team#2941 used the Checker Framework, which analyses every file of the compilation unit regardless of what the package opted into, and stalled on the size of the resulting diff. @NullMarked applies to a package rather than to a source set, and nine test packages share a name with a main one, so NullAway stays off for test compilation: marking org.testng or org.testng.internal would otherwise sweep in their test halves. JSpecify ships as a regular api dependency rather than compileOnly: its annotations are runtime-retained, so hiding them would strip nullness information from the published artifact instead of passing it to consumers.
TestNG has no nullness checking, so whether a method may return null is only discoverable by reading it. NullAway plugs into the Error Prone pass that already runs on every module, and OnlyNullMarked confines it to code declaring JSpecify's @NullMarked. No package declares it yet, so this commit touches no Java source and reports nothing. That scoping is the point. Nullness can now be adopted package by package, each step green on its own, instead of as a single sweep over the whole API. The earlier attempt in #2941 used the Checker Framework, which analyses every file of the compilation unit regardless of what the package opted into, and stalled on the size of the resulting diff. @NullMarked applies to a package rather than to a source set, and nine test packages share a name with a main one, so NullAway stays off for test compilation: marking org.testng or org.testng.internal would otherwise sweep in their test halves. JSpecify ships as a regular api dependency rather than compileOnly: its annotations are runtime-retained, so hiding them would strip nullness information from the published artifact instead of passing it to consumers.
TestNG has no nullness checking, so whether a method may return null is only discoverable by reading it. NullAway plugs into the Error Prone pass that already runs on every module, and OnlyNullMarked confines it to code declaring JSpecify's @NullMarked. No package declares it yet, so this commit touches no Java source and reports nothing. That scoping is the point. Nullness can now be adopted package by package, each step green on its own, instead of as a single sweep over the whole API. The earlier attempt in #2941 used the Checker Framework, which analyses every file of the compilation unit regardless of what the package opted into, and stalled on the size of the resulting diff. @NullMarked applies to a package rather than to a source set, and nine test packages share a name with a main one, so NullAway stays off for test compilation: marking org.testng or org.testng.internal would otherwise sweep in their test halves. JSpecify ships as a regular api dependency rather than compileOnly: its annotations are runtime-retained, so hiding them would strip nullness information from the published artifact instead of passing it to consumers.
3 participants
Try checkerframework and errorprone