Re: [PR] Add @Nullable annotations and NullAway profile for Maven 4 API [maven]

["gnodet-bot (via GitHub)" <gi...@apache.org>]

gnodet-bot commented on code in PR #11818:
URL: https://github.com/apache/maven/pull/11818#discussion_r4306699856


##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/ModelBuilderRequest.java:
##########
@@ -88,7 +88,7 @@ enum RepositoryMerging {
         REQUEST_DOMINANT,
     }
 
-    @Nonnull
+    @Nullable
     ModelSource getSource();

Review Comment:
   ⚠️ **High — Unguarded callers of newly-`@Nullable` `getSource()`**
   
   `getSource()` was changed from `@Nonnull` to `@Nullable`, but `DefaultModelBuilder.java` has ~10 call sites (lines 1007, 1313, 1322, 1334, 1816, 1824, 2172, etc.) that dereference the result directly via `.getPath()`, `.resolve()`, or pass it to `cache()` — all without null guards.
   
   The `build()` method validates `session` and `requestType` with `requireNonNull()` but does not do the same for `source`. Either restore the `@Nonnull` contract and add `requireNonNull(source, "source")` in `build()`, or add null guards at every caller. The former is the better choice given the builder pattern here.
   
   ```suggestion
       @Nonnull
       ModelSource getSource();
   ```



##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/ModelBuilderRequest.java:
##########
@@ -130,7 +130,7 @@ enum RepositoryMerging {
     @Nonnull
     Map<String, String> getUserProperties();
 
-    @Nonnull
+    @Nullable
     RepositoryMerging getRepositoryMerging();

Review Comment:
   ⚠️ **High — Unguarded callers of newly-`@Nullable` `getRepositoryMerging()`**
   
   `getRepositoryMerging()` changed from `@Nonnull` to `@Nullable`, but callers use the result unsafely:
   - `DefaultModelBuilder.java:731`: `request.getRepositoryMerging() == RepositoryMerging.REQUEST_DOMINANT` — safe (null ≠ enum constant), but silently falls through to the else branch, changing behaviour.
   - `DefaultProjectBuilder.java:1181`: switches on it — will hit the default branch unexpectedly (or NPE if `switch` requires non-null).
   
   If this must be nullable, the builder should default to `POM_DOMINANT` (the previous implicit behaviour) rather than leaving it null, and callers need updating. If it should stay non-null, add `requireNonNull` in `build()`:
   
   ```suggestion
       @Nonnull
       RepositoryMerging getRepositoryMerging();
   ```



##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/ToolchainManager.java:
##########
@@ -51,7 +52,7 @@ public interface ToolchainManager extends Service {
      * @throws ToolchainManagerException if toolchain retrieval fails
      */
     @Nonnull
-    List<Toolchain> getToolchains(@Nonnull Session session, String type, Map<String, String> requirements);
+    List<Toolchain> getToolchains(@Nonnull Session session, String type, @Nullable Map<String, String> requirements);

Review Comment:
   💡 **Medium — `type` parameter lacks explicit nullability annotation**
   
   `requirements` gets `@Nullable` and `session` has `@Nonnull`, but `type` (the primary lookup key) has no annotation. NullAway defaults unannotated parameters to `@Nonnull`, so this is functionally correct if `type` must not be null — but it should be explicit for consistency with the newly-annotated `session` and `requirements`, and to make the NullAway contract unambiguous to readers.
   
   ```suggestion
       List<Toolchain> getToolchains(@Nonnull Session session, @Nonnull String type, @Nullable Map<String, String> requirements);
   ```



##########
pom.xml:
##########
@@ -1254,5 +1254,52 @@ under the License.
         </plugins>
       </build>
     </profile>
+    <profile>
+      <id>nullaway</id>
+      <properties>
+        <errorProneVersion>2.36.0</errorProneVersion>

Review Comment:
   💡 **Low — Error Prone 2.36.0 is significantly behind current 2.50.0**
   
   Error Prone 2.36.0 is 14 minor versions behind the current release (2.50.0). Prefer the latest for bug fixes and improved diagnostics. If this version is pinned due to a NullAway 0.12.6 compatibility constraint, add a comment explaining the pin so future maintainers don't blindly upgrade one without the other.



##########
api/maven-api-xml/src/main/java/org/apache/maven/api/xml/XmlNode.java:
##########
@@ -458,30 +473,41 @@ public XmlNode build() {
         }
 
         private record Impl(

Review Comment:
   🔴 **High — Rebase-breaking: `Impl` record is missing the `namespaces` field from master**
   
   Master's `XmlNode.Impl` is an **8-parameter** record that includes `@Nonnull Map<String,String> namespaces` (added for namespace prefix tracking). This PR's `Impl` has only **7 parameters** — `namespaces` is absent entirely.
   
   On rebase to current master, `Builder.build()` passes 8 arguments to `new Impl(...)` but the record only accepts 7, causing a compile error. The `namespaces` field, its compact constructor handling (`ImmutableCollections.copy(namespaces)`), and the `namespaces()` accessor must be included in this change.
   
   Current master `Impl` signature for reference:
   ```java
   private record Impl(
           @Nonnull String prefix,
           @Nonnull String namespaceUri,
           @Nonnull String name,
           @Nullable String value,
           @Nonnull Map<String, String> attributes,
           @Nonnull Map<String, String> namespaces,   // ← missing from this PR
           @Nonnull List<XmlNode> children,
           @Nullable Object inputLocation)
   ```



##########
api/maven-api-annotations/src/main/java/org/apache/maven/api/annotations/Nullable.java:
##########
@@ -31,4 +33,5 @@
 @Experimental
 @Documented
 @Retention(RetentionPolicy.RUNTIME)

Review Comment:
   💡 **Medium — `@Retention` inconsistency between `@Nullable` and `@Nonnull`**
   
   `@Nullable` uses `RetentionPolicy.RUNTIME`, but `@Nonnull` uses `RetentionPolicy.CLASS`. These companion annotations should use the same retention policy.
   
   NullAway and Error Prone only need `CLASS` retention — they run at compile time. Using `RUNTIME` for `@Nullable` embeds it in bytecode and makes it available via reflection at runtime, which is unnecessary overhead. More importantly, the inconsistency is surprising: both annotations serve the same null-safety purpose, so they should have identical retention.
   
   ```suggestion
   @Retention(RetentionPolicy.CLASS)
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org