gnodet-bot commented on code in PR #1151: URL: https://github.com/apache/maven-compiler-plugin/pull/1151#discussion_r4179309544
########## src/main/java24/org/apache/maven/plugin/compiler/incremental/ClassfileClassAnalyzer.java: ########## @@ -0,0 +1,636 @@ +/* + * 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.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.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 record accessors.** The `TreeSet` instances for `signatureTypes`, `implementationTypes`, and `annotationTypes` are passed directly into the `ClassAnalysis` record. Since records expose component accessors directly, any caller can mutate these sets after construction. This is a latent footgun — no callers exist yet, but the upcoming ABI incremental strategy will consume these. Wrapping them in `Collections.unmodifiableSet()` (or `Set.copyOf()` which also deduplicates the `TreeSet` overhead) makes the contract explicit: ```suggestion return new BytecodeAnalyzer.ClassAnalysis( className, abiFingerprint, abiCanonical, Set.copyOf(signatureTypes), Set.copyOf(implementationTypes), Set.copyOf(annotationTypes), /* moduleName= */ "", /* isModuleInfo= */ false, sourceFileName); ``` Same pattern applies to the `analyzeModuleInfo` path (line 220: `sigTypes`). ########## 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: 💡 **Minor efficiency:** `String.format("%02x", b)` per byte is surprisingly expensive (locale lookup, format string parsing on each call). Since this targets JDK 17+, `HexFormat` is available and cleaner: ```suggestion var md = MessageDigest.getInstance("SHA-256"); byte[] digest = md.digest(content); return HexFormat.of().formatHex(digest).substring(0, 16); ``` Requires `import java.util.HexFormat;`. Single allocation vs 32 `String.format` calls. ########## src/test/java/org/apache/maven/plugin/compiler/incremental/CompilerTestHelper.java: ########## @@ -0,0 +1,69 @@ +/* + * 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 javax.tools.JavaCompiler; +import javax.tools.StandardLocation; +import javax.tools.ToolProvider; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.stream.Stream; + +/** + * Shared helper for incremental compilation tests. + */ +class CompilerTestHelper { + + static void writeSource(Path sourceDir, String packageName, String className, String source) throws IOException { + Path packageDir = sourceDir.resolve(packageName.replace('.', '/')); + Files.createDirectories(packageDir); + Files.writeString(packageDir.resolve(className + ".java"), source); + } + + /** + * Compiles all {@code .java} files under {@code sourceDir} into {@code outputDir}. + */ + static void compileAndAnalyze(Path sourceDir, Path outputDir) throws IOException { + Files.createDirectories(outputDir); + List<Path> sourceFiles; + try (Stream<Path> walk = Files.walk(sourceDir)) { + sourceFiles = + walk.filter(p -> p.toString().endsWith(".java")).sorted().toList(); + } + + JavaCompiler compiler = ToolProvider.getSystemJavaCompiler(); + try (var fm = compiler.getStandardFileManager(null, null, null)) { + fm.setLocation(StandardLocation.CLASS_OUTPUT, List.of(outputDir.toFile())); + var units = fm.getJavaFileObjectsFromPaths(sourceFiles); + var task = compiler.getTask(null, fm, null, null, null, units); + + if (!task.call()) { + throw new RuntimeException("Compilation failed"); + } + } + } + + static void deleteSource(Path sourceDir, String packageName, String className) throws IOException { + Path file = sourceDir.resolve(packageName.replace('.', '/') + "/" + className + ".java"); + Files.deleteIfExists(file); + } +} Review Comment: 💡 **Dead code.** `deleteSource` is defined but never called from any test. If it's intended for the upcoming PRs (part 2/3), consider deferring it to that PR to keep this one minimal. -- 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]
