refactor: simplify annotation processor incremental handling

[gnodet]

Summary

Simplify amendincrementalCompilation() to always add REBUILD_ON_ADD and REBUILD_ON_CHANGE when annotation processors are detected, instead of the previous multi-branch logic that completely disabled incremental compilation for explicit proc=only and proc=full modes.

Depends on #1152 (ABI-fingerprint incremental compilation strategy).

Changes

AbstractCompilerMojo

• amendincrementalCompilation(): Simplified from a 3-branch conditional (JDK version check, proc value check, fallback) to a single unconditional add of REBUILD_ON_ADD + REBUILD_ON_CHANGE. The old behavior was overly conservative — it called aspects.clear() + NONE when processors were explicitly configured, which completely bypassed incremental compilation for any build using annotation processors.
• hasAnnotationProcessor() Javadoc: Removed "In case of doubt" qualifier since the method no longer drives JDK-version-dependent branching.

Removed tests

• testCompilerProcOnlyRunsWhenSourcesAreUnchanged: verified the old NONE behavior (proc=only ⇒ always recompile)
• testCompilerProcFullRunsWhenSourcesAreUnchanged: verified the old NONE behavior (proc=full ⇒ always recompile)
• Test resource directories: compiler-proc-only-test/, compiler-proc-full-test/

The testCompilerProcOnlyRespectsExplicitIncrementalCompilation test is retained — it verifies that an explicit incrementalCompilation configuration is not overridden by processor detection.

Rationale

With the ABI incremental strategy (#1152) handling annotation processors through ProcessorClassification (isolating/aggregating/unknown cascade logic), the timestamp strategy no longer needs to disable incremental compilation entirely when processors are present. The new behavior is: detect processors → add rebuild-on-add/change aspects → let the active strategy handle the details.

Part 3 of 3: MRJAR infrastructure (#1151) → ABI strategy (#1152) → annotation processor handling.

[gnodet-bot]

ABI Incremental Compilation — Initial Review

Verdict: COMMENT — Solid design, well-tested for a draft. A few production resilience gaps to address before this graduates.

The architecture (CompilationAnalyzer TaskListener → DependencyScanner → AbiExtractor → cascade engine) is clean and the test coverage is thorough (81 unit tests, 2 ITs). The binary state format versioning is handled correctly with forward compatibility. The three-strategy resolution cascade in ExternalAbiResolver (manifest → reactor → bytecode) is well-designed.

Key concerns are around exception handling, a fragile mutable field mutation pattern, and a gap in inner class cleanup.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Re-review — All previous findings addressed ✅

Verified commit cf283d0 against each finding from the previous review:

Finding	Status
DependencyScanner.resolveElement() — broad catch (Exception)	✅ Narrowed to IllegalArgumentException | NullPointerException
ToolExecutor — sourceFiles not restored on exception	✅ Wrapped in try/finally
AbiIncrementalBuild.deleteClassFile() — missing inner class cleanup	✅ Scans for ClassName$*.class pattern with best-effort cleanup
ExternalAbiResolver — silent IOException swallowing	✅ Added LOGGER.log(Level.FINE, ...)
AbiIncrementalBuild.initialize() — mtime-first optimization	Acknowledged as follow-up (correctness unaffected)
CompilationAnalyzer — per-CU over-cascading	Acknowledged as follow-up (safe, not under-cascading)

All fixes are correct and complete. No new issues introduced. Code is solid and ready to merge.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Re-review — Follow-up optimizations are correct ✅

Verified commit 0888d8e against the two follow-up items flagged in the previous review:

Optimization	Status
mtime-first hash short-circuit in hashSourceFiles()	✅ Correct — same trade-off as Gradle/make. v4→v5 migration is clean (getSourceMtime() returns empty, first build hashes everything). save() iterates sourceHashes, so deleted-file mtimes are never persisted.
Per-type scoping in CompilationAnalyzer	✅ Correct — Trees.getPath(typeElement) returns the ClassTree for the specific type. Inner classes are correctly scoped to their subtree. Fallback to full-CU scan is safe (pre-existing behavior).

No new issues introduced. Both optimizations are backward-compatible and the state version bump is handled correctly.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[rmannibucau]

i'd avoid asm now there is classfile api and ignore older hava version (warning maybe), bites too often and sometimes requires code change not just dep upgrade

also wonder if ABI works since it ignores bodies right?

[gnodet]

Thanks for the review @rmannibucau. Two points:

ASM vs ClassFile API: Replied inline — the short version is that the ClassFile API (java.lang.classfile) was only finalized in JDK 24, and we target JDK 17. ASM is already a dependency (ByteCodeTransformer). Happy to add a dual-path implementation (ClassFile API preferred on JDK 24+, ASM fallback) if you think it's worth the added complexity now, or track it as a follow-up issue.

"does ABI work since it ignores bodies?": Yes, that's exactly the point — and it's well-established. ABI (Application Binary Interface) fingerprinting deliberately ignores method bodies because downstream dependents can only depend on the signatures, not the implementation. If you change a method body but keep the same return type, parameter types, exceptions, and modifiers, no downstream class needs to be recompiled — the JVM spec guarantees binary compatibility. This is the same principle used by:

• Gradle's ABI-based incremental compilation (since Gradle 3.4)
• Eclipse JDT's structural delta detection
• Zinc (sbt) incremental compiler

The fingerprint covers: class modifiers, superclass/interfaces, non-private fields (types + constant values for inlined static final), non-private methods (return type, parameter types, exception types, modifiers), and annotations. A change to any of those means the ABI changed → dependents are recompiled. A change to only method bodies → fingerprint stays the same → dependents are not recompiled → faster builds.

The AbiExtractor (javac element API, used during compilation) and BytecodeAnalyzer (ASM, used for external deps) both produce canonical forms that capture exactly this surface.

[rmannibucau]

@gnodet on my side i would be ok to only support it for java 24+ and ignore asm, it is the way to go and agree 2 impl are not worth it (but asm costs more than it helps today IMHO).

On ABI I agree with the downstream support but somehow we should have the module support without relying on git. While ABI is 100% for upstream/downstream deps it is awesome but means we have 2 incremental mechanism, I would hope a centralised state with potentially both info (ABI and module fingerprints) to avoid to spread it

[gnodet-bot]

Re-review — JDK < 24 build breakage

Commit 35bb1d2 addresses rmannibucau's feedback by introducing a dual-path ClassAnalyzer strategy (ASM on JDK <24, java.lang.classfile on JDK 24+). The refactoring itself is clean — the ClassAnalyzer interface, the Sha256 extraction, and the selectAnalyzer() reflection fallback are well-designed. However, the build configuration will break on any JDK below 24.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet]

Commit 950b860 addresses both issues from the latest review:

1. pom.xml — Moved compile-java24 execution into a java24+ activation profile (<jdk>[24,)</jdk>). On JDK 17/21, the execution is skipped and only ASM is used. On JDK 24+, ClassfileClassAnalyzer is compiled and selected at runtime.

2. BytecodeAnalyzerTest — Added @EnabledForJreRange(min = JAVA_24) and switched to reflective instantiation (same pattern as BytecodeAnalyzer.selectAnalyzer()) to avoid a compile-time dependency on the JDK 24+-only class.

[gnodet-bot]

Re-review — JDK < 24 build breakage fixed ✅

Verified commit 950b860 against each finding from the previous REQUEST_CHANGES review:

Finding	Status
pom.xml — compile-java24 execution in main <build> breaks JDK < 24 (--release 24 unsupported)	✅ Moved into java24+ profile with <jdk>[24,)</jdk> activation. Profile is inert on JDK 17/21.
BytecodeAnalyzerTest — compile-time reference to ClassfileClassAnalyzer breaks on JDK < 24	✅ Added @EnabledForJreRange(min = JAVA_24) + reflective instantiation via Class.forName(). Javadoc correctly changed from {@link} to {@code} since the class may not be on the compilation classpath.

Both fixes are correct and complete. The reflective fallback in selectAnalyzer() handles the missing class gracefully (catch Exception | LinkageError), so the JAR built on JDK < 24 (without ClassfileClassAnalyzer.class) will still work — it just always uses AsmClassAnalyzer. No new issues introduced.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Re-review — JDK 25 enforcer + unconditional build

Commit 8aec6e4 reverses the previous java24+ profile approach and instead requires JDK 25+ at build time via enforcer, moving compile-java24 back into the unconditional <build> section. The design intent is sound — every build always produces ClassfileClassAnalyzer.class, eliminating the conditional-compilation headache.

However, the CI workflow hasn't been updated to match this new build requirement.

Finding	Status
pom.xml — enforcer [25,) vs CI JDK matrix	🔴 CI will break — see inline comment
ASM kept as runtime fallback for JDK < 24	✅ Reasonable — selectAnalyzer() reflective loading is correct. ASM is already a dependency. rmannibucau's "drop ASM" suggestion is partially addressed (build-time only); fully dropping ASM would remove bytecode analysis for JDK 17-23 users, which is a separate design decision.
Previous java24+ profile fix	Superseded — new approach is cleaner but needs CI fix

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Re-review — CI fix is correct ✅

Verified commit 9a5c56d against the finding from the previous review:

Finding	Status
CI will break — enforcer [25,) vs default JDK matrix ["21", "25", "8"]	✅ Fixed — ff-jdk: '25', jdk-matrix: '["25"]', dropped Microsoft distribution (no JDK 25 available). CI matrix now matches the enforcer requirement.
BytecodeAnalyzerTest — reflective instantiation reformatted	✅ Cosmetic only — no functional change

All previous findings from earlier reviews remain addressed. No new issues introduced.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet]

@rmannibucau Thanks for the follow-up.

ASM removal: Commit 8aec6e4 moved in that direction — the build now requires JDK 25+ (enforcer) and ClassfileClassAnalyzer is always compiled. However, ASM is still kept as a runtime fallback in selectAnalyzer() for the edge case where the plugin JAR ends up running on JDK < 24 (which shouldn't happen with the enforcer, but belt-and-suspenders). If you'd prefer to hard-drop ASM entirely and make JDK 24+ a strict runtime requirement too, I can remove AsmClassAnalyzer and simplify selectAnalyzer() to just instantiate ClassfileClassAnalyzer directly. That would also let us drop the ASM dependency from this module (though it's still used by ByteCodeTransformer for the JDK-8318913 workaround — separate concern).

Would you prefer the hard-drop approach?

Centralised state: Agreed that having a single state file carrying both ABI fingerprints and module-level metadata would be cleaner than having two separate incremental mechanisms. The current implementation already stores everything in a single versioned binary state file (AbiIncrementalBuild.State), so extending it with module-level info (exports, requires, etc.) should be straightforward. I'd prefer to track that as a follow-up issue though — the current PR is already substantial, and the module integration is an additive feature. Sound good?

[gnodet-bot]

Re-review — Javadoc fix is correct ✅

Commit ea53ebb replaces {@link ClassfileClassAnalyzer} with {@code ClassfileClassAnalyzer} and removes @see ClassfileClassAnalyzer. Correct — ClassfileClassAnalyzer is compiled under --release 24 in a separate source root and may not be on the Javadoc classpath when building on JDK < 25, so {@link}/@see would produce broken references. {@code} is the right tag here.

No functional change. All previous findings remain addressed.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Re-review — Inner class cleanup, ClassAnalyzer refactoring, and JPMS support

Verified commits 122d0a0, 38336d7, and 1f72731 against the full PR context.

Commit 122d0a0 — Stale inner class cleanup on recompile

initIncrementalBuild() now calls deleteClassFile() for every previously-known type in a source file before clearing its state entries. This closes the gap where modifying a source file that previously had inner classes would leave stale $-named class files on disk. Correct — the files are being recompiled anyway, so deleting first is safe and the $-prefix scan pattern from cf283d0 handles nested classes correctly.

Commit 38336d7 — Extract abstract ClassAnalyzer base

Extracted to base	Status
FieldInfo / MethodInfo records	✅ MethodInfo correctly drops the unused exceptions field (never appeared in canonical form)
buildCanonicalForm()	✅ Identical logic to both previous implementations
appendAccessFlags()	✅ Same flag order, same bitmask values
resolveInternalName()	✅ Same null/primitive/array-unwrap logic as the inlined addInternalName() in both analyzers
isPrivateOrSynthetic()	✅ Replaces repeated (access & PRIVATE) == 0 && (access & SYNTHETIC) == 0 checks
ClassfileClassAnalyzer.accessMask()	✅ Correctly converts Set<AccessFlag> to int bitmask via flag.mask() — JVM spec guarantees same values

Clean refactoring, no behavioral change. Both analyzers now produce identical canonical forms through the shared code path.

Commit 1f72731 — JPMS support

Component	Status
AbiExtractor.moduleCanonicalForm()	✅ All 5 JPMS directive types handled (requires/exports/opens/uses/provides). TreeSet ensures deterministic ordering. Qualified to/with clauses sorted.
CompilationAnalyzer.analyzeModule()	✅ Correctly detects module-info via cu.getModule(). MODULE_PREFIX avoids name collisions with regular types. uses/provides types correctly tracked as signature deps; requires/exports/opens correctly excluded (module/package names, not types).
AbiIncrementalBuild.deleteClassFile() module handling	✅ MODULE_PREFIX check → deletes module-info.class. Early return correctly skips inner class cleanup (module-info has no inner classes).
ToolExecutor reactor path detection	✅ Files.isDirectory() correctly distinguishes reactor output dirs from external JARs on MODULE_PATH. LinkedHashSet appropriate for dedup.
5 new tests	✅ Covers full build tracking, incremental detection, fingerprint change, and inner class cleanup. Modular compilation correctly sets SOURCE_PATH.

No new issues introduced. All previous findings from earlier reviews remain addressed.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Re-review — Inner class cleanup, ClassAnalyzer refactor, and JPMS support

Verified 3 new commits (122d0a0, 38336d7, 1f7273) against all previous findings and for new issues:

Commit 1: 122d0a0 — Inner class cleanup on recompilation

Aspect	Status
Stale Foo$Bar.class files deleted before recompilation	✅ Calls deleteClassFile() for all types from previousState.getTypesFromSource() before removeTypesForSource()
module: prefix handled in deleteClassFile()	✅ Early return mapping to module-info.class
Test coverage	✅ removedInnerClassDeletesStaleClassFile() exercises full cycle

Commit 2: 38336d7 — Extract abstract ClassAnalyzer base

Aspect	Status
FieldInfo / MethodInfo shared records	✅ Semantically identical — old AsmClassAnalyzer.MethodInfo had unused exceptions field, old ClassfileClassAnalyzer.MethodInfo already lacked it
buildCanonicalForm() produces identical output	✅ Same field/method iteration, same access flag ordering, same EXCLUDED_SUPERTYPES set
resolveInternalName() consolidates array unwrapping	✅ Same logic as removed addInternalName() in both implementations
isPrivateOrSynthetic() replaces inline checks	✅ Identical bitmask logic
ClassfileClassAnalyzer uses accessMask() to convert Set<AccessFlag> → int	✅ Correct — AccessFlag.mask() returns the JVM spec constants

Commit 3: 1f7273 — JPMS support

Aspect	Status
CompilationAnalyzer.analyzeModule() — module-info tracked with module: prefix	✅ Clean separation from type analysis
AbiExtractor.moduleCanonicalForm() — deterministic with sorted TreeSet per directive type	✅ Requires/exports/opens/uses/provides all sorted independently
Module dependency tracking — uses/provides as signature deps, requires/exports/opens NOT tracked as deps	✅ Correct — requires/exports/opens affect the module descriptor ABI (captured by fingerprint) but don't represent type-level dependencies that would cascade
ToolExecutor — reactor module paths collected from MODULE_PATH directory entries	✅ Passed to AbiIncrementalBuild.setReactorModulePaths()
Test coverage	✅ 4 new tests: full build tracking, incremental detection, fingerprint change on directive change, stale class cleanup

All previous findings from earlier reviews remain addressed. No new issues introduced. Clean refactoring with good test coverage.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Re-review — MODULE_SOURCE hierarchy support is correct ✅

Verified commit ef507f4 (fix: handle MODULE_SOURCE hierarchy in ABI incremental class file paths) against the full PR context.

Component	Status
ToolExecutor — passes useModulePrefixedPaths flag after determineDirectoryHierarchy()	✅ Correct timing — flag is set before compilation starts
AbiIncrementalBuild.processRound() — gates module name storage on useModulePrefixedPaths	✅ Non-modular projects always store empty module name, avoiding spurious path prefixes
AbiIncrementalBuild.deleteClassFile() — now takes TypeInfo parameter for module-aware path resolution	✅ baseDir = moduleName.isEmpty() ? outputDir : outputDir.resolve(moduleName) correctly handles both modular and non-modular cases. Null info safely defaults to empty module name
CompilationAnalyzer.resolveModuleName() — uses Elements.getModuleOf() with defensive catch (Exception)	✅ Correct — javac API behavior is implementation-dependent; unnamed/null modules return empty string
CompilationAnalyzer.analyzeModule() — passes moduleName from moduleTree.getName() to SourceFileAnalysis	✅ Module-info entries correctly carry their module name for path resolution
IncrementalState v5→v6 migration	✅ version >= 6 ? in.readUTF() : "" in load() preserves backward compatibility. Old v5 state files trigger full rebuild (expected, not lossy). save() always writes module name
SourceFileAnalysis record — new moduleName field	✅ Clean extension with Javadoc
IncrementalStateTest — updated test data	✅ Tests pass empty module name for non-modular scenarios

All previous findings from earlier reviews remain addressed. No new issues introduced. Clean, focused commit.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Re-review — Module name change detection is correct ✅

Verified commit 5e0fb8c (fix: force full rebuild when JPMS module name changes) against the full PR context.

Component	Status
hasModuleNameChanged() — compares module: prefixed keys between current and previous state	✅ Correct — runs after state.setType() has added the new module entry and after initIncrementalBuild() has removed the old one via removeTypesForSource(). The set comparison correctly detects renamed/added/removed modules.
forceFullRebuild() — deletes stale class files for uncompiled types, returns all remaining source files	✅ Correct — deleteIfExists makes re-deletion of already-cleaned files a no-op. The !allCompiled.contains(sf) guard correctly skips files compiled in the current round (e.g. the module-info itself).
Placement in processRound() — after abiChanged.isEmpty() early-exit	✅ Safe — a module name change always produces an ABI change (different canonical form in AbiExtractor.moduleCanonicalForm()), so abiChanged is non-empty and the early-exit doesn't fire.
Test moduleNameChangeTriggersFullRebuild()	✅ Exercises the full cycle: initial build → rename module → verify cascade returns all files → verify final state has new name and not old.

All previous findings from earlier reviews remain addressed. No new issues introduced.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Re-review — Follow-up integration fixes and test coverage

Verified 4 new commits (61b5951, 707404d, 50e1021, bbea62f) since the last review.

Change	Status
State collision fix (state file moved to outputDir)	✅ Correct — prevents compile and test-compile from clobbering each other's state. buildDir still used for ABI manifest (correct — downstream modules look there).
Multi-release JAR (isBaseRelease guard)	✅ Correct — only the base release gets the ABI analyzer attached. Versioned releases are compiled fully.
Annotation processor path wiring	✅ Correct — pattern-matches JavaPathType for ANNOTATION_PROCESSOR_PATH / MODULE_PATH, consistent with existing classpath wiring above.
invalidate() on compilation failure	✅ Correct — called before throwing CompilationFailureException, ensures next build starts fresh.
Sealed class permits in fingerprint	✅ Correct — permits list is sorted (declaration order doesn't matter semantically; determinism matters).
Enum constant declaration-order fingerprinting	✅ Correct — getEnclosedElements() preserves source order, and ordinal() depends on it, so reordering must change the fingerprint. Enum constants are excluded from the sorted members stream to avoid double-emission.
6 new tests	✅ Well-designed — test meaningful behavior (sealed permits, enum reordering, package-info, state isolation, separate output dirs, invalidation).
ForkedTool warning	⚠️ See inline comment

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Re-review — ForkedTool invalidation, config hash tracking, and generic signatures

Verified 3 new commits (6545041, e79f971, ea54371) against all previous findings.

Commit e79f971 — Invalidate ABI state for forked compilation fallback

Finding from previous review	Status
ToolExecutor — warning says "Falling back to full compilation" but initialize() returns only changed files when state exists	✅ Fixed — abiBuild.invalidate() deletes the state file before initialize(), so load() returns null → initFullBuild() is triggered. The warning message now matches the actual behavior.

Correct and complete. The invalidate() method uses Files.deleteIfExists() with best-effort error handling, which is appropriate — a missing state file simply triggers a full rebuild on next initialize().

Commit 6545041 — Track module-info-patch.maven changes in ABI incremental

Component	Status
AbiIncrementalBuild.setConfigHash()	✅ Null-safe (hash != null ? hash : ""), called before initialize()
AbiIncrementalBuild.initialize() — config hash comparison	✅ !configHash.equals(previousState.getConfigHash()) triggers full rebuild when patch files change. Empty string default on both sides means no false rebuilds when no patch files exist.
AbiIncrementalBuild.finish() — persists config hash	✅ state.setConfigHash(configHash) before save()
IncrementalState v6→v7 migration	✅ version >= 7 ? in.readUTF() : "" preserves backward compatibility. Old v6 state files load with empty configHash, matching the default — no spurious full rebuilds on upgrade.
IncrementalState.copy()	✅ Copies configHash field
ToolExecutor.computeModuleInfoPatchHash()	✅ Iterates source directories, hashes each module-info-patch.maven file with SHA-256, then hashes the concatenated result. Handles I/O errors gracefully (marks as "unreadable" which changes the hash → triggers rebuild, correct behavior).
Test: configHashChangeTriggersFullRebuild()	✅ Covers 3 scenarios: first build, same hash (incremental), different hash (full rebuild)

Commit ea54371 — Include generic signatures in bytecode ABI fingerprint

This fixes a real correctness gap: AbiExtractor (javac element API, used during compilation) already captures generic types naturally via TypeMirror.toString() (e.g., List<String>), but the bytecode analyzers (AsmClassAnalyzer, ClassfileClassAnalyzer) were using only the erased JVM descriptor. Changing List<String> to List<Integer> on a public method produced identical bytecode ABI fingerprints — so cross-module ABI change detection would miss the generic type change.

Component	Status
ClassAnalyzer.FieldInfo record — new signature field	✅ Nullable, from the JVM Signature attribute
ClassAnalyzer.MethodInfo record — new signature field	✅ Same pattern
ClassAnalyzer.buildCanonicalForm() — new classSignature parameter	✅ All three levels (class, field, method) append <sig: ...> when signature is present. Deterministic — same signature always produces same canonical form.
AsmClassAnalyzer — captures signature from ASM visitor callbacks	✅ visit() stores class signature, visitField()/visitMethod() pass signature through to records
ClassfileClassAnalyzer — captures Signature attribute via ClassFile API	✅ Uses findAttribute(Attributes.signature()) for class, fields, and methods. .map(s -> s.signature().stringValue()).orElse(null) is clean.
Test: genericTypeChangeAffectsFingerprint()	✅ List<String> → List<Integer> produces different fingerprints. Verifies canonical form contains <sig:.
Test: classLevelGenericSignatureAffectsFingerprint()	✅ Box<T> → Box<T extends Comparable<T>> produces different fingerprints.

All previous findings from earlier reviews remain addressed. No new issues introduced.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Re-review — useIncrementalCompilation=false bypass is correct ✅

Verified commit b9be80a (fix: respect useIncrementalCompilation=false with ABI strategy):

Change	Status
compile() — !Boolean.FALSE.equals(useIncrementalCompilation) guard	✅ Correct. null (default) and true → ABI path. false → falls through to timestamp path, which maps to EnumSet.of(CLASSES) (full recompile). Uses Boolean.FALSE.equals() rather than unboxing, so null-safe.
Javadoc — incrementalStrategy	✅ Accurately documents: ABI doesn't use staleMillis/incrementalExcludes/incrementalCompilation aspects; forked compilation falls back to full; useIncrementalCompilation=false disables. Also adds JPMS support docs and sealed/enum/generics coverage.
Javadoc — timestamp strategy	✅ Now documents that it respects incrementalCompilation aspects, staleMillis, and incrementalExcludes — previously implicit.

All previous findings from earlier reviews remain addressed. No new issues.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Re-review — IT verify script path fix is correct ✅

Verified commits 53ffbe69 and c829e204 (merge) against the full PR context.

Finding	Status
verify.groovy in abi-incremental-basic and abi-incremental-cascade — asserted .incremental-state in target/	✅ Fixed — now correctly asserts target/classes/.incremental-state, matching AbiIncrementalBuild constructor (outputDir.resolve(".incremental-state") where outputDir = classes dir)
.abi-fingerprints still asserted at target/	✅ Correct — manifest is written via buildDir (outputDir.getParent())

All previous findings from earlier reviews remain addressed. No new issues introduced.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Re-review — showCompilationChanges wiring, state version reset, SHA-256 unification

Verified commit 60f4565 against all previous findings and for new issues.

Change	Status
ToolExecutor — wire showCompilationChanges for ABI rebuild cause and file list	✅ Correct — getRebuildCause() null-guarded, cascade files logged separately. showCompilationChanges is an existing Mojo parameter (not introduced by this PR).
IncrementalState — VERSION 7→1, remove version-conditional reads	✅ Correct — this feature is unreleased, so no production state files need migration. All version-conditional branches in load() (v2/v3/v4/v5/v6/v7 guards) removed; load now reads unconditionally. Clean simplification.
AbiIncrementalBuild — replace inline sha256() with Sha256.hash()	✅ Correct — hash truncation from 64→16 hex chars is safe: 64-bit space gives collision probability ~10⁻¹¹ even with 50k types (birthday bound). Both ABI fingerprints and source content hashes now use the same utility. The version reset to 1 ensures no old 64-char hashes are compared against new 16-char ones.
AbiIncrementalBuild — rebuildCause field and cause descriptions	✅ Correct — set for all initialize() paths: no previous state, config changed, and incremental (changed/new/deleted/invalidated counts). initIncrementalBuild() returns Set.of() when nothing changed, so rebuildCause stays null — correctly handled by the null check in ToolExecutor.
Comment cleanup in IncrementalState.save()	✅ Cosmetic — version-tracking comments (// v2:, // v5:, etc.) removed since version-conditional logic is gone.

All previous findings from earlier reviews remain addressed. No new issues introduced.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Re-review — Javadoc fixes and rebuild cause tests ✅

Verified commit 794575a against the full PR context.

Change	Status
AbiExtractor javadoc — added module descriptor fingerprinting paragraph	✅ Accurate — computeModuleFingerprint() exists and handles all 5 JPMS directive types
AbiIncrementalBuild javadoc — updated usage example with setProcessorPath/setConfigHash/invalidate() on failure	✅ Matches actual API. State file location corrected from "in the output directory" to "inside the output directory" for .incremental-state and "in the build directory" for .abi-fingerprints — matches code (entry.getParent()) and IT assertions (target/.abi-fingerprints)
CompilationAnalyzer javadoc — module-info.java handling via AbiExtractor.computeModuleFingerprint	✅ Accurate reference
noChangeReturnsEmpty — added assertNull(getRebuildCause())	✅ Correct — rebuildCause is only set in initialize() when changes are detected
rebuildCauseDescribesChanges — asserts cause contains "changed" and "new"	✅ Matches causes.add(changedFiles.size() + " changed") and causes.add(newFiles.size() + " new")
fullBuildCauseDescribed — asserts cause contains "no previous"	✅ Matches rebuildCause = "no previous build state"

All previous findings from earlier reviews remain addressed. No new issues.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet]

Thanks for the detailed feedback — addressing each point:

1. Two things in one PR (dependency scanning + ABI compatibility)

The two are actually inseparable here. Dependency tracking already exists in the timestamp strategy — it's what drives which files get recompiled when a source changes. The new ABI strategy builds on top of that: the dependency graph is the same mechanism, ABI fingerprinting is the only new thing it adds. Without the fingerprint comparison, the ABI strategy would just be a slower version of timestamp (same recompilation decisions, plus the overhead of reading .class files). So the ABI computation isn't a separate concern that can be deferred — it is the strategy.

2. ABI formatted as String before SHA-256

Agreed — this is a known inefficiency. The canonical form is a human-readable string hashed as UTF-8 bytes. The upside is debuggability (the canonical is stored in the state file and diffable). The downside is the unnecessary string allocation and encoding step. We note it as a known trade-off; a binary encoding could be introduced if it seems preferred.

3. ACC_PUBLIC etc. duplicating java.lang.reflect.Modifier

Already fixed — the current code uses java.lang.reflect.AccessFlag (standard API since JDK 20) rather than raw integer constants. AccessFlag is more precise than Modifier for classfile-level flags (it distinguishes PUBLIC on a field vs. a class, handles MANDATED, etc.). No duplication of ASM or classfile constants in the current implementation.

4. Dependency on jdk.compiler

This is no longer the case. The current implementation analyses .class files after compilation using java.lang.classfile (JDK 24 standard API) — there is no dependency on jdk.compiler, no com.sun.* or jdk.* internal APIs. Analysis is fully post-compilation and compiler-agnostic: it works with javac, ECJ, or any compiler that produces standard .class files. Forked mode is supported for the same reason.

5. Annotation processor classification via non-standard property files

Fair concern. The META-INF/gradle/incremental.annotation.processors file is a Gradle convention; META-INF/javaci/incremental.annotation.processors was an earlier Maven-side proposal. The rationale for reading both is pragmatic: many popular processors (Lombok, MapStruct, Immutables) already ship the Gradle file, so we get correct classification for free without requiring them to add a Maven-specific file. That said, if the project prefers not to take a dependency on a Gradle convention, we can drop the Gradle path and rely solely on META-INF/javaci/ — with the expectation that processor authors would need to add it explicitly. We're open to either.

On your proposed implementation strategy

The MRJAR setup (your step 1) is already done in this PR — happy to extract it as a standalone PR if that makes the review easier. Steps 2–4 broadly align with the current direction: post-compilation .class analysis, java.lang.classfile only, no jdk.compiler dependency. The main divergence is on ABI fingerprinting, which as noted in point 1 above is the core of the strategy rather than an optional add-on.

[gnodet-bot]

Re-review — Processor classification path rename is correct ✅

Verified commit 31504eb (rename: META-INF/javaci -> META-INF/maven/compiler for processor classification).

Component	Status
ProcessorClassification — JAVACI_RESOURCE → MAVEN_COMPILER_RESOURCE, path META-INF/maven/compiler/incremental.annotation.processors	✅ Consistent rename, both directory and JAR loading updated
AbiManifest — #javaci:v1 → #maven-compiler:v1 header	✅ Reader updated (startsWith("#maven-compiler:v")), version extraction substring length matches
AbiIncrementalBuild / ExternalAbiResolver — Javadoc references to "javaci"	✅ Updated to "maven-compiler-plugin"
Tests — all 8 test methods in ProcessorClassificationTest + 2 in AbiManifestTest	✅ Paths and headers updated consistently
Grep for remaining javaci in full PR diff	✅ Zero matches — rename is complete

Clean branding rename with no behavioral change. No new issues introduced.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[desruisseaux]

Why a hidden file? (the . prefix).

[desruisseaux]

The answer to feedback 1 seems strange (maybe it was generated by AI)? Dependency tracking never existed before this pull request. We only had a direct, one-to-one, relationship between modified files (as detected by timestamp) and recompiled files. We could very well add dependency graph without ABI. They are two orthogonal axes:

• Dependency graph replaces the one-to-one relationship between modified files and recompiled files by a one-to-many relationship, but the modified files can still be detected by timestamp.
• ABI replaces "modified files" by "effectively modified files" (from the point of view of a consumer of the API).

Committing the two concepts in two steps would make the reviews easier. For example, one thing that I would like to check is whether the .abi-incremental-state file created by this pull request could be merged with the existing file generated by the current version based on timestamps.

[gnodet-bot]

Re-review — Broken cascade IT after ABI_MARKER removal

Commit 1ba1359 addresses most review comments correctly (Javadoc, @snippet, pom comment, CompilerTestHelper N×M fix). However, it introduced a regression in the cascade integration test.

Change	Status
pom.xml — comment wording "via the classesDirectory parameter"	✅ Correct
ToolExecutor — Javadoc "Compiles sources and performs ABI analysis..."	✅ Correct
AbiIncrementalBuild — {@snippet : replaces <pre>{@code	✅ Correct
CompilerTestHelper — remove N×M walk redundancy, single Files.walk	✅ Correct
Model.java — removed // ABI_MARKER	🔴 Breaks cascade IT — see inline

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

🔴 Critical: The // ABI_MARKER comment was removed here, but the Groovy script in pom.xml (line 56) still does text.replace('// ABI_MARKER', 'public boolean isValid() { ... }'). Since String.replace() silently returns the original string when the target isn't found, step 2 of the IT will appear to succeed but won't actually modify Model.java — step 3's incremental build will see no source changes and the cascade behavior goes untested.

The IT becomes a false green: it passes but tests nothing.

Either restore the marker:

Suggested change

	}
	}
	// ABI_MARKER
	}

Or update the Groovy script in pom.xml to use a different insertion strategy (e.g. text.replace('}\n}', 'public boolean isValid() { return name != null; }\n}\n}')).

[gnodet]

You're right to call out both the factual error and the design point.

The answer to feedback 1 was incorrect. The DEPENDENCIES aspect detects that a classpath JAR changed (by mtime) and triggers a full rebuild of all sources — it is not a dependency graph. There is no tracking of which source files use which classes. The response conflated two different things and was wrong.

Your two axes are correct and orthogonal:

• Dependency graph: replace the module-level "recompile everything" with selective recompilation — track which source files transitively depend on a changed class, recompile only those. Change detection is still by content hash or mtime; any change to a class cascades to all its source-level consumers.
• ABI fingerprinting: prune the cascade further — only recompile consumers of a class whose public API surface actually changed. Private body changes produce no cascade.

Both are genuinely useful independently. A dependency graph without ABI is already a significant improvement over today's module-level rebuild for projects where most changes touch isolated classes.

To make the review more tractable, I'm splitting this into three PRs:

1. MRJAR infrastructure — BytecodeAnalyzer with java.lang.classfile on JDK 24+, ASM fallback on older JDKs, proper multi-release JAR packaging. No behavior change.
2. Dependency graph strategy (incrementalStrategy=graph) — post-compilation bytecode walk to extract class-level deps, content-hash change detection, cascade on any class change. No ABI.
3. ABI fingerprinting (incrementalStrategy=abi) — adds the public API surface comparison on top of the graph; private body changes no longer cascade.

On merging .abi-incremental-state with the existing timestamp state file: the two formats currently serve different strategies and have different contents (the graph/ABI state is richer). Happy to discuss a unified format — the key question is whether the graph strategy should replace the timestamp strategy or coexist alongside it as a separate opt-in. I'd lean toward coexistence for now, with a migration path later.

[desruisseaux]

We mean "cross sub-project"?

[desruisseaux]

But which Maven plugins would use that?

[desruisseaux]

Why converting internal names to human-readable names? This is for internal purpose only. Using the internal Java name would be faster (no conversion), shorter (less data to process for computing SHA) and reduce the complexity of the code.

[desruisseaux]

Same comment as above: we could keep the internal method signature as stored in the JVM and remove all this conversion to human-readable form.

[desruisseaux]

Those constants are already available in java.lang.reflect.Modifier.

[desruisseaux]

Just store the int flag in binary, no need to make it human-readable.

[gnodet-bot]

Re-review — Annotation processor simplification is correct ✅

Verified commit a6aa54c (rebase on top of #1151 + #1152) against the full PR context.

Component	Status
amendincrementalCompilation() — simplified from 3-branch conditional to unconditional REBUILD_ON_ADD + REBUILD_ON_CHANGE	✅ Correct — the old aspects.clear() + NONE path (proc=only/full ⇒ disable incremental entirely) was overly conservative. With the ABI strategy handling processors via ProcessorClassification, the timestamp strategy no longer needs to completely bypass incremental compilation. The isAbsent(incrementalCompilation) guard still protects explicit user config.
Removed tests testCompilerProcOnlyRunsWhenSourcesAreUnchanged / testCompilerProcFullRunsWhenSourcesAreUnchanged	✅ These verified the old NONE behavior (proc=only/full ⇒ always recompile). That behavior is intentionally removed — correctly deleted.
Retained test testCompilerProcOnlyRespectsExplicitIncrementalCompilation	✅ Still valid — verifies explicit incrementalCompilation config takes precedence over processor detection.
Removed test resources compiler-proc-only-test/, compiler-proc-full-test/	✅ No longer referenced by any test.
hasAnnotationProcessor() Javadoc — removed "In case of doubt" qualifier	✅ Correct — the method no longer drives JDK-version-dependent branching, so the caveat is no longer relevant.

All previous findings from earlier reviews (exception narrowing, sourceFiles restore, inner class cleanup, MRJAR infrastructure, CI fix, ForkedTool invalidation) remain addressed in the rebased commits. No new issues introduced.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[desruisseaux]

Okay, it answers my previous question about who would use the manifest. But if this is about adding the manifest inside the JAR file, this is a whole new topic. It brings the question of our level of commitment to this format, the compatibility with future versions, etc. Can we leave that for a separated pull request?

[desruisseaux]

I started the review, but it will be large effort to complete it. Can we break this pull request into smaller ones? My analysis is that this pull request mixes many aspects that could be treated independently:

1. Adding dependency graph for replacing the one-to-one relationship by one-to-many between modified files and compiled files.
2. Adding ABI for replacing "modified files" by "effectively modified files" from the point of view of API consumers.
3. Saving ABI in a manifest file for detecting when a change in a sub-project requires a recompilation of another sub-project.
4. Detecting the type of annotation processors (using non-standard metadata) for deciding which change detection strategy to use.

I don't see any reason for addressing those 4 aspects in the same pull request. In my opinion, they can be addressed separately. Can we focus on 1 only for starting?

[gnodet-bot]

Re-review — Post-consolidation review (force-push incorporating #1151 and #1152)

This PR was force-pushed to consolidate the 3-PR stack (#1151 MRJAR infrastructure, #1152 ABI strategy, #1144 annotation processor handling) into a single PR. All 34 commits are new relative to the previous review baseline. Verified against the full diff.

Architecture

The post-compilation bytecode analysis approach (scan .class files after javac completes) is the right design — it decouples the ABI engine from javac internals, works with any compiler invocation mode, and aligns with desruisseaux's recommendation (feedback 4: "instead of doing analysis during compilation, do the analysis of .class files after compilation"). The multi-release JAR structure (src/main/java24/ with ClassfileClassAnalyzer) is clean.

Generic signature parser

The parseSig/parseClassTypeSignature recursive descent parser (JVMS §4.7.9.1) is correct. The inFormalTypeParams mode correctly distinguishes FormalTypeParameters (class/method declarations) from TypeArguments (usage sites), and the test coverage for T-prefixed parameter names and multi-param bounds confirms the fix for the earlier parsing bug.

Static method duplication in multi-release JAR

The utility methods (descriptorToReadable, parseParams, parseReturn, toJavaName) are duplicated verbatim between src/main/java/...BytecodeAnalyzer.java and src/main/java24/...BytecodeAnalyzer.java. This is an inherent limitation of the multi-release JAR approach (the versioned class fully replaces the root class, so static methods can't be inherited). Acceptable trade-off — any future changes to these methods must be mirrored in both files.

One issue found — see inline comment

All previous findings from earlier reviews remain addressed. No regressions introduced by the consolidation.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

💡 Low — Wasteful iteration in test helper. The outer for (Path sf : sourceFiles) loop walks the entire output directory once per source file, but the inner walk doesn't filter by sf — every .class file is analyzed and mapped to sf.toString() regardless of which source file produced it. The results map deduplicates by className (last writer wins), so this doesn't cause test failures, but it means:

1. Class files are analyzed O(sourceFiles × classFiles) times instead of O(classFiles) once
2. Every type gets attributed to the last source file in the iteration, not the one that actually defines it

Since this is test-only code and the sourceFile field isn't validated in these tests, this is cosmetic — but it would break if a test ever asserted sourceFile accuracy from this helper.

Suggested change

	analysis.annotationTypes(),
	// Build a minimal SourceFileAnalysis map from bytecode (no dep graph needed here)
	Map<String, SourceFileAnalysis> results = new java.util.LinkedHashMap<>();
	try (Stream<Path> walk = Files.walk(outputDir)) {
	walk.filter(p -> p.toString().endsWith(".class")).forEach(cf -> {
	try {
	var analysis = BytecodeAnalyzer.analyze(cf);
	var sfa = new SourceFileAnalysis(
	analysis.className(),
	cf.toString(),
	analysis.signatureTypes(),
	analysis.implementationTypes(),
	analysis.abiFingerprint(),
	analysis.abiCanonical(),
	analysis.annotationTypes(),
	analysis.moduleName());
	results.put(analysis.className(), sfa);
	} catch (IOException e) {
	// best effort
	}
	});
	}

[gnodet-bot]

Re-review — Rebased combined stack (MRJAR + graph + ABI)

Verified the rebased PR (11 commits ahead, diverged from master) that consolidates the full incremental compilation stack. All previous findings from earlier reviews remain addressed. Found 3 new issues introduced by the rebase/consolidation.

Finding	Status
Previous: DependencyScanner broad catch	✅ Still fixed
Previous: ToolExecutor sourceFiles restore	✅ Still fixed (try/finally in compileWithGraphIncremental)
Previous: Inner class cleanup	✅ Still fixed ($-prefix scan)
Previous: ExternalAbiResolver silent IOException	✅ Still fixed (FINE logging)
Previous: mtime-first optimization	✅ Implemented (hashSourceFiles())
Previous: JDK <24 build / CI	✅ Enforcer [25,) + CI jdk-matrix: [25]
Previous: ForkedTool fallback	Superseded — not in this PR's scope
NEW: abi strategy Javadoc misleading	🔴 See inline
NEW: abi-incremental-basic IT will fail	🔴 See inline
NEW: MODULE_PATH entries not tracked	💡 See general comments

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

⚠️ Warning (medium): abi strategy Javadoc is misleading. The description says:

only files whose ABI actually changed cascade to their consumers; body-only changes recompile only the changed file

But processCompiledClasses() cascades ALL compiled types regardless of whether their ABI fingerprint changed (line ~2108: changedTypes = new TreeSet<>(results.keySet())). The abi flag only controls whether ABI fingerprints and fine-grained sig/impl deps are stored — it doesn't reduce cascading based on fingerprint comparison.

The actual distinction between graph and abi is:

• graph: all deps go into signatureConsumersIndex → transitive cascade for everything
• abi: separate sig/impl indexes → transitive cascade only through signature consumers; implementation consumers cascade directly but not transitively

Plus abi writes the abi-fingerprints manifest for cross-module tracking.

Either implement ABI-fingerprint-based cascade pruning in processCompiledClasses() (compare new fingerprint vs stored fingerprint, skip cascade if unchanged), or fix the Javadoc to accurately describe the current behavior:

Suggested change

	* changes, only files whose ABI actually changed cascade to their consumers; body-only
	* changes recompile only the changed file. Accepted as an alias for {@code graph}.</li>
	* </ul>
	*
	* @since 4.0.0-beta-7
	*/
	@Parameter(property = "maven.compiler.incrementalStrategy", defaultValue = "timestamp")
	* <li>{@code abi} — ABI-aware dependency-graph strategy. Extends {@code graph} with
	* fine-grained signature/implementation dependency classification and ABI fingerprints.
	* Signature changes cascade transitively to consumers; implementation-only dependency
	* changes cascade directly but not transitively. Writes an ABI manifest for cross-module
	* incremental detection in reactor builds.</li>

[gnodet-bot]

🔴 Critical: IT will fail — manifest assertion vs strategy mismatch. This IT uses incrementalStrategy=graph, but verify.groovy (line 14 of that file) asserts that target/abi-fingerprints exists. The manifest is only written when abiTracking=true, which requires incrementalStrategy=abi.

GraphIncrementalBuild.finish() → if (abiTracking) { AbiManifest.write(...) } — with graph strategy, abiTracking=false, so no manifest is written.

Either change the strategy to abi to match the IT's name and assertions:

Suggested change

	<properties>
	<maven.compiler.incrementalStrategy>abi</maven.compiler.incrementalStrategy>

Or remove the manifest assertions from verify.groovy.

[gnodet-bot]

🔴 Same issue as abi-incremental-basic — uses incrementalStrategy=graph but verify.groovy asserts graph cascade: recompiling. The cascade assertion is valid for graph, but the IT name abi-incremental-cascade is misleading.

The cascade verify.groovy doesn't assert manifest existence (so it won't fail for the same reason as the basic IT), but the name should match the strategy. Consider renaming to graph-incremental-cascade or switching to incrementalStrategy=abi.

[gnodet-bot]

💡 Note (low): MODULE_PATH entries not collected for external ABI tracking. The compileWithGraphIncremental method collects entries from CLASS_PATH and ANNOTATION_PROCESSOR_PATH/ANNOTATION_PROCESSOR_MODULE_PATH, but does not collect entries from MODULE_PATH. For JPMS reactor projects where dependencies are on the module path rather than the classpath, external ABI changes in those dependencies won't be detected.

This may be acceptable for an initial implementation (non-modular projects are the common case), but worth tracking as a follow-up — a modular reactor project where module A depends on module B via requires would not detect ABI changes in B.

[gnodet-bot]

💡 Note (low): getFullBuildClassIndex() walks the entire output tree. On a full build, this walks all .class files in the output directory and runs BytecodeAnalyzer.analyze() on each one to read the SourceFile attribute. This happens once per source file via collectClassAnalyses() when packageDir is null (no previous state).

The index is cached (outputClassIndex), so the walk happens at most once per round. But for large projects with thousands of class files, this duplicates the analysis work since every class file is also individually analyzed in the final pass of collectClassAnalyses(). The cached analysis (classFileCache.putIfAbsent(entry.getKey(), null)) doesn't reuse the analysis from the index walk — it stores null and re-analyzes.

Not a correctness issue, but a performance opportunity for large codebases.