From 980f7bc2f0f073b563b8add97a1f46a452666c2f Mon Sep 17 00:00:00 2001 From: rombirli Date: Tue, 13 Oct 2026 10:23:01 +0200 Subject: [PATCH 1/3] Tighten sample, test and implementation guidance --- .agents/skills/new-rule/SKILL.md | 120 ++++++++++++++++++++++++++++--- 1 file changed, 110 insertions(+), 10 deletions(-) diff --git a/.agents/skills/new-rule/SKILL.md b/.agents/skills/new-rule/SKILL.md index 6ed42e4a115..67c4e4e1614 100644 --- a/.agents/skills/new-rule/SKILL.md +++ b/.agents/skills/new-rule/SKILL.md @@ -46,15 +46,15 @@ Stop and resolve gaps before Step 1. ### Step 2 — Failing reproducer → commit -1. Write the test sample (`java-checks-test-sources/default/src/main/java/checks/{RuleId}CheckSample.java`) covering **every** edge case, noncompliant and compliant, with precise location markers (see the test conventions in `AGENTS.md`). -2. Write `{RuleId}CheckTest.java`, with both a semantic and a `withoutSemantic()` test. +1. Write the test sample (`java-checks-test-sources/default/src/main/java/checks/{RuleId}CheckSample.java`) covering **every** edge case, noncompliant and compliant, with caret markers on **every** primary and secondary location (see [Test sample location markers](#test-sample-location-markers)). +2. Write `{RuleId}CheckTest.java`, with both a semantic and a `withoutSemantic()` test, free of duplication (see [Test class structure](#test-class-structure)). 3. Add an **empty** check class (`@Rule(key = "SXXXX")`, minimal `nodesToVisit`/`visitNode`). 4. Run the test — it **must fail**. 5. Commit (reproducer + empty check + test). ### Step 3 — Implementation → commit -1. Implement the check until the unit test **passes**. +1. Implement the check until the unit test **passes**. Match methods and types with [matchers](#method-and-type-matching), never with raw strings, and keep the check [stateless](#stateless-checks). 2. Commit. ### Step 4 — Push & PR @@ -131,23 +131,123 @@ The PR is done **only** when: - `sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S{RULE_ID}.json` - `sonar-java-plugin/src/main/resources/profiles/{Sonar_way|Sonar_agentic_AI}/S{RULE_ID}` -### MethodMatchers +### Test sample location markers -Use `MethodMatchers` to match method calls by type, name, and signature: +Every **primary** and **secondary** location must be asserted with caret markers. Never leave a bare `// Noncompliant` when the rule reports on a sub-expression: a test that does not pin the location does not protect it. + +Put the carets in a comment on the line **below** the reported line, aligned under the flagged token: + +```java +b = sb1.length() == 0; // Noncompliant {{Use "isEmpty()" to check whether a "StringBuilder" is empty or not.}} +// ^^^^^^^^^^^^^^^^^ +``` + +Secondary locations use the same carets plus a direction marker: `<` when the primary is **above**, `>` when it is **below**. + +```java +new Thread(() -> { +// ^^^^^^> + Thread.sleep(1000); // Noncompliant {{Message.}} +// ^^^^^ +}); +``` + +Several locations can be marked on one caret line: + +```java +Arrays.fill(textBuffer, 42); // Noncompliant {{Message.}} +// ^^^^ ^^^^^^^^^^< +``` + +Use `[[sc=…;ec=…]]` / `[[secondary=…]]` only when carets cannot work: a token at column 1, a multi-line span, or quickfix declarations (`[[quickfixes=qf1]]`). + +To build an aligned caret line: + +```shell +str=">"; sub=""; pre="${str%%"$sub"*}"; echo "${pre//?/ }${sub//?/^}" +``` + +### Test class structure + +Extract the sample path and the verifier setup so neither the path nor the check instantiation is repeated. Declare the sample path as a constant and build the verifier in one private factory: ```java -private static final MethodMatchers MY_MATCHER = MethodMatchers.create() - .ofTypes("java.util.List") - .names("add") - .withAnyParameters() +class MoreThanOneLoggerCheckTest { + + private static final String SAMPLE = "checks/MoreThanOneLoggerCheckSample.java"; + + @Test + void test() { + createCheckVerifier().verifyIssues(); + } + + @Test + void test_without_semantic() { + createCheckVerifier() + .withoutSemantic() + .verifyNoIssues(); + } + + private static CheckVerifier createCheckVerifier() { + return CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath(SAMPLE)) + .withCheck(new MoreThanOneLoggerCheck()); + } +} +``` + +When a test needs different samples (non-compiling, Java version variants), keep one constant per sample and one factory parameterised by path — still no inline duplication. + +### Method and type matching + +Match methods with `MethodMatchers` and types with the type API. **Never** compare fully qualified names, method names or type names with raw strings (`equals`, `contains`, `startsWith`): matchers handle overloads, inheritance and unknown symbols correctly. + +```java +private static final MethodMatchers CONSTRUCTOR_LOOKUP = MethodMatchers.create() + .ofTypes("java.lang.Class") + .names("getConstructor", "getDeclaredConstructor") + .addParametersMatcher("java.lang.Class[]") .build(); // In visitNode: -if (MY_MATCHER.matches(methodInvocationTree)) { +if (CONSTRUCTOR_LOOKUP.matches(methodInvocationTree)) { reportIssue(methodInvocationTree, "Message."); } ``` +Prefer an explicit `addParametersMatcher(...)` over `withAnyParameters()` whenever the rule only targets specific overloads. For types, use `symbolType().isSubtypeOf("java.util.Collection")` rather than string checks on the type name. + +### Stateless checks + +A check instance is reused across files, so it must carry **no mutable state**. A check class may only declare `private static final` fields (messages, matchers, immutable constants). + +Anything accumulated during a scan — sets of visited symbols, maps, counters, lookup tables — must live in a local variable and be passed explicitly through the call graph, never stored in a field. + +```java +@Rule(key = "SXXXX") +public class MyCheck extends IssuableSubscriptionVisitor { + + private static final String MESSAGE = "Message."; + private static final MethodMatchers MATCHER = MethodMatchers.create() /* … */ .build(); + + @Override + public List nodesToVisit() { + return List.of(Tree.Kind.CLASS); + } + + @Override + public void visitNode(Tree tree) { + Set seen = new HashSet<>(); + collectFields((ClassTree) tree, seen); + report(seen); + } +} +``` + +If a rule genuinely needs cross-node context, compute it from the subscribed node by walking the AST, or subscribe to the enclosing node (class, method, compilation unit) and do the whole analysis there with local state. + +The only accepted non-static fields are the ones the API itself requires: the `JavaFileScannerContext` of a `BaseTreeVisitor` rule, assigned at the start of `scanFile`, and the flags a `DependencyVersionAware` rule sets in `isCompatibleWithDependencies` before the scan starts. + ### External Dependencies in Tests - **DO NOT** add external library dependencies for test samples - **DO** create mock-ups or use non-compiling tests when external libraries are needed From 55488d77ecf7fdd220577948439e7b6e918f542b Mon Sep 17 00:00:00 2001 From: rombirli Date: Tue, 13 Oct 2026 10:25:41 +0200 Subject: [PATCH 2/3] Simplify test guidance --- .agents/skills/new-rule/SKILL.md | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/.agents/skills/new-rule/SKILL.md b/.agents/skills/new-rule/SKILL.md index 67c4e4e1614..d232fd4db6d 100644 --- a/.agents/skills/new-rule/SKILL.md +++ b/.agents/skills/new-rule/SKILL.md @@ -169,34 +169,34 @@ str=">"; sub=""; pre="${str%%"$sub"*}"; ec ### Test class structure -Extract the sample path and the verifier setup so neither the path nor the check instantiation is repeated. Declare the sample path as a constant and build the verifier in one private factory: +Do not repeat the sample path or the check instantiation in each test method. Extract them as constants and keep the verifier chain in the tests: ```java -class MoreThanOneLoggerCheckTest { +class FooCheckTest { - private static final String SAMPLE = "checks/MoreThanOneLoggerCheckSample.java"; + private static final String SAMPLE = "checks/FooCheckSample.java"; + private static final FooCheck CHECK = new FooCheck(); @Test void test() { - createCheckVerifier().verifyIssues(); + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath(SAMPLE)) + .withCheck(CHECK) + .verifyIssues(); } @Test void test_without_semantic() { - createCheckVerifier() + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath(SAMPLE)) + .withCheck(CHECK) .withoutSemantic() .verifyNoIssues(); } - - private static CheckVerifier createCheckVerifier() { - return CheckVerifier.newVerifier() - .onFile(mainCodeSourcesPath(SAMPLE)) - .withCheck(new MoreThanOneLoggerCheck()); - } } ``` -When a test needs different samples (non-compiling, Java version variants), keep one constant per sample and one factory parameterised by path — still no inline duplication. +A single shared check instance is safe because checks are [stateless](#stateless-checks). When a test needs several samples (non-compiling or Java version variants), declare one constant per sample. ### Method and type matching From 1ed506824537b858b3c2e52bfbbbd47f9aefad80 Mon Sep 17 00:00:00 2001 From: rombirli Date: Tue, 13 Oct 2026 15:51:19 +0200 Subject: [PATCH 3/3] Fix review comments --- .agents/skills/new-rule/SKILL.md | 10 ++++++++-- AGENTS.md | 2 ++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/.agents/skills/new-rule/SKILL.md b/.agents/skills/new-rule/SKILL.md index d232fd4db6d..cd1fb7ae2b1 100644 --- a/.agents/skills/new-rule/SKILL.md +++ b/.agents/skills/new-rule/SKILL.md @@ -156,7 +156,7 @@ Several locations can be marked on one caret line: ```java Arrays.fill(textBuffer, 42); // Noncompliant {{Message.}} -// ^^^^ ^^^^^^^^^^< +// ^^^^ ^^^^^^^^^^< ``` Use `[[sc=…;ec=…]]` / `[[secondary=…]]` only when carets cannot work: a token at column 1, a multi-line span, or quickfix declarations (`[[quickfixes=qf1]]`). @@ -246,7 +246,13 @@ public class MyCheck extends IssuableSubscriptionVisitor { If a rule genuinely needs cross-node context, compute it from the subscribed node by walking the AST, or subscribe to the enclosing node (class, method, compilation unit) and do the whole analysis there with local state. -The only accepted non-static fields are the ones the API itself requires: the `JavaFileScannerContext` of a `BaseTreeVisitor` rule, assigned at the start of `scanFile`, and the flags a `DependencyVersionAware` rule sets in `isCompatibleWithDependencies` before the scan starts. +The only accepted non-static fields are the ones the API itself requires, all set before the scan and never mutated during it: + +- `@RuleProperty` parameter fields, which the platform injects and therefore cannot be `static final` +- the `JavaFileScannerContext` of a `BaseTreeVisitor` rule, assigned at the start of `scanFile` +- the flags a `DependencyVersionAware` rule sets in `isCompatibleWithDependencies` + +Because a stateless check has nothing to reset, it does not need `clearState()`. The `clearState()` guidance in `AGENTS.md` applies to existing checks that still hold per-file state, not to new rules. ### External Dependencies in Tests - **DO NOT** add external library dependencies for test samples diff --git a/AGENTS.md b/AGENTS.md index 35be242870d..32823c097b5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -127,6 +127,8 @@ reportIssue(tree, "Message").withQuickFix(() -> quickFix); ### Per-File State Cleanup +New rules must be **stateless**: see the stateless-checks section of `.agents/skills/new-rule/SKILL.md`. The guidance below applies to existing checks that still accumulate per-file state. + Rules that accumulate state in collections or mutable fields during file analysis **must clear that state between files**. A single rule instance is reused across all files in a module. For `IssuableSubscriptionVisitor` subclasses, override `clearState()` — it is called automatically before and after each file: