Skip to content

Add @Nullable annotations and NullAway profile for Maven 4 API#11818

Open
gnodet wants to merge 1 commit into
apache:masterfrom
gnodet:nullaway-annotations
Open

Add @Nullable annotations and NullAway profile for Maven 4 API#11818
gnodet wants to merge 1 commit into
apache:masterfrom
gnodet:nullaway-annotations

Conversation

@gnodet

@gnodet gnodet commented Mar 20, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add opt-in NullAway profile (-Pnullaway) using Error Prone 2.36.0 + NullAway 0.12.6 for compile-time null safety checking on org.apache.maven.api packages
  • Add @Nullable annotations across the Maven 4 API where values can genuinely be null: fields, method parameters, return types, builder fields, generated model code (via Velocity template)
  • Add requireNonNull() validation in builder build() methods to preserve existing @Nonnull API contracts — no @Nonnull annotations were changed to @Nullable
  • Fix InputLocation.locations field: was incorrectly annotated @Nullable but is never null (always initialized via ImmutableCollections which returns empty map for null input)
  • Fix Sources.BuildPathSource.resolve() null safety for Path.getParent() which can return null

Details

The NullAway profile runs at WARN level with -XepDisableAllChecks so only NullAway warnings are reported. It is designed as an opt-in quality check, not a build gate. The profile configures NullAway to recognize Maven's custom @Nullable and @Nonnull annotations.

Files changed fall into these categories:

  • Hand-written API classes: Added @Nullable to genuinely nullable fields, params, and return types
  • Builder classes: Added @Nullable to builder fields (which start null) and requireNonNull() in build() methods for non-null params
  • Velocity template (model.vm): Added @Nullable to non-primitive, non-collection generated fields and their getters
  • Template source files (InputLocation.java, InputSource.java, etc.): Added @Nullable annotations
  • .mdo model files: Added @Nullable to code segments where needed

Test plan

  • All existing tests pass (mvn clean test -f api)
  • Clean compile without NullAway profile (mvn clean compile -f api)
  • Zero NullAway warnings with profile enabled (mvn clean compile -f api -Pnullaway)
  • Verify no downstream breakage in maven-core modules

🤖 Generated with Claude Code

@gnodet gnodet force-pushed the nullaway-annotations branch 2 times, most recently from ffcc698 to b603fc4 Compare March 20, 2026 10:58
…ile-time null checks

- Add opt-in NullAway profile (-Pnullaway) using Error Prone 2.36.0 + NullAway 0.12.6
  for compile-time null safety checking on org.apache.maven.api packages
- Add @nullable annotations to fields, method parameters, and return types across
  the Maven 4 API where values can genuinely be null
- Add @nullable to Velocity model template (model.vm) for generated model classes:
  non-primitive, non-collection fields and their getters
- Add @nullable to hand-written template files: InputLocation, InputSource,
  InputLocationTracker, ImmutableCollections
- Add @nullable to XmlNode builder, XmlService merge methods, XmlReaderRequest,
  XmlWriterRequest
- Add @nullable to builder fields in request classes (fields start null before
  builder methods are called)
- Add requireNonNull() validation in builder build() methods for parameters
  that must be non-null, preserving existing @nonnull API contracts
- Fix InputLocation.locations field: not nullable (always initialized via
  ImmutableCollections which returns empty map for null input)
- Fix Sources.BuildPathSource.resolve() null safety for Path.getParent()
- Add @nullable to Nullable annotation itself (meta-annotation for NullAway)
- Fix Phase.getEffectiveId() return annotation in lifecycle.mdo

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@gnodet gnodet force-pushed the nullaway-annotations branch from b603fc4 to 7a89764 Compare March 20, 2026 11:06
@gnodet gnodet added mvn4 enhancement New feature or request labels Jun 22, 2026

@gnodet gnodet left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Add @nullable annotations and NullAway profile for Maven 4 API

Well-structured PR that adds @Nullable annotations and an opt-in NullAway Error Prone profile across the Maven 4 API. The annotations are overwhelmingly correct, CI passes across all platforms and JDK versions, and the NullAway profile is properly configured as opt-in at WARN level — the right approach for incremental adoption.

Key strengths

  • @Nullable annotations are applied consistently to builder fields (which start null before builder methods are called) and to genuinely nullable API return values
  • requireNonNull() additions in builder build() methods correctly enforce the distinction between nullable builder state and non-null API contracts
  • The InputLocation.locations fix and XmlNode value null-safety fix in toStringObject() are genuine bug fixes
  • Session parameter annotations match the underlying factory request contracts

Finding: ModelBuilderRequest.getSource() contract change (medium)

api/maven-api-core/src/main/java/org/apache/maven/api/services/ModelBuilderRequest.java

getSource() changed from @Nonnull to @Nullable. Multiple callers in DefaultModelBuilder dereference getSource() without null checks — e.g., request.getSource().getPath(), request.getSource().resolve(...). The build() method does not add requireNonNull for source (unlike session and requestType).

If source is genuinely nullable, the callers need null guards. If it was @Nonnull for a reason, consider either:

  • Adding requireNonNull(source, "source cannot be null") in build() to preserve the non-null contract, or
  • Adding null guards in DefaultModelBuilder callers

Observation: getRepositoryMerging() contract change (low)

Same pattern — getRepositoryMerging() changed from @Nonnull to @Nullable. The builder field starts null and build() does not enforce non-null. Callers should be audited for null-safety, though this is less likely to cause issues than getSource().

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of Guillaume Nodet

gnodet added a commit to gnodet/maven that referenced this pull request Jul 9, 2026
Reviewed 3 PRs: apache#12416 (BOM version resolution fix), apache#12410 (path-traversal
re-review), apache#11818 (@nullable annotations). 5 findings verified, 5 false
positives dropped.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request mvn4

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant