gnodet-bot commented on code in PR #13584:
URL: https://github.com/apache/maven/pull/13584#discussion_r4298444852
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -978,6 +982,10 @@ private ClassRealm setupContainerRealm(
// sisu uses realm imports to establish component
visibility
extRealm.importFrom(realm, realm.getId());
}
+ if (realm != null) {
Review Comment:
⚠️ **Dead code.** `realm` is unconditionally dereferenced at line 983 via
`extRealm.importFrom(realm, realm.getId())` — if `realm` were `null`, a
`NullPointerException` would have already been thrown there. This guard can
never be `false` in practice. Remove it.
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -952,18 +952,22 @@ private List<CoreExtension>
selectCoreExtensions(Map<Path, List<CoreExtension>>
return new ArrayList<>(selectedExtensions.values());
}
- private ClassRealm setupContainerRealm(
+ ClassRealm setupContainerRealm(
Review Comment:
💡 The visibility was widened from `private` to package-private solely for
test access. Please annotate with `@VisibleForTesting` (from Guava or an
equivalent) to make the intent explicit:
```suggestion
@VisibleForTesting
ClassRealm setupContainerRealm(
```
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -952,18 +952,22 @@ private List<CoreExtension>
selectCoreExtensions(Map<Path, List<CoreExtension>>
return new ArrayList<>(selectedExtensions.values());
}
- private ClassRealm setupContainerRealm(
+ ClassRealm setupContainerRealm(
ClassWorld classWorld, ClassRealm coreRealm, List<File>
extClassPath, List<CoreExtensionEntry> extensions)
throws Exception {
if (!extClassPath.isEmpty() || !extensions.isEmpty()) {
ClassRealm extRealm = classWorld.newRealm("maven.ext", null);
extRealm.setParentRealm(coreRealm);
- slf4jLogger.debug("Populating class realm {}", extRealm.getId());
+ if (slf4jLogger != null) {
+ slf4jLogger.debug("Populating class realm {}",
extRealm.getId());
+ }
for (File file : extClassPath) {
- slf4jLogger.debug(" Included {}", file);
+ if (slf4jLogger != null) {
+ slf4jLogger.debug(" Included {}", file);
+ }
Review Comment:
⚠️ **Test-only null guards polluting production code.** The original code
called `slf4jLogger.debug(...)` unconditionally, proving `slf4jLogger` is
always non-null in production. These guards exist purely to let the new test
call `setupContainerRealm` on a partially-initialized `MavenCli` instance. The
right fix is to initialize `slf4jLogger` in the test via reflection rather than
weakening the production invariant:
```java
Field f = MavenCli.class.getDeclaredField("slf4jLogger");
f.setAccessible(true);
f.set(cli, LoggerFactory.getLogger(MavenCli.class));
```
Then these guards can be removed.
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -978,6 +982,10 @@ private ClassRealm setupContainerRealm(
// sisu uses realm imports to establish component
visibility
extRealm.importFrom(realm, realm.getId());
}
+ if (realm != null) {
+ // sisu uses realm imports to establish component
visibility
Review Comment:
💡 **Duplicated comment.** This is a verbatim copy of the comment on line
982, but the two `importFrom` calls have opposite directions and different
purposes. The forward import (`extRealm.importFrom(realm, ...)`) establishes
package visibility. The new reverse import makes Sisu discover components
registered in the container realm from within the extension realm — worth a
distinct comment, e.g.:
```suggestion
// reverse import: allows Sisu to look up container
components from within the extension realm
```
--
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]
