Repository navigation
[DO NOT MERGE] AIRES-8088 Fix SemSitter frontend gaps found on the ECJ replacement PoC - #6443
antonioaversa wants to merge 13 commits into
Conversation
…neric supertypes and single-source attribution in the SemSitter PoC
…orary directory for single-source parses
…vered binding does
…s compiler warnings
…m their initializer and declared type
…s of a class of the file
…on, field, return and creation types
…ssigned as effectively final
…ookup into smaller methods
CI failed: Two separate CI failures: a ruling-diff comment job failed on malformed S6212 expected-ruling JSON (unrelated to this PR), and java-frontend test compilation failed on unresolved ECJ/JDT symbols with the connection to this PR unclear; no failure is confirmed as caused by the change.OverviewTwo distinct failure patterns were found across 2 analyzed logs (9 templates reported, 2 unique failure groups after synthesis). Neither is confirmed as caused by this PR's changes, and the ruling QA matrix failure that triggered the diff job still lacks its own diagnosis. FailuresRuling diff comment generation failed on malformed S6212 expected JSON (confidence: high)
java-frontend test compilation fails on unresolved ECJ/JDT symbols (confidence: high)
Summary
Code Review
|
| Auto-apply | Compact | Unblock |
|
|
|
Was this helpful? React with 👍 / 👎 | Gitar
antonioaversa
changed the title
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| @Nullable | ||
| private Object declaredConstant() { | ||
| VariableTree declaration = declaration(); | ||
| if (declaration == null || !(declaration.initializer() instanceof LiteralTree literal) | ||
| || !(isFinal() || owner().isInterface() || isEnum())) { | ||
| if (declaration == null || declaration.initializer() == null || declaration.type() == null || !isStaticFinalOrInterfaceMember()) { | ||
| return null; | ||
| } | ||
| return LiteralUtils.constantValue(literal); | ||
| ExpressionTree initializer = declaration.initializer(); | ||
| Object value = initializer.is(Tree.Kind.LONG_LITERAL, Tree.Kind.FLOAT_LITERAL, Tree.Kind.DOUBLE_LITERAL) | ||
| ? literalNumber((LiteralTree) initializer) | ||
| : ExpressionUtils.resolveAsConstant(initializer); | ||
| return convert(value, declaration.type().symbolType()); | ||
| } | ||
|
|
||
| private boolean isStaticFinalOrInterfaceMember() { | ||
| boolean constantKind = isFinal() || owner().isInterface() || isEnum(); |
There was a problem hiding this comment.
💡 Bug: Final instance fields lose their constant value; non-constant types gain one
isStaticFinalOrInterfaceMember now requires isStatic(). Before this PR, a final int X = 5; instance field with a literal initializer reported 5 as its constant. It now reports nothing, although JLS 4.12.4 constant variables (and ECJ's getConstantValue) do not need static. The declared type is also not checked. static final Object O = "s" or static final Integer I = 1 now get a constant, even though only primitive and String final variables can be constant variables. Fix: drop the static requirement, and only evaluate when the declared type is primitive or java.lang.String.
Was this helpful? React with 👍 / 👎
| static void addTo(JavaTree.CompilationUnitTreeImpl unit) { | ||
| CompilerWarnings visitor = new CompilerWarnings(unit.sema); | ||
| visitor.scan(unit.packageDeclaration()); | ||
| visitor.scan(unit.types()); | ||
| unit.imports().forEach(visitor::scan); | ||
| visitor.reportUnusedImports(unit); | ||
| unit.addWarnings(visitor.warnings); | ||
| } |
There was a problem hiding this comment.
⚠️ Bug: Imports used only in module-info directives are reported as unused
CompilerWarnings.addTo collects identifier uses from packageDeclaration(), types() and imports(), but never scans unit.moduleDeclaration(). In a module-info.java such as import com.x.Service; module m { uses Service; }, the identifier Service is never collected. isUsed then returns false and an UNUSED_IMPORT warning is emitted. UselessImportCheck turns that warning into a false positive. Fix: also call visitor.scan(unit.moduleDeclaration()).
Fix:
visitor.scan(unit.packageDeclaration());
visitor.scan(unit.moduleDeclaration());
visitor.scan(unit.types());
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| private boolean isResolvable(ImportTree importTree, String name, String currentPackage) { | ||
| if (importTree instanceof JavaTree.ImportTreeImpl impl && impl.isModule()) { | ||
| return true; | ||
| } | ||
| boolean onDemand = name.endsWith(".*"); | ||
| String qualifier = onDemand ? name.substring(0, name.length() - 2) : name; | ||
| if (onDemand && !importTree.isStatic() && qualifier.equals(currentPackage)) { | ||
| return true; | ||
| } | ||
| return resolvesType(importTree.isStatic() && !onDemand ? qualifier.substring(0, Math.max(0, qualifier.lastIndexOf('.'))) : qualifier); | ||
| } | ||
|
|
||
| private boolean resolvesType(String dotted) { | ||
| return binaryName(dotted) != null; | ||
| } |
There was a problem hiding this comment.
💡 Edge Case: Unused package on-demand imports are never reported
isResolvable only treats a name as resolvable if binaryName finds a declared type. For import java.util.*; it tries java.util and then java$util, gets null from both, and returns false. As a result, an unused on-demand import of a package is never reported, unless it is the current package. ECJ does report these, and UselessImportCheck.isImportFromSamePackage even has handling for ECJ's star-import message. Fix: also treat an on-demand qualifier as resolvable when some known type lives in that package, for example by asking the sema or the bridge whether the package exists.
Was this helpful? React with 👍 / 👎
| Path directory = null; | ||
| try { | ||
| SemsitterBridge bridge = unitBridge(); | ||
| directory = privateTempDirectory(); | ||
| Path file = directory.resolve(Path.of(unitName.endsWith(".java") ? unitName : (unitName + ".java")).getFileName().toString()); | ||
| Files.writeString(file, source, StandardCharsets.UTF_8); | ||
| JsonObject document; | ||
| synchronized (bridge) { | ||
| bridge.analyze(directory, List.of(file), JParserConfig.bridgeClasspath(classpath)); | ||
| document = bridge.cst(file.toString()); | ||
| } | ||
| JSema sema = new JSema(); | ||
| sema.ingestDocument(document); | ||
| // Types load lazily while rules run, so the bridge outlives this call. | ||
| sema.loadTypes(names -> { |
There was a problem hiding this comment.
💡 Bug: Shared unit bridge answers lazy lookups for earlier trees from the latest analyze
The trees returned by JParser.parse load types lazily through bridge.types, and the bridge is shared by the whole JVM. The bridge server answers types from self.analysis, which every new analyze replaces (server.rs:83,90-93). Once a second unit is parsed, lazy lookups from the first tree run against the second unit's project types and classpath, which may be a different classpath argument or empty. On top of that, any failure in a later parse calls discardUnitBridge() and closes the bridge, so every lookup from trees already returned fails with UncheckedIOException. This goes against the comment that the bridge outlives the call. Each parse also registers another shutdown hook that keeps the bridge it started. Fix: give each parse its own analysis handle or bridge, or re-run that unit's analyze before answering its lookups.
Was this helpful? React with 👍 / 👎
| b'<' => { | ||
| *i += 1; | ||
| args.clear(); | ||
| while *i < bytes.len() && bytes[*i] != b'>' { | ||
| args.push(signature_type(signature, i)); | ||
| } | ||
| *i += 1; | ||
| } | ||
| b'.' => { | ||
| name.push(b'$'); | ||
| *i += 1; | ||
| } |
There was a problem hiding this comment.
💡 Bug: Signature parser keeps the outer class's type arguments on a non-generic inner type
In signature_type, the b'.' branch adds $ to the name but does not clear args. A supertype written Lp/Outer<TT;>.Inner; therefore comes out as p.Outer$Inner with args [T], using the outer class's arguments. supertypeFromFact then builds a parameterized Outer$Inner<T> for a type that has no type parameters. The unit test only covers an inner type that is itself generic, where < clears the list. Fix: call args.clear() in the . branch as well.
Fix:
b'.' => {
args.clear();
name.push(b'$');
*i += 1;
}
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| /** | ||
| * Marks the local variables and parameters that are never reassigned (JLS 4.12.4), as the compiler does. | ||
| * A variable declared without a value stays effectively final after one plain assignment. | ||
| */ | ||
| final class EffectivelyFinalVariables extends BaseTreeVisitor { | ||
|
|
||
| private final Map<Symbol, int[]> assignments = new IdentityHashMap<>(); | ||
| private final List<VariableTree> variables = new ArrayList<>(); | ||
|
|
||
| private EffectivelyFinalVariables() { | ||
| } | ||
|
|
||
| static void mark(JavaTree.CompilationUnitTreeImpl unit) { | ||
| EffectivelyFinalVariables visitor = new EffectivelyFinalVariables(); | ||
| visitor.scan(unit); |
There was a problem hiding this comment.
💡 Edge Case: Effectively-final check counts assignments in the text, not definite assignment
mark treats a local declared without a value as effectively final when the text contains exactly one plain assignment to it. JLS 4.12.4 uses definite assignment instead. int x; if (c) x = 1; else x = 2; is effectively final but gets marked not final, because the count is 2. int x; while (c) { x = 1; } is not effectively final but gets marked final, because the count is 1. The class doc says this matches the compiler. Checks that read isEffectivelyFinal() will disagree with ECJ in both directions.
Was this helpful? React with 👍 / 👎
|
/pvf |
|
⚖️ Ruling update ready for review: #6444 |
|
|
⚖️ Ruling update ready for review: #6444 |
Ruling Diff SummaryDetected changes in 123 rule files: 2803 issues removed, 2153 issues added. S112 (
|
|
📊 Performance Validation complete |
Stacked on #6415 (base
rombirli/semsitter-ecj-replacement-v2). Do not merge: opened to run the PVF against the SemSitter frontend with these fixes.Does not modify #6415 or its branch; 13 commits on top of
4c48ffe5cb.d2455142ce).a.b, nota.a.b).UNUSED_IMPORTandASSIGNMENT_HAS_NO_EFFECTwarnings, constant evaluation, implicit default constructor, effectively-final flag.Unit tests in
java-checks: 2,199 of 2,488 pass (was 2,128). Semdiff differing facts: 29,624 (was 33,816). Open gaps and evidence:60-sonarjava-side-fixes.mdin the AIRES-8030 attachments.Epic: AIRES-8030.