gnodet-bot commented on code in PR #1153:
URL:
https://github.com/apache/maven-compiler-plugin/pull/1153#discussion_r4181110441
##########
pom.xml:
##########
@@ -85,7 +85,7 @@ under the License.
<mavenVersion>4.0.0-rc-7</mavenVersion>
<asmVersion>9.10.1</asmVersion>
- <mockitoVersion>5.24.0</mockitoVersion>
+ <mockitoVersion>5.23.0</mockitoVersion>
Review Comment:
⚠️ **Unintended dependency downgrade.** This changes mockitoVersion from
5.24.0 (current master) to 5.23.0. The PR doesn't touch test code that uses
Mockito, so this looks like a merge artifact from the branch base. Drop this
hunk to avoid reverting the Dependabot bump.
##########
src/main/java24/org/apache/maven/plugin/compiler/incremental/ClassfileClassAnalyzer.java:
##########
@@ -0,0 +1,648 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.plugin.compiler.incremental;
+
+import java.lang.classfile.Annotation;
+import java.lang.classfile.Attributes;
+import java.lang.classfile.ClassFile;
+import java.lang.classfile.ClassModel;
+import java.lang.classfile.FieldModel;
+import java.lang.classfile.MethodModel;
+import java.lang.classfile.attribute.ModuleAttribute;
+import java.lang.classfile.attribute.ModuleProvideInfo;
+import java.lang.classfile.attribute.ModuleRequireInfo;
+import java.lang.classfile.constantpool.ClassEntry;
+import java.lang.classfile.instruction.FieldInstruction;
+import java.lang.classfile.instruction.InvokeInstruction;
+import java.lang.classfile.instruction.NewMultiArrayInstruction;
+import java.lang.classfile.instruction.NewObjectInstruction;
+import java.lang.classfile.instruction.TypeCheckInstruction;
+import java.lang.reflect.AccessFlag;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Set;
+import java.util.TreeSet;
+
+/**
+ * {@link ClassAnalyzer} implementation using the standard {@code
java.lang.classfile}
+ * API, available since JDK 24.
+ *
+ * <p>This implementation lives in {@code META-INF/versions/24/} as part of the
+ * multi-release JAR. It is instantiated directly by the JDK 24+ override of
+ * {@link BytecodeAnalyzer} — no reflection required. On JDK < 24, the root
+ * {@code BytecodeAnalyzer} stub is loaded instead and ABI fingerprinting is
+ * unavailable.
+ *
+ * <p>Type references are classified into two sets:
+ * <ul>
+ * <li><b>signatureTypes</b> — types from the public API surface: supertype,
interfaces,
+ * field/method descriptors of non-private members, exception types,
annotations.
+ * These are what downstream consumers structurally depend on.</li>
+ * <li><b>implementationTypes</b> — types referenced only in method body
bytecode
+ * instructions (INVOKE*, field access, NEW, CHECKCAST, etc.), and
descriptor
+ * types of private members. Changes to these do not cascade to the
class's
+ * signature consumers.</li>
+ * </ul>
+ *
+ * <p>{@code module-info.class} is handled specially: its ABI fingerprint is
derived
+ * from the {@code Module} attribute directives (requires, exports, opens,
uses,
+ * provides), and no implementation types are collected.
+ *
+ * @see BytecodeAnalyzer
+ * @see ClassAnalyzer
+ */
+class ClassfileClassAnalyzer extends ClassAnalyzer {
+
+ @Override
+ public BytecodeAnalyzer.ClassAnalysis analyze(byte[] classBytes) {
+ ClassModel cm = ClassFile.of().parse(classBytes);
+
+ // module-info.class has the MODULE access flag
+ if (cm.flags().has(AccessFlag.MODULE)) {
+ return analyzeModuleInfo(cm);
+ }
+
+ String className =
BytecodeAnalyzer.toJavaName(cm.thisClass().asInternalName());
+ Set<String> signatureTypes = new TreeSet<>();
+ Set<String> implementationTypes = new TreeSet<>();
+ Set<String> annotationTypes = new TreeSet<>();
+
+ collectTypes(cm, signatureTypes, implementationTypes, annotationTypes);
+
+ // Remove self-references and JDK types
+ signatureTypes.remove(className);
+ implementationTypes.remove(className);
+ implementationTypes.removeAll(signatureTypes); // sig takes precedence
+ signatureTypes.removeIf(ClassfileClassAnalyzer::isJdkType);
+ implementationTypes.removeIf(ClassfileClassAnalyzer::isJdkType);
+ annotationTypes.removeIf(ClassfileClassAnalyzer::isJdkType);
+
+ String abiCanonical = buildCanonical(cm);
+ String abiFingerprint = Sha256.hash(abiCanonical);
+
+ // Read the SourceFile attribute for accurate source-file attribution
+ String sourceFileName = cm.findAttribute(Attributes.sourceFile())
+ .map(sf -> sf.sourceFile().stringValue())
+ .orElse("");
+
+ return new BytecodeAnalyzer.ClassAnalysis(
+ className,
+ abiFingerprint,
+ abiCanonical,
+ signatureTypes,
+ implementationTypes,
+ annotationTypes,
+ /* moduleName= */ "",
+ /* isModuleInfo= */ false,
+ sourceFileName);
Review Comment:
💡 **Mutable sets leaked through public record.** `ClassAnalysis` is a public
record and its `signatureTypes`/`implementationTypes`/`annotationTypes` fields
are the raw `TreeSet` instances built above. Any caller can
`.add()`/`.remove()` on them, breaking the record's implied immutability
contract. The follow-up PRs (B and C) will consume these sets — a stray
mutation would produce silent, hard-to-debug fingerprint drift.
Wrap them with `Collections.unmodifiableSet()` or `Set.copyOf()` before
passing to the constructor:
```suggestion
return new BytecodeAnalyzer.ClassAnalysis(
className,
abiFingerprint,
abiCanonical,
Set.copyOf(signatureTypes),
Set.copyOf(implementationTypes),
Set.copyOf(annotationTypes),
/* moduleName= */ "",
/* isModuleInfo= */ false,
sourceFileName);
```
Note: `Set.copyOf()` also discards the `TreeSet` ordering (returns an
unmodifiable `HashSet`-based set). If sorted iteration matters downstream, use
`Collections.unmodifiableSortedSet()` instead.
##########
src/main/java/org/apache/maven/plugin/compiler/incremental/Sha256.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.plugin.compiler.incremental;
+
+import java.nio.charset.StandardCharsets;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
+
+/**
+ * SHA-256 hashing utility shared by the bytecode analyzer implementations.
+ */
+public final class Sha256 {
+
+ private Sha256() {}
+
+ /**
+ * Returns the first 16 hex characters of the SHA-256 hash of {@code
input}.
+ *
+ * @param input the string to hash
+ * @return 16-character hex string
+ */
+ public static String hash(String input) {
+ return hash(input.getBytes(StandardCharsets.UTF_8));
+ }
+
+ /**
+ * Returns the first 16 hex characters of the SHA-256 hash of {@code
content}.
+ *
+ * @param content the bytes to hash
+ * @return 16-character hex string
+ */
+ public static String hash(byte[] content) {
+ try {
+ var md = MessageDigest.getInstance("SHA-256");
+ byte[] digest = md.digest(content);
+ var hex = new StringBuilder(32);
+ for (byte b : digest) {
+ hex.append(String.format("%02x", b));
+ }
+ return hex.substring(0, 16);
Review Comment:
💡 **`String.format` per byte is ~10x slower than `HexFormat`.** This runs
for every `.class` file analyzed during an incremental build — it's on the hot
path. `HexFormat` has been available since JDK 17 (the project's minimum
target).
```suggestion
var hex = HexFormat.of().formatHex(digest);
return hex.substring(0, 16);
```
Requires adding `import java.util.HexFormat;` at the top.
##########
src/main/java/org/apache/maven/plugin/compiler/incremental/BytecodeAnalyzer.java:
##########
@@ -0,0 +1,179 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.plugin.compiler.incremental;
+
+import java.io.IOException;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.Set;
+
+/**
+ * Facade for analyzing compiled {@code .class} files to extract type
references
+ * and compute bytecode-level ABI fingerprints.
+ *
+ * <p>This class is part of a multi-release JAR. The root implementation
(loaded on
+ * JDK < 24) always returns {@code false} from {@link #isAvailable()} — ABI
+ * fingerprinting is not supported on JDK 17–23. The {@code
META-INF/versions/24/}
+ * override (loaded automatically by the JVM on JDK 24+) provides the real
+ * implementation backed by the standard {@code java.lang.classfile} API.
+ *
+ * <p>Callers must check {@link #isAvailable()} before calling {@link
#analyze}.
+ * When unavailable, the ABI incremental strategy falls back to the timestamp
strategy.
+ *
+ * <p>The {@link ClassAnalysis} record carries the class name, ABI fingerprint,
+ * human-readable canonical form, and two classified sets of type references:
+ * {@link ClassAnalysis#signatureTypes()} (API surface) and
+ * {@link ClassAnalysis#implementationTypes()} (method body instructions only).
+ */
+public final class BytecodeAnalyzer {
+
+ /**
+ * Prefix used to distinguish module-info qualified names from regular
class names.
+ * A type name starting with this prefix is a module name, not a class
name.
+ */
+ public static final String MODULE_PREFIX = "module:";
+
+ /**
+ * Analysis result for a single {@code .class} file.
+ *
+ * @param className fully-qualified class name (dot-separated)
+ * @param abiFingerprint 16-character hex SHA-256 prefix of the ABI
canonical form
+ * @param abiCanonical human-readable representation of the public
API surface
+ * @param signatureTypes types appearing in public API surface
(method/field descriptors,
+ * supertype, interfaces, exception types,
annotation types)
+ * @param implementationTypes types appearing only in method body bytecode
instructions
+ * (INVOKEVIRTUAL, NEW, CHECKCAST, field
owners, etc.)
+ * @param annotationTypes fully-qualified names of annotation types
present on the class
+ * or its members, used for annotation
processor cascade decisions
+ * @param moduleName Java module name, or empty string if unnamed
or non-modular
+ * @param isModuleInfo {@code true} if this represents {@code
module-info.class}
+ * @param sourceFileName simple source file name from the {@code
SourceFile} class file
+ * attribute (e.g. {@code "Foo.java"}); empty
string if absent
+ */
+ public record ClassAnalysis(
+ String className,
+ String abiFingerprint,
+ String abiCanonical,
+ Set<String> signatureTypes,
+ Set<String> implementationTypes,
+ Set<String> annotationTypes,
+ String moduleName,
+ boolean isModuleInfo,
+ String sourceFileName) {}
+
+ private BytecodeAnalyzer() {}
+
+ /**
+ * Returns {@code true} if ABI fingerprinting is available on the running
JVM.
+ *
+ * <p>This method returns {@code false} in the root JAR (JDK < 24). The
+ * {@code META-INF/versions/24/} override returns {@code true}.
+ *
+ * <p>When this returns {@code false}, callers should fall back to the
timestamp
+ * incremental strategy and log a warning to the user.
+ *
+ * @return {@code true} if {@link #analyze} can be called safely
+ */
+ public static boolean isAvailable() {
+ return false;
+ }
+
+ /**
+ * Analyzes the class file at {@code classFile}.
+ *
+ * <p>Only call this method after confirming {@link #isAvailable()}
returns {@code true}.
+ *
+ * @param classFile path to the {@code .class} file
+ * @return analysis result
+ * @throws IOException if reading the file fails
+ * @throws UnsupportedOperationException if called on JDK < 24
+ */
+ public static ClassAnalysis analyze(Path classFile) throws IOException {
+ throw new UnsupportedOperationException("ABI fingerprinting requires
JDK 24 or later (running JDK "
+ + Runtime.version().feature()
+ + "). Check BytecodeAnalyzer.isAvailable() before calling
analyze().");
+ }
+
+ /**
+ * Analyzes the given class file bytes.
+ *
+ * <p>Only call this method after confirming {@link #isAvailable()}
returns {@code true}.
+ *
+ * @param classBytes raw {@code .class} file content
+ * @return analysis result
+ * @throws UnsupportedOperationException if called on JDK < 24
+ */
+ public static ClassAnalysis analyze(byte[] classBytes) {
+ throw new UnsupportedOperationException("ABI fingerprinting requires
JDK 24 or later (running JDK "
+ + Runtime.version().feature()
+ + "). Check BytecodeAnalyzer.isAvailable() before calling
analyze().");
+ }
+
+ // --- package-private utilities shared by the analyzer implementations
and tests ---
+
+ static String descriptorToReadable(String desc) {
+ if (desc.isEmpty()) {
+ return desc;
+ }
+ return switch (desc.charAt(0)) {
+ case 'V' -> "void";
+ case 'Z' -> "boolean";
+ case 'B' -> "byte";
+ case 'C' -> "char";
+ case 'S' -> "short";
+ case 'I' -> "int";
+ case 'J' -> "long";
+ case 'F' -> "float";
+ case 'D' -> "double";
+ case 'L' -> toJavaName(desc.substring(1, desc.indexOf(';')));
+ case '[' -> descriptorToReadable(desc.substring(1)) + "[]";
+ default -> desc;
+ };
+ }
+
+ static String parseParams(String methodDesc) {
+ int close = methodDesc.indexOf(')');
+ String params = methodDesc.substring(1, close);
+ var result = new ArrayList<String>();
+ int i = 0;
+ while (i < params.length()) {
+ int start = i;
Review Comment:
🔵 **Nit — no guard against malformed `L`-type descriptors in
`descriptorToReadable`.** If `desc` is `"Lfoo"` (missing `;`),
`desc.indexOf(';')` returns `-1` and `desc.substring(1, -1)` throws
`StringIndexOutOfBoundsException`. Same issue in `parseParams` where
`params.indexOf(';', i)` returning `-1` would set `i = 0`, causing an infinite
loop.
These are package-private utilities processing JVM-spec-compliant bytecode,
so the real-world risk is minimal — the JVM won't load a class with a malformed
descriptor. But a defensive guard (or at least a comment documenting the
precondition) would help future maintainers.
--
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]
