-
Notifications
You must be signed in to change notification settings - Fork 1
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
JSpecify 0.3.0, Checker Framework nullness, and avaje-record-builder in Maven #43
Comments
For posterity, NullAway produces the logically same warnings as Checker Framework, meaning they agree on the analysis. This is a good thing, and unsurprising. NullAway is more flexible with respect to selecting input for analysis. It can ignore classes with specific annotations, or all Neither the builder nor NullAway, nor the ErrorProne harness, presently offer protection against failing to invoke the setter of a non-nullable component. That is, One crucial difference between Checker Framework and NullAway is their behaviour of Editor's note: Checker Framework's treatment of <profile>
<id>errorprone</id>
<build>
<plugins>
<plugin>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<fork>true</fork>
<annotationProcessorPaths combine.children="append">
<path>
<groupId>com.google.errorprone</groupId>
<artifactId>error_prone_core</artifactId>
<version>2.24.1</version>
</path>
<path>
<groupId>com.uber.nullaway</groupId>
<artifactId>nullaway</artifactId>
<version>0.10.19</version>
</path>
</annotationProcessorPaths>
<compilerArgs combine.children="append">
<arg>-XDcompilePolicy=simple</arg>
<arg>
-Xplugin:ErrorProne \
-XepOpt:NullAway:AnnotatedPackages=example \
-XepOpt:NullAway:ExcludedClassAnnotations=io.avaje.recordbuilder.Generated \
</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED</arg>
<arg>-J--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED</arg>
<arg>-J--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED</arg>
</compilerArgs>
</configuration>
</plugin>
</plugins>
</build>
</profile> |
so I've not used any of these null checking tools myself, do you happen to have an example project I can clone and take a look at? Do you have any preferred path you'd like me to take to try and resolve this? |
Hmm, what might resolve this would be to generate:
So instead of: Generate: |
Sounds cool, how would one go about this? EDIT: nvm I figured something out |
Just to say that the annotations in |
So do you want me to use the jspecify ones now? |
@commonquail
here's my pom, am I missing something? EDIT: nvm I figured it out haha |
Sorry for the delay. I was not in a position to easily do so so I settled for inlining hopefully-adequate samples.
I reported this more as an observation on the current state of the ecosystem than an attempt to suggest any components are malfunctioning, in the even that either you or other potential users would find value in that. That said, my personal preferences are the moment are for you to...
Note that I believe such a change is sufficient to avoid excluding generated builders from NullAway's analysis. |
Yeah went with option 2. Though due to the |
Better late than never I guess, the changes are in 1.1-RC1 |
Or, propagating TYPE_USE annotations (#19) is insufficient.
Or, builders are really validators.
I've been fooling around with nullability analysis to get a sense of the state of the ecosystem primo 2024. For this experiment I'm using
I'm filing this issue for posterity to record that this combination of tools presently is not workable. The essence of why is the same as the essence of uber/NullAway#121, the outcome of which is not clear to me. I am aware of the extensive history represented by #19 and Randgalt/record-builder#111.
There are variations I have not experimented with much or at all and so cannot really comment on:
Given a record
avaje-record-builder-1.0 generates the moral equivalent of
When compiled (POM below), Checker Framework produces three errors:
The line numbers refer to the canonical generated lines, not the compressed version shown above. The compressed version is annotated with the error codes.
There are two errors in the generated code, one "obvious" and one subtle. Other limitations in Checker Framework, Checker Framework's Maven integration, and avaje-record-builder combine to make those two errors somewhere between difficult and impossible to work around (I have not found a way).
assignment
: Although theNullityBuilder b(@Nullable String b)
receiver correctly hadorg.jspecify.annotations.Nullable
propagated from theNullity.b
component the annotation was not propagated to theNullityBuilder.b
field, which Checker Framework consequently resolves to@NonNull
.@Nullable
should have been propagated all the way to the field.initialization.fields.uninitialized
: Thebuilder()
factory instantiates a builder in its default state according to the JLS, i.e. with all fields uninitialized and implicitly null. This is valid Java but Checker Framework effectively resolves those fields to@NonNull
, which is a contradiction and therefore rejected. This behaviour is consistent with the various rules in play but the rules that are in play are not representative of a builder's role. The trouble is thatNullityBuilder.[@o.j.a.NonNull] String a
is an incorrect declaration:NullityBuilder.a
must actually be the moral equivalent of@o.j.a.Nullable @javax.validation.constraints.NotNull String
, comparable to Checker Framework's@org.checkerframework.checker.nullness.qual.MonotonicNonNull
. This is because the builder is stateful and cannot start out in a state ofNullityBuilder.a
being non-null. A knock-on consequence is that the builder must validate that the values passed to the record's@NonNull
components are in fact non-null. Checker Framework makes this extra difficult by rejectingObjects.requireNonNull
.NullityBuilder a(@NonNull String a)
receiver had the@NonNull
annotation propagated. It is reasonable because passing a@Nullable
value is clearly nonsensical but it is unimportant becauseNullityBuilder.a
cannot practically be@NonNull
.NullityBuilder.a
and all other@NonNull
record components could be promoted to parameters on thebuilder()
method. This would undermine the utility of the resulting builder, however.POM:
The text was updated successfully, but these errors were encountered: