feat: multi-release JAR infrastructure for bytecode analysis (JDK 24+ classfile API) - #1153
Conversation
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Solid foundation for the MRJAR bytecode analysis infrastructure. The generic signature parser is carefully structured with the FormalTypeParameters/TypeArguments distinction, and the test coverage is thorough — especially the regression tests for T-prefixed type parameter names. A few items to address.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
801d7cc to
178c49e
Compare
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after force-push: PR scope narrowed from full bytecode analyzer implementation to build infrastructure only (pom.xml + CI workflow). Previous findings #2–#4 (mutable sets, HexFormat, descriptor guards) no longer apply — those files were removed from this PR.
Build infrastructure looks solid: enforcer constraint, MRJAR compilation execution, Surefire classpath ordering, and CI matrix narrowing are all correctly configured. One previous finding remains unaddressed.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
178c49e to
f562878
Compare
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after force-push (f562878): PR scope now includes the ByteCodeTransformer JDK 24+ override using java.lang.classfile API. The classfile implementation is a faithful port of the ASM root version — same method signature, same null-return semantics, correct preservation of exports/opens/uses/provides. Build infrastructure (enforcer, compile-java24, surefire classpath, Multi-Release manifest) is solid. One previous finding remains unaddressed (3rd time raised).
This review was generated by an AI agent, Hermès on behalf of @gnodet.
…odeTransformer Sets up the MRJAR build infrastructure and provides the first concrete java.lang.classfile-backed implementation as proof of concept. ## Build changes (pom.xml) - maven-enforcer: require JDK 25+ at build time (src/main/java24/ must compile unconditionally; the plugin JAR still runs on JDK 17+) - maven-compiler-plugin: compile-java24 execution -- compiles src/main/java24/ with --release 24 into target/classes/META-INF/versions/24/ - maven-surefire-plugin: prepend META-INF/versions/24/ on the test classpath so JDK 24+ class overrides are resolved during tests (MRJAR dispatch applies to JARs only, not directories) - mockito: 5.23.0 -> 5.24.0 (align with master; was stale-base artifact) ## CI (.github/workflows/maven-verify.yml) - Restrict build matrix to JDK 25+ to satisfy the enforcer rule ## java.lang.classfile override: ByteCodeTransformer The existing ASM-backed ByteCodeTransformer.patchJdkModuleVersion() (JDK-8318913 / MCOMPILER-542 workaround) is supplemented by a java.lang.classfile-backed implementation in src/main/java24/: - On JDK 24+, the JVM automatically loads the versioned override (META-INF/versions/24/), removing the ASM dependency from the hot path - On JDK 17-23, the existing ASM implementation is loaded instead - Both implementations are functionally identical: patch the version attribute on java.* / jdk.* module requires entries The new implementation uses ClassFile.of().transformClass() with a ClassTransform lambda that intercepts ModuleAttribute elements and rebuilds the requires list with patched version strings. ## Tests 4 tests in ByteCodeTransformerTest: - non-module class (no ModuleAttribute -> null) - jdk requires: patch applied and version verified to be '21' (via ASM re-parse) - patched bytes are a valid class file (CAFEBABE magic) - empty module does not throw
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after 4b4348f (Mockito version fix).
Previous findings status:
- ✅ Mockito downgrade (raised 3×) — Addressed.
mockitoVersionis now 5.24.0, matching master. - ❌ Test gap (raised 1×) — Not addressed. Tests still only verify CAFEBABE magic, not that the patched version is actually
"21". - ❌ Self-referential Javadoc (raised 1×) — Not addressed.
{@link ByteCodeTransformer}on the JDK 24+ override links to itself.
The ByteCodeTransformer classfile API port is a faithful reimplementation of the ASM root version — same null-return semantics, same preservation of exports/opens/uses/provides. Build infrastructure (enforcer, compile-java24, surefire classpath, Multi-Release manifest) and CI matrix narrowing are all correct. Two cosmetic items remain from the previous review.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
4b4348f to
af41175
Compare
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after force-push (af41175): all three previous findings addressed.
Previous findings status:
- ✅ Mockito downgrade (raised 3×) — Addressed.
mockitoVersionproperty is no longer modified in this diff. - ✅ Test gap (raised 2×) — Addressed.
patchesJdkRequiresVersionToTargetnow parses the result with ASM and assertsassertEquals("21", ...)for each JDK module requires entry. - ✅ Self-referential Javadoc (raised 2×) — Addressed. Line 29 now uses
{@code ByteCodeTransformer}instead of{@link ByteCodeTransformer}.
The java.lang.classfile implementation is a faithful port of the ASM root version — same null-return semantics, same version-patching logic for java.*/jdk.* requires entries, correct preservation of exports/opens/uses/provides via ModuleAttribute.of() builder. Build infrastructure (enforcer JDK 25+, compile-java24 execution, Surefire MRJAR classpath ordering, Multi-Release manifest) is solid.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
@gnodet Please assign appropriate label to PR according to the type of change. |
2 participants
Summary
Adds the MRJAR (multi-release JAR) infrastructure for bytecode analysis as the foundation for a class-level dependency-graph incremental build strategy (follow-up PR).
What this PR adds
BytecodeAnalyzer— public MRJAR facade with two implementations:isAvailable()returnsfalse; allanalyze()calls throwUnsupportedOperationExceptionMETA-INF/versions/24/(JDK 24+ real impl): delegates toClassfileClassAnalyzerClassfileClassAnalyzer— full bytecode analysis usingjava.lang.classfileAPI:signatureTypes(public API surface) andimplementationTypes(method bodies, private members) usingisPrivateOrSynthetic()consistently for both fields and methods (bridge method correctness)annotationTypesfor annotation processor cascade decisionsmodule-info.classwith themodule:prefix conventionClassAnalyzer— abstract base class with sharedbuildCanonicalForm(),FieldInfo/MethodInforecords, andisPrivateOrSynthetic()utilitySha256— SHA-256 hash utility (16-char hex prefix for fingerprints)compile-java24execution targets--release 24, outputs toMETA-INF/versions/24/;Multi-Release: truemanifest; SurefireclassesDirectorypoints to versions/24/ so tests exercise the real analyzerTest coverage
BytecodeAnalyzerTest: 29 tests covering descriptor parsing, generic signatures, type arg tracking, formal type parameter bounds, sig/impl split,annotationTypes,sourceFileName,module-info.classfingerprinting, and idempotencyRelation to follow-up PRs
SourceFileAnalysis,IncrementalState,AbiIncrementalBuildusingBytecodeAnalyzerfor thegraphincremental strategyThis PR intentionally contains no incremental strategy wiring.