Denis Zhdanov opened SPR-16291 and commented
The idea is to replace explicit null checks by NonNull annotation on target method arguments and let the actual checks be inserted into resulting byte code during compilation based on that annotations.
This PR illustrates the concept on the AbstractAliasAwareAnnotationAttributeExtractor class:
Before
AbstractAliasAwareAnnotationAttributeExtractor(
Class<? extends Annotation> annotationType, @Nullable Object annotatedElement, S source) {
Assert.notNull(annotationType, "annotationType must not be null");
Assert.notNull(source, "source must not be null");
this.annotationType = annotationType;
After
AbstractAliasAwareAnnotationAttributeExtractor(
@Nonnull Class<? extends Annotation> annotationType, @Nullable Object annotatedElement, @Nonnull S source) {
this.annotationType = annotationType;
The actual null check is added by the Traute javac plugin - it's configured to generate them for all method parameters marked by the org.springframework.lang.NonNull annotation - link.
Result looks as if it's compiled from the source below:
AbstractAliasAwareAnnotationAttributeExtractor(@NonNull Class<? extends Annotation> annotationType, @Nullable Object annotatedElement, @NonNull S source) {
if (annotationType == null) {
throw new NullPointerException("annotationType must not be null");
}
if (source == null) {
throw new NullPointerException("source must not be null");
}
this.annotationType = annotationType;
Actual result:
javap -c ./spring-core/build/classes/java/main/org/springframework/core/annotation/AbstractAliasAwareAnnotationAttributeExtractor.class
...
org.springframework.core.annotation.AbstractAliasAwareAnnotationAttributeExtractor(java.lang.Class<? extends java.lang.annotation.Annotation>, java.lang.Object, S);
Code:
0: aload_0
1: invokespecial #1 // Method java/lang/Object."<init>":()V
4: aload_1
5: ifnonnull 18
8: new #2 // class java/lang/NullPointerException
11: dup
12: ldc #3 // String annotationType must not be null
14: invokespecial #4 // Method java/lang/NullPointerException."<init>":(Ljava/lang/String;)V
17: athrow
18: aload_3
19: ifnonnull 32
22: new #2 // class java/lang/NullPointerException
25: dup
26: ldc #5 // String source must not be null
28: invokespecial #4 // Method java/lang/NullPointerException."<init>":(Ljava/lang/String;)V
31: athrow
...
An additional benefit is that the annotation is present in javadocs, i.e. the contract is defined in more clear way. Another good thing is that IDEs highlight potential NPE when a might-be-null is used as a NonNull-parameter.
I'm the Traute's author and I'm keen on applying it into beloved Spring Framework, so, if the team likes the change, I'm fine with creating a PR which replaces all existing null-checks by the NonNull annotation.
Affects: 5.0.2
Comment From: spring-projects-issues
Juergen Hoeller commented
We have a quite comprehensive nullability revision in Spring Framework 5.0 already: Aside from @Nullable declarations, we actually set nullability to non-null by default at the package level. This is still a formal declaration and understood (or at least configurable) with IntelliJ IDEA and Eclipse, reducing the noise at the individual source code level by assuming non-null and just annotating the exception to the rule through @Nullable. This is a fine trade-off for us; I wouldn't want to switch this to explicit @NonNull declarations everywhere. The only reason why we have a @NonNull annotation of our own is to override a @Nullable base declaration with @NonNull in a subclass, e.g. for an overridden method's return type.
So we get quite a bit of benefit out of this already, not just for IDE use but in particular for the Kotlin compiler which allows assignments to non-null declarations that way. While I see the additional convenience of generated null checks for input parameters, I don't see our assertions as a big enough pain to justify the use of a compiler plugin for that purpose. We're quite happy with explicit assert calls, often doing additional checks with custom exception messages in the same implementation style.
Also, we consistently throw IllegalArgumentException for any such input parameter checks, allowing consistent handling of both null and other invalid input. We wouldn't want to switch to NullPointerException which we rather see as a compiler-generated "oops that should have never happened inside this piece of code" case that needs to be fixed. In other words, if you get a NullPointerException out of a Spring API call, that's a bug in our code; if you get an IllegalArgumentException, it is clearly a bug in your calling code. On a related note, we pay the same degree of attention to IllegalArgumentException versus IllegalStateException.
So in short, while I see the value of your plugin for certain kinds of projects, I'd rather keep our existing framework approach as-is. FWIW, we don't use other conveniences such as Lombok either, trying to keep our code as straightforward and obvious as possible. We mostly care about user convenience, not about our own, and our current nullability declarations provide all possible user benefit already. Last but not least, there is value in consistency with the rest of our assertions and in our consistent throwing of IllegalArgumentException out of Spring APIs.
Comment From: spring-projects-issues
Denis Zhdanov commented
Hi Juergen Hoeller,
Thanks for the feedback and it makes sense indeed!
However, the plugin fits perfectly the mentioned use-cases:
1. NullPointerException is thrown by default from failed checks but it's possible to configure any other exception, e.g. IllegalArgumentException
2. The plugin supports either explicit @NotNull or @NotNullByDefault + @Nullable (by the way, Spring's @NonNullApi and @Nullable are already used by default)
So, if further Spring codebase evolution trends toward package-level @NonNullApi + @Nullable, I don't think it makes sense to insert explicit checks for all method parameters (there are quite a few of them). However, end-users benefit if invalid API usage is reported as early as possible, hence, it's convenient to provide such automatically generated checks which throw IllegalArgumentException for invalid null calls.
Please let me know what do you think about that.
Comment From: spring-projects-issues
Juergen Hoeller commented
Hi Denis Zhdanov,
Oh nice, I didn't expect that level of configurability. Good job :-)
So at that point, it's about hard-coded Assert calls versus generated assertions. No general concerns on my side there... just a mild preference for consistency with other kinds of programmatic assertions and for explicit visibility of those assertion statements in our source code.
Of course, I'm specifically talking about our framework codebase here, with many 10+ year traditions of its own and thousands of people looking at it. For application codebases, I'd be much more willing to use a compiler plugin; same for annotation processors such as Lombok.
Comment From: spring-projects-issues
Denis Zhdanov commented
Thanks, Juergen Hoeller!
Got your point about the established traditions for a long-running project.
The only note is that we might consider not replacing existing checks by automatically generated code but enforce package-level contracts - if more and more package-level nullability annotations are introduced to the Spring source, it doesn't look attractive to insert explicit checks for all of them.
Do you think it's worth to introduce such automatic checks and avoid adding new explicit checks (Traute can be configured to generate the code only for @NotNullByDefault rules and left @NonNull arguments as-is)?
Comment From: spring-projects-issues
Juergen Hoeller commented
I don't really see a clear line for programmatic checks versus automatic checks here since many non-null defaulted parameters have explicit assertions at this point. If anything is missing, I'd rather add some further assertion statements there. Mixing and matching would lead to a rather inconsistent source reading experience overall.
For an application project, I'd argue along the same lines: For a new project, go with automatic checks right from the start - or stick with consistent programmatic assertions. For refactoring an existing project, do it wholesale and replace all programmatic checks with automatic checks as far as possible; no mixing and matching.
Comment From: spring-projects-issues
Denis Zhdanov commented
Yeah, I see.
Thank you for your time and attention!
Sad that I was unable to contribute to my beloved project :)
Regards, Denis