Repository navigation
Conversation
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| @Override | ||
| protected void scan(@Nullable List<? extends Tree> trees) { | ||
| if (trees == null) { | ||
| return; | ||
| } | ||
| Map<Tree.Kind, Integer> seen = new HashMap<>(); | ||
| for (Tree tree : trees) { | ||
| if (tree == null) { | ||
| continue; | ||
| } | ||
| int index = seen.merge(tree.kind(), 1, Integer::sum) - 1; | ||
| enter(tree, index); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
⚠️ Bug: Sibling nodes reached via scan(Tree) all get index [0], so paths collide
DumpVisitor.scan(Tree) always calls enter(tree, 0). BaseTreeVisitor reaches most children through separate scan(Tree) calls: visitMemberSelectExpression scans expression() and then identifier(), and visitVariable scans type() and then simpleName(). When two of those children have the same kind, they get the same path, e.g. .../MEMBER_SELECT[0]/IDENTIFIER[0] for both values and get in the sample corpus. ListTree children like class superInterfaces/permittedTypes hit the same problem, since they go through scan((Tree) listTree), and so do separate scan(List) calls under one parent. In compare.py, load() keys records by path (nodes[name][record["path"]] = record), so the later record overwrites the earlier one. Node counts come out too low (the reported 63 shared nodes included), the descendants of colliding subtrees get merged, and mismatches on the overwritten node are never seen. Fix: keep a per-parent Map<Tree.Kind,Integer> counter on the stack and assign the index in enter, no matter which scan overload reached the child.
Index children per parent instead of per scan call:
private final java.util.ArrayDeque<Map<Tree.Kind, Integer>> counters = new java.util.ArrayDeque<>(List.of(new HashMap<>()));
@Override
protected void scan(@Nullable List<? extends Tree> trees) {
if (trees != null) { for (Tree t : trees) { scan(t); } }
}
@Override
protected void scan(@Nullable Tree tree) {
if (tree != null) {
enter(tree, counters.peek().merge(tree.kind(), 1, Integer::sum) - 1);
}
}
// in enter(): counters.push(new HashMap<>()) before accept, counters.pop() after
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| @Override | ||
| protected void scan(@Nullable Tree tree) { | ||
| if (tree != null) { | ||
| enter(tree, 0); | ||
| } | ||
| } | ||
|
|
||
| private void enter(Tree tree, int index) { | ||
| String parent = stack.isEmpty() ? "" : stack.peek(); | ||
| String path = (parent.isEmpty() ? "" : parent + "/") + tree.kind().name() + "[" + index + "]"; | ||
| stack.push(path); | ||
| emit(tree, path); | ||
| tree.accept(this); | ||
| stack.pop(); | ||
| } |
There was a problem hiding this comment.
⚠️ Bug: Binary-expression operands are never emitted or compared
BaseTreeVisitor.visitBinaryExpression calls tree.leftOperand().accept(this) and tree.rightOperand().accept(this) directly, not scan(...). Because of that, DumpVisitor.enter never runs for the operands: they get no record, so their symbols and types are never compared. Their descendants are also given paths directly under the binary node, as if the operands weren't there. In a + b, for example, neither identifier is dumped. Any AST or semantic difference on operands, which show up all over a real corpus, is invisible to the benchmark. Fix: override visitBinaryExpression in DumpVisitor to call scan(tree.leftOperand()); scan(tree.rightOperand());. Also check BaseTreeVisitor for any other direct accept(this) calls.
Route operands through scan so they are emitted:
@Override
public void visitBinaryExpression(org.sonar.plugins.java.api.tree.BinaryExpressionTree tree) {
scan(tree.leftOperand());
scan(tree.rightOperand());
}
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| ecj = args.ecj | ||
| if ecj is None: | ||
| candidate = REPO.parent / "sonar-java" | ||
| if not candidate.exists(): | ||
| raise SystemExit("pass --ecj, the ECJ checkout was not found at ../sonar-java") | ||
| ecj = candidate | ||
| corpus = args.corpus.resolve() | ||
| out = args.out.resolve() | ||
| out.mkdir(parents=True, exist_ok=True) | ||
| files = java_files(corpus) | ||
| print(f"classpath {ecj}", file=sys.stderr) | ||
| ecj_cp = tool_classpath(ecj) | ||
| print(f"classpath {args.semsitter}", file=sys.stderr) | ||
| sem_cp = tool_classpath(args.semsitter.resolve()) | ||
| ecj_dump = out / "ecj.jsonl" |
There was a problem hiding this comment.
💡 Bug: Relative --ecj path breaks the mvn output file location
args.semsitter is resolved before tool_classpath, but ecj is passed through unresolved. When --ecj ../sonar-java is given, output stays relative to Python's cwd. Maven, however, runs with cwd=checkout and resolves -Dmdep.outputFile relative to that directory (or to the module basedir), not Python's cwd. The classpath file ends up somewhere else, and output.read_text() then fails or reads a stale file. Fix: resolve ecj once (ecj = ecj.resolve()) before tool_classpath.
Resolve the ECJ checkout path up front:
ecj = ecj.resolve()
corpus = args.corpus.resolve()
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
|
⚖️ Ruling update ready for review: #6438 |
Ruling Diff SummaryDetected changes in 123 rule files: 4940 issues removed, 5151 issues added. S104 (
|
cc21c23 to
33bf97f
Compare
| def issue_name(item: dict) -> str: | ||
| category = item["category"] | ||
| if category == "parse": | ||
| if item.get("ecj_error") and item.get("sem_error"): | ||
| return "parse failure on both sides" | ||
| if item.get("sem_error"): | ||
| return "parse failure on SemSitter" | ||
| return "parse failure on ECJ" | ||
| if category == "symbol": | ||
| return symbol_issue(item["ecj"], item["sem"]) | ||
| if category == "type": | ||
| return type_issue(item["ecj"], item["sem"]) | ||
| if category == "span": | ||
| return span_issue(item["ecj"], item["sem"]) | ||
| node = item.get("ecj") or item.get("sem") or {} |
There was a problem hiding this comment.
⚠️ Bug: type_arguments mismatches get bucketed as 'node only on SemSitter'
bucket_sample.py samples the semantic categories {"symbol", "type", "type_arguments"} (line 345), and collect adds type_arguments items from semantic_kind. But issue_name only handles parse, symbol, type and span. Every other category falls through to ast_issue(category, node), and that returns f"node only on SemSitter: {kind}" for anything that isn't ast_only_ecj. So each type-argument mismatch shows up in semantic-issue-buckets.csv as an AST gap such as node only on SemSitter: IDENTIFIER, and fix_for then labels it semsitter. The semantic buckets give the wrong root cause and fix target for this whole category. Fix: add a type_arguments branch to issue_name (for example, one that compares symbolFull/typeFull), and send only the ast_only_* categories to ast_issue.
Handle type_arguments explicitly in issue_name:
if category == "type_arguments":
return "type arguments differ"
if category in {"ast_only_ecj", "ast_only_semsitter"}:
node = item.get("ecj") or item.get("sem") or {}
return ast_issue(category, node)
return category
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| Map<String, Tree> trees = new HashMap<>(); | ||
| Map<String, String> errors = new HashMap<>(); | ||
| config.parse(inputFiles, () -> false, new AnalysisProgress(inputFiles.size()), (input, result) -> { | ||
| String name = names.get(input); | ||
| try { | ||
| trees.put(name, result.get()); | ||
| } catch (Exception failure) { | ||
| errors.put(name, describe(failure)); | ||
| } | ||
| }); | ||
| StringBuilder body = new StringBuilder(); | ||
| List<String> order = new ArrayList<>(names.values()); | ||
| order.sort(Comparator.naturalOrder()); | ||
| for (String name : order) { | ||
| if (errors.containsKey(name)) { |
There was a problem hiding this comment.
⚠️ Performance: Every parsed tree is held in memory until all files are parsed
In this delta, AstSemanticDump parses through JParserConfig.parse and stores every result.get() in the trees map (lines 95-104). Nothing is dumped until the whole corpus has been parsed. Each retained CompilationUnitTree keeps its full semantic model alive (with ECJ in FILE_BY_FILE mode, that includes per-file binding environments). Memory therefore grows with corpus size instead of staying at one file at a time, as it does in the rule tests and the scanner. The PR's next step is running against a larger corpus, and that run may hit OOM or slow down badly. Fix: run DumpVisitor (or write the error record) inside the callback, appending to a per-file buffer, so each tree can be released straight away. The output order is already fixed by sorting names.
Was this helpful? React with 👍 / 👎
| for name in files: | ||
| left = ecj_nodes.get(name, {}) | ||
| right = sem_nodes.get(name, {}) | ||
| ecj_count += len(left) | ||
| sem_count += len(right) | ||
| left_only = set(left) - set(right) | ||
| right_only = set(right) - set(left) | ||
| for path in sorted(left_only): | ||
| only_ecj_all.append((name, path)) | ||
| for path in sorted(right_only): | ||
| only_sem_all.append((name, path)) | ||
| for path in collapsed(left_only): | ||
| only_ecj_roots.append((name, path)) | ||
| for path in collapsed(right_only): | ||
| only_sem_roots.append((name, path)) |
There was a problem hiding this comment.
💡 Quality: Global AST counts include whole-file parse failures; per-file diffs don't
load stores a file either as an error or as nodes, never both. So when one side fails to parse a file, compare() puts every node from the other side into only_ecj_all/only_sem_all and counts them in ast.mismatches, only_ecj and mismatch_proportion. mismatches.txt also lists COMPILATION_UNIT[0] as "AST only in ECJ/SemSitter". For the same file, file_diff sets whole_file and reports zero AST mismatches ("one side did not parse this file"), and bucket_sample.collect skips the file's AST rows. A single parse failure can therefore inflate the headline AST mismatch rate, while the per-file diffs and the buckets disagree with that number. Fix: skip whole-file parse failures in compare() the same way file_diff and collect do, and keep reporting them under parse_failures.
Was this helpful? React with 👍 / 👎
|
⚖️ Ruling update ready for review: #6438 |
This comment has been minimized.
This comment has been minimized.
33bf97f to
050e0d6
Compare
|
⚖️ Ruling update ready for review: #6438 |
050e0d6 to
13fe6f2
Compare
| private static String withArguments(Type type, int depth) { | ||
| if (type.isUnknown()) { | ||
| return "unknown"; | ||
| } | ||
| String name = erase(type.fullyQualifiedName()); | ||
| if (depth > 3 || !type.isParameterized()) { | ||
| return name; | ||
| } | ||
| StringBuilder out = new StringBuilder(name).append('<'); | ||
| List<Type> arguments = type.typeArguments(); | ||
| for (int i = 0; i < arguments.size(); i++) { | ||
| out.append(i == 0 ? "" : ",").append(withArguments(arguments.get(i), depth + 1)); | ||
| } | ||
| return out.append('>').toString(); | ||
| } |
There was a problem hiding this comment.
💡 Edge Case: Type arguments of array element types are dropped from typeFull
withArguments erases fullyQualifiedName() and only adds arguments when type.isParameterized() is true. Array types are never parameterized (JType.isParameterized only returns true for Kind.NAMED with args), and their name is built from the element's FQN plus []. So List<String>[], and varargs such as List<T>... in method signatures, both come out as java.util.List[] in typeFull/symbolFull. A type-argument mismatch inside an array element type is never counted, which falls short of what this commit says it does ("Compare type arguments"). Fix: when the type is an ArrayType, recurse into elementType() and append [].
Recurse into the element type for arrays before checking parameterization:
if (type.isArray()) {
return withArguments(((Type.ArrayType) type).elementType(), depth) + "[]";
}
String name = erase(type.fullyQualifiedName());
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
|
⚖️ Ruling update ready for review: #6438 |
1 similar comment
|
⚖️ Ruling update ready for review: #6438 |
13fe6f2 to
1df8a88
Compare
|
⚖️ Ruling update ready for review: #6438 |
1df8a88 to
045c9a3
Compare
|
⚖️ Ruling update ready for review: #6438 |
The benchmark runs the ECJ frontend and this frontend on the same sources and writes absolute counts, proportions, and each mismatch.
Input files were keyed by base name, so files with the same name in different directories overwrote each other and 196 samples were never compared.
…ples compare.py also splits the dumps on line feeds only, so a string literal holding a Unicode next-line character no longer breaks a record. bucket_sample.py samples 1000 AST and 1000 semantic mismatches evenly across files, groups them by underlying issue, and says whether the fix belongs to the bridge or to SemSitter.
045c9a3 to
3fe5877
Compare
CI failed: Two unrelated CI test failures in the Plugin QA and Sanity Test jobs: six integration-test assertion failures (JSP, modular-JDK classpath, tutorial telemetry, SonarLint parse-error) and a Surefire forked-JVM OutOfMemoryError in SanityTest. Neither is attributed to this PR's benchmark-only changes.OverviewTwo unique error templates were analyzed from 2 of the 60 logs in the run (commit 3fe5877). Both failures are in the test stage. The analyzers found no evidence linking either to this PR, which only adds standalone benchmark tooling and corpus files under FailuresPlugin QA integration-test assertion failures (confidence: high)
Sanity Test OutOfMemoryError in Surefire fork (confidence: high)
Summary
Code Review
|
| Auto-apply | Compact | Unblock |
|
|
|
Was this helpful? React with 👍 / 👎 | Gitar
|
⚖️ Ruling update ready for review: #6438 |
Summary
benchmark/out/mismatches.txt, plussummary.jsonandmismatches.jsonl).--ecj, default../sonar-java). Both checkouts needjava-frontendalready compiled.Test plan
python3 benchmark/compare.py --ecj ~/Documents/sonar-java --semsitter <v2 checkout> --corpus benchmark/corpus--corpusat a larger tree (check samples) once both frontends are compiled in the checkouts you want to score.Summary by Gitar
bucket_sample.pyto sample and bucket AST and semantic mismatches per fileAstSemanticDump.javato dump SonarJava trees with symbols and types for ECJ and SemSitterSample.javaandUse.javasample sources underbenchmark/corpus/benchast-issue-buckets.csv/semantic-issue-buckets.csvoutputs;out/ignored via.gitignoreThis will update automatically on new commits.