Modernize codebase with Java improvements

[gnodet]

This PR includes several improvements to modernize the codebase:

1. Replace IllegalArgumentException with ClassCastException for type cast failures
2. Replace IllegalArgumentException with NullPointerException for null checks
3. Use modern Java collections API (toList() instead of collect(Collectors.toList()))
4. Replace custom null checks with Objects.requireNonNull
5. Add @nonnull and @nullable annotations throughout the codebase

These changes make the code more maintainable, safer, and aligned with modern Java practices.

[kwin]

Regarding point 2. (and the related point 4.): I consider the IAE more concise than an NPE in case a method argument is passed with null (although not allowed). Compare also with https://discuss.kotlinlang.org/t/why-does-requirenotnull-throw-an-illegalargumentexception/7617/4

[desruisseaux]

I think that the main argument in favour of NullPointerException is consistency. Not all methods perform explicit null checks. Especially since JEP 358: Helpful NullPointerExceptions provides even more helpful messages than many explicit null checks. Therefore, if Objects.requireNonNull was throwing IllegalArgumentException for a null argument, we would be at risk of having sometime an IllegalArgumentException, and sometime a NullPointerException, depending on whether the method chooses to perform explicit null checks or not. It would also be a risk of compatibility break if the method implementation changes its strategy over time.

[Pankraz76]

• Use modern Java collections API (toList() instead of collect(Collectors.toList()))

Hi folks, please check out attached openrewrite link as its related and some can be done by computer.

This could be supplemented in dedicated PR as there is a migration for this:

• https://docs.openrewrite.org/recipes/staticanalysis/replacestreamtolistwithcollect

Maybe we can team up with as its kind of related. The migration is "free" as done by one click so we can reproduce anytime.

• https://docs.openrewrite.org/recipes/java/migrate/upgradetojava21
• [POC] Migrate to Java 21 - code #2281

[gnodet]

its nice to fix this once, but without a computer checking and enforcing this by some tool like checkstyle or rewrite, its not completed.

I don't see where it's done in your PR as it only provides the result of executing the transformation.

[Pankraz76]

Current State of NullPointerException Handling

The current implementation around NullPointerException is not ideal, but in context, it's clear what's happening.

Options:

1. Keep it as-is
   • Works but lacks elegance.
2. Remove all such checks
   • Simplifies but loses explicit null-check messaging.
3. Refactor into a dedicated layer
   • Separate the "cannot be null" concern to avoid:
     • DRY violations (repetition)
     • Feature envy (fluff and copy-paste)

---

Proposal: Dedicated Logging Layer

If improved logging is desired, this concern should be moved to a dedicated utility layer, similar to Apache Maven's approach:

maven/impl/maven-impl/src/main/java/org/apache/maven/impl/ImplUtils.java

Line 37 in d65c375

throw new IllegalArgumentException(name + " cannot be null");

Benefits:

• Contextual clarity: Right now, the check is just a short "non-null" hint.
• Better debugging: Explicit messaging improves NullPointerException readability.
• Consistency: Centralized handling avoids duplication.

This change is already reflected in PR #2290.

---

[Pankraz76]

Current State of NullPointerException Handling

The current implementation around NullPointerException is not ideal, but in context, it's clear what's happening.

Options:

1. Keep it as-is
   • Works but lacks elegance.
2. Remove all such checks
   • Simplifies but loses explicit null-check messaging.
3. Refactor into a dedicated layer
   • Separate the "cannot be null" concern to avoid:
     • DRY violations (repetition)
     • Feature envy (fluff and copy-paste)

---

Proposal: Dedicated Logging Layer

If improved logging is desired, this concern should be moved to a dedicated utility layer, similar to Apache Maven's approach:

maven/impl/maven-impl/src/main/java/org/apache/maven/impl/ImplUtils.java

Line 37 in d65c375

throw new IllegalArgumentException(name + " cannot be null");

Benefits:

• Contextual clarity: Right now, the check is just a short "non-null" hint.
• Better debugging: Explicit messaging improves NullPointerException readability.
• Consistency: Centralized handling avoids duplication.

This change is already reflected in PR #2290.

---

[gnodet]

@Pankraz76 I'm not sure what you aim for with your PRs. How do they differ from this one ? It's fine very fine that you used openrewrite rules, but isn't the result the same at the end ?

[elharo]

"Use modern Java collections API (toList() instead of collect(Collectors.toList()))" is riskier than the other changes because of the move of mutable to immutable lists. Nine times out of ten this is fine. The tenth time it's a bear to debug. These changes should be pulled out into a separate PR.

The rest could be split, but maybe don't have to be. Nuulable and Nonnull might also call for a separate PR.

[Pankraz76]

Regarding point 2. (and the related point 4.): I consider the IAE more concise than an NPE in case a method argument is passed with null (although not allowed). Compare also with https://discuss.kotlinlang.org/t/why-does-requirenotnull-throw-an-illegalargumentexception/7617/4

Java and Lombok prefer NPE as well.

https://docs.oracle.com/javase/8/docs/api/java/util/Objects.html#requireNonNull-T-

https://projectlombok.org/features/NonNull

both valid points. Ideally its just one config-switch to adjust:

[gnodet]

There 's no plan to use Lombok at this point.

[Pankraz76]

this is dedicated in:

• [chore] Modernize codebase with Java improvements - Replace custom null checks with Objects.requireNonNull #2290

if possible lets merge this first as this is kind of huge.

[gnodet]

AI Review — PR #2277: Modernize codebase with Java improvements

This is a large (191 files) but predominantly mechanical modernization PR by the project maintainer. Five categories of changes: @Nonnull/@Nullable annotations, Objects.requireNonNull replacing custom nonNull(), Stream.toList() replacing collect(Collectors.toList()), List.of()/Map.of() replacing Collections.emptyList()/singletonList(), and inner classes → records in Lifecycles.java.

toList() Mutability Concern

As elharo noted in their CHANGES_REQUESTED review, the toList() migration is the highest-risk category. Collectors.toList() returns a mutable ArrayList; Stream.toList() returns an unmodifiable list. Two specific call sites worth auditing:

1. ImplUtils.map() — Changes from collect(Collectors.toList()) to toList(). Callers that mutate the returned list will get UnsupportedOperationException. Sampled callers appear read-only, but a systematic audit would be prudent.

2. LifecycleDependencyResolver.getProjects() — Same pattern. Callers in MojoExecutor iterate without mutation, but the contract change is latent.

elharo's suggestion to split the toList() changes into a separate PR has merit — it isolates the mutability risk from the purely safe annotation/null-check changes.

What Looks Good

• The Objects.requireNonNull migration (from custom ImplUtils.nonNull()) is clean and complete — all call sites updated, dead methods removed
• The @Nonnull/@Nullable annotations improve IDE/static-analysis support
• The List.of()/Map.of() changes are safe — they replace already-immutable Collections.singletonList()/emptyList()
• The null check improvement in DefaultLifecycleBindingsInjector (adding guard before Map.of which doesn't accept null) is a correctness fix

File paths are current — all sampled paths exist on master.

---

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]

Code Review

The PR bundles five distinct refactoring categories across 191 files (annotation additions, exception type changes, Collections modernization, class-to-record conversions, null-check standardization). Two confirmed regressions were found in DefaultLifecycleRegistry:

1. WrappedPhase.plugins() returns ALL lifecycle plugins (DefaultLifecycleRegistry.java:228): The new WrappedPhase record passes the entire lfPhases map to parseLifecyclePhaseDefinitions(lfPhases), which iterates ALL entries. The original code looked up only the current phase's plugins via lfPhases.get(name). Every phase would now incorrectly return plugins from every other phase.

2. WrappedLifecycle.phases() drops extra default-phases support (DefaultLifecycleRegistry.java:240): The PR branch (from May 2025, 653 commits behind master) predates fix #11796 (June 2026). The WrappedLifecycle record only iterates lifecycle.getPhases(), omitting the buildOwnPhases() helper and expanded phases logic that preserves plugin bindings from components.xml default-phases entries. Merging this would revert fix #11796.

3. Stream.toList() mutability (low, multiple files): Stream.toList() produces unmodifiable lists whereas collect(Collectors.toList()) returned mutable ArrayLists. Already flagged by elharo's CHANGES_REQUESTED review recommending this be split into a separate PR.

Note: Three other reviewer findings were independently determined to be false positives (parseLifecyclePhaseDefinitions caller breakage, Phase.allPhases() default method compatibility, and CoreUtils exception type changes).

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

Claude Code on behalf of gnodet