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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
