diff --git a/.agents/skills/new-rule/SKILL.md b/.agents/skills/new-rule/SKILL.md index 6ed42e4a115..cd1fb7ae2b1 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,129 @@ 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 -private static final MethodMatchers MY_MATCHER = MethodMatchers.create() - .ofTypes("java.util.List") - .names("add") - .withAnyParameters() +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 + +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 FooCheckTest { + + private static final String SAMPLE = "checks/FooCheckSample.java"; + private static final FooCheck CHECK = new FooCheck(); + + @Test + void test() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath(SAMPLE)) + .withCheck(CHECK) + .verifyIssues(); + } + + @Test + void test_without_semantic() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath(SAMPLE)) + .withCheck(CHECK) + .withoutSemantic() + .verifyNoIssues(); + } +} +``` + +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 + +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, 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 - **DO** create mock-ups or use non-compiling tests when external libraries are needed 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: