Enhance the NullAway configuration - #68
Open
ThoSap wants to merge 2 commits into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Enhance the NullAway configuration
Two changes that reinforce each other:
errorprone.args. All four measure zero findings.AcknowledgeRestrictiveAnnotationshas real data to work with instead of guessing.See the NullAway wiki, Adopting NullAway, Configuration and JSpecify Support for details.
Why now
The NullAway 0.14.1 configuration surface has 39 options. Only two were set.
I read the full list out of
ErrorProneCLIFlagsConfigin the 0.14.1 sources and measured every candidate that could apply to a JSpecify project, rather than picking from the wiki by eye.What changed
enable additional NullAway checks in errorprone.args# Nullnesssectionconfigure jOOQ code-generation to generate nullability annotationsgenerated/build.gradle.kts, the regenerated sources, and a README sectionThe four options
AcknowledgeRestrictiveAnnotations@Nullableannotation found in unannotated code, instead of ignoring itCheckOptionalEmptinessOptional.get()where the value was never checked as presentOptionalappears in 7 main source filesExhaustiveOverrideOverrideannotationMissingOverrideruns at ERROR, which guarantees the annotation is present. This is the setting the wiki page already linked frombuild.gradle.ktsdescribesHandleTestAssertionLibrariesassertThat(x).isNotNull()establishes non-nullnessThe jOOQ codegen change
Every generated accessor now carries the real nullness of its column, so NullAway models the persistence layer instead of assuming it.
One caveat is recorded in the README.
jOOQ does not officially support the
TYPE_USEpositioning that JSpecify requires (jOOQ#10759).The positioning is correct here only because the generated code uses no generics, collections, maps, arrays or forced types with inner classes.
If the
includeslist ever grows such an object, the annotation positions need a review.That review will not stay manual. NullAway's wiki documents a
JSpecifyUnrecognizedAnnotationLocationchecker that reports a JSpecify annotation in any location the specification does not read,and ships a suggested fix so Error Prone's patch mode can correct them in bulk.
It is documented as version 0.14.2, and 0.14.2 is not released - 0.14.1 is the latest, and the class is absent from its sources.
The wiki tracks master. Worth enabling the moment it ships, because it checks exactly the property this caveat depends on.
Considered and not adopted
TreatGeneratedAsUnannotated- rejected, and it would now do harmIt was in the first draft of this PR and came out again. Two independent reasons:
:generated:compileJavadoes not fail onRequireExplicitNullMarking, since no generated class carries@NullMarked.It is not.
RequireExplicitNullMarkingis a standaloneBugCheckerthat never reads NullAway'sConfig, while the option lives inCodeAnnotationInfo.shouldTreatAsUnannotatedand only feeds theNullAwaycheck's model.Setting it or clearing it changes nothing here.
RestrictiveAnnotationHandler: "with the generated-as-unannotated option enabled, we want to ignore annotations in generated code no matter what" - including underAcknowledgeRestrictiveAnnotations=true.NullAway's own source notes the gap: "In the future, we might want finer grain controls to distinguish code that is generated with nullability info and without." This project is now exactly that case.
OnlyNullMarked- tried, measured, and rejectedThe JSpecify-native alternative to
AnnotatedPackages: only@NullMarkedcode counts as annotated.It reads better, because
@NullMarkedlives in the source where JSpecify tooling and IntelliJ read it, whileAnnotatedPackagesis a NullAway-only setting no other tool understands.It was switched on during this PR and switched back.
It is strictly weaker here, because it silently turns off checking of the generated module. jOOQ annotates the column accessors only.
120 generated methods have unannotated parameters -
as(String alias),rename(Name name),where(Condition condition)and their siblings - and this project calls such methods 32 times inoperator/src/main/java.PG_AUTHID.as((String) null)AnnotatedPackagesOnlyNullMarkedUnder
AnnotatedPackagesthe generated module is annotated code, so an unannotated parameter means non-null and NullAway enforces it.Under
OnlyNullMarkedthe module falls back to unannotated, where NullAway is optimistic and lets null through.AcknowledgeRestrictiveAnnotationsdoes not close the gap:it reads annotations that make an API stricter, and an absent annotation carries nothing to read.
The strictness can be restored under
OnlyNullMarked, but not cheaply.@NullMarkedon a package does not apply to subpackages - verified both ways:a
package-info.javain...persistencewhile the class sits in...persistence.tablesreports nothing, while the same file in...persistence.tablesreports the error.That means four
package-info.javafiles, one per generated package, which must also survive jOOQ's cleaning ofsrc/main/javaon every regeneration.That is more machinery than the tidiness is worth, so
AnnotatedPackagesstays.JSpecifyExperimental- a follow-up PRIt turns on three things at once: the jspecify/jdk standard-library models, wildcard generics, and inference-failure warnings. Measured on this codebase it reports 12 findings, and they are genuine signal rather than noise:
@Nullablevalue where@NonNullis required@Nullable, butFunction.applyreturns@NonNullUis constrained to be@Nullable@Nullableexpression@Nullableexpression from a@NonNullmethodOne is worth naming:
matcher.group(1)is passed where non-null is required. The JDK model knows that method can return null, so this is a latent NPE that nothing catches today.Upstream plans to make
JSpecifyExperimentalthe default in a future release, so the 12 findings must be fixed sooner or later. A follow-up PR fixes them and enables the flag.That follow-up need not be one step. Each of the three features has its own flag -
JSpecifyJDKModels,HandleWildcardGenericsandWarnOnGenericInferenceFailure- and the wiki notes that most new errors come from the JDK models.The 12 findings can therefore be taken a flag at a time rather than in one hit.
Rejected outright
AssertsEnabledassertstatementsCheckContracts,CustomContractAnnotations@ContractannotationsReview notes
RequireExplicitNullMarkingcorrectly uses-Xep:, not-XepOpt:. It is a check, not an option: NullAway 0.14.1 ships exactly two@BugPatterncheckers,NullAwayandRequireExplicitNullMarking. Writing it as-XepOpt:NullAway:RequireExplicitNullMarking:ERRORparses as an unknown option with no=, is silently ignored, and lets the check fall back to its defaultSUGGESTIONseverity, where it reports nothing. Verified by compiling a scratch class with no@NullMarked: the-XepOptform builds successfully with 0 hits, the-Xepform fails the build with 2. Please do not "fix" this line.@SuppressWarnings({"all", "unchecked", "rawtypes", "this-escape"}), and Error Prone honours that annotation for checks at ERROR too. Verified by deleting that one line fromRoutines.javaand recompiling:RequireExplicitNullMarking×1,PrivateConstructorForUtilityClass×2 andVarifier×6 fire immediately. The README records this, so nobody hunts for a flag that does not exist.ExhaustiveOverridehas a dependency. IfMissingOverrideis ever lowered from ERROR, this option must come out with it, or NullAway will silently stop checking overrides that lack the annotation. The comment inerrorprone.argsrecords this.HandleTestAssertionLibrariescannot break the build. It only teaches NullAway to trust an assertion, so it removes findings.AcknowledgeRestrictiveAnnotationsis the one that can bite later. It is clean today, but a dependency bump that adds@Nullableannotations to fabric8 or jOOQ will surface new errors. That is the point of the option, and the errors will be real.Test scope
:operator:compileJava,:operator:compileTestJavaand:generated:compileJavawith--rerun-tasks. All four report 0.--rerun-taskson the same three tasks - BUILD SUCCESSFUL, 0 warnings../gradlew build -x test- BUILD SUCCESSFUL.