Repository navigation
Enhance the NullAway configuration #68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: enable-javac-lint-categories-and-fix-deprecations
Are you sure you want to change the base?
Changes from all commits
05435a9
419b733
8151bfe
64efe30
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The annotations only land on record getters/setters and table methods. Field constants stay unannotated (
TableField<PgAuthidRecord, String> ROLPASSWORD), and the operator only uses those plusfetchOneInto/fetchSingle, never the record getters.So NullAway gains almost nothing today, while we take on the caveats documented in the readme. Maybe I'm wrong, so feel free to correct me on this.
Is it worth keeping this part, or should the PR stick to the NullAway options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right about today: the field constants stay unannotated, and no operator code calls a record getter. I would still like to keep it, for three reasons.
1. Without the annotations, NullAway trusts every getter as non-null.
The generated package
it.aboutbits.postgresql.core.infrastructure.persistenceis inside-XepOpt:NullAway:AnnotatedPackages=it.aboutbits.postgresql. So NullAway treats the jOOQ code as annotated code, and an unannotated return type there means non-null. For a nullable column, that is wrong, and NullAway stays silent.I checked this with a small probe class that dereferences
PgAuthidRecord.getRolpassword():error: [NullAway] dereferenced expression authid.getRolpassword() is @NullableBUILD SUCCESSFULSo the annotations do not only add information, they remove a false guarantee. The first time someone uses a record getter, NullAway checks it correctly. I first thought
AcknowledgeRestrictiveAnnotationswas the reason, but that option only applies to unannotated third-party code. The generated package is insideAnnotatedPackages, so NullAway reads it as annotated code.2. It is a real-world example for the open jOOQ issue.
jOOQ does not support TYPE_USE positioning yet (jOOQ/jOOQ#10759, still open). The six array accessors in this module show the problem:
@Nullable String[] getNspacl()reads as a non-null array of nullable strings. I would like to show this to Lukas Eder as a concrete case, possibly as the base for a reproducer.3. It is a good case for feedback to NullAway, too.
The usual way to handle generated code,
NullAway:TreatGeneratedAsUnannotated, would discard exactly these annotations.RestrictiveAnnotationHandlerignores annotations in generated code "no matter what", even withAcknowledgeRestrictiveAnnotations=true, which JSpecify mode turns on anyway. NullAway's own source names the gap inCodeAnnotationInfo: "In the future, we might want finer grain controls to distinguish code that is generated with nullability info and without." This module is exactly that case: generated code that carries nullability info.The caveats are documented in the README and in
generated/build.gradle.kts. The only wrong positions are the six array accessors, and no operator code calls them. If one of them is needed later, we can switchnullableAnnotationTypeto a declaration annotation or wait for the jOOQ fix.