From 017510df52f01567cdec466e936924f20213881b Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Fri, 21 Aug 2026 17:09:12 +0200 Subject: [PATCH 1/4] Add AGENTS.md to provide guidance to agents making changes to rules in this repo --- AGENTS.md | 255 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ CLAUDE.md | 1 + 2 files changed, 256 insertions(+) create mode 100644 AGENTS.md create mode 120000 CLAUDE.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 00000000000..05ca2482590 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,255 @@ +# Introduction + +SonarJava is a static code analyzer for Java. + +The rules are implemented in **Java** using the SonarJava API. Test resources are **Java** files annotated with special comment markers. + +--- + + +# General Coding Guidelines + +- Do **NOT** write comments, inside the code. Comments can easily become outdated. +- Do write documentation for complex functions. +- Use KISS (Keep It Simple, Stupid) and DRY (Don't Repeat Yourself) principles. + +--- + +# Repository Structure + +## Maven Module Structure + +``` +sonar-java/ +├── java-frontend/ # Parser, AST, semantic model, type inference +├── java-checks/ # Community rule implementations +│ ├── src/main/java/org/sonar/java/checks/ # Rule Java source +│ └── src/test/ +│ ├── java/org/sonar/java/checks/ # Test classes +│ └── files/checks/ # Legacy Java test resource files. Should be migrated to java-checks-test-sources +├── java-checks-test-sources/ # Shared Java test resource files used across modules +├── java-checks-aws/ # AWS-specific rule implementations +├── java-symbolic-execution/ # Symbolic execution engine +├── sonar-java-plugin/ # Plugin packaging +└── its/ # Integration tests + └── ruling/ # Ruling integration tests +``` + +## Rule Implementation Pattern + +Rules are Java classes annotated with `@Rule(key = "SXXXX")` that extend one of: + +- **`IssuableSubscriptionVisitor`** — Event-based: register tree consumers for specific tree kinds +- **`BaseTreeVisitor`** — Visitor pattern: override `visit*` methods; combined with `JavaFileScanner` to report issues +- **`JavaFileScanner`** — Full scan: override `scanFile` to traverse the entire compilation unit + +### Example: IssuableSubscriptionVisitor + +```java +@Rule(key = "S1234") +public class MyCheck extends IssuableSubscriptionVisitor { + + @Override + public List nodesToVisit() { + return Collections.singletonList(Tree.Kind.METHOD_INVOCATION); + } + + @Override + public void visitNode(Tree tree) { + MethodInvocationTree mit = (MethodInvocationTree) tree; + // Analyze the invocation... + reportIssue(mit, "Message describing the issue."); + } +} +``` + +### Example: BaseTreeVisitor + +```java +@Rule(key = "S1234") +public class MyCheck extends BaseTreeVisitor implements JavaFileScanner { + + private JavaFileScannerContext context; + + @Override + public void scanFile(JavaFileScannerContext context) { + this.context = context; + scan(context.getTree()); + } + + @Override + public void visitMethodInvocation(MethodInvocationTree tree) { + // Analyze the invocation... + context.reportIssue(this, tree, "Message describing the issue."); + super.visitMethodInvocation(tree); + } +} +``` + +### Key API Elements + +- **`reportIssue(tree, message)`** — Report an issue at a specific AST node +- **`reportIssue(tree, message, secondaries, cost)`** — Report with secondary locations +- **`Tree.Kind.*`** — AST node type enumeration (e.g., `Tree.Kind.METHOD`, `Tree.Kind.IF_STATEMENT`, `Tree.Kind.METHOD_INVOCATION`) +- **`TreeUtils.firstAncestorOfKind(tree, Tree.Kind.METHOD)`** — Navigate up the AST +- **`ExpressionUtils`**, **`MethodMatchers`** — Utility classes for common analysis patterns + +### MethodMatchers + +Use `MethodMatchers` to match method calls by type, name, and signature: + +```java +private static final MethodMatchers MY_MATCHER = MethodMatchers.create() + .ofTypes("java.util.List") + .names("add") + .withAnyParameters() + .build(); + +// In visitNode: +if (MY_MATCHER.matches(methodInvocationTree)) { + reportIssue(methodInvocationTree, "Message."); +} +``` + +### Quick Fix API + +```java +JavaQuickFix quickFix = JavaQuickFix.newQuickFix("Remove redundant check") + .addTextEdit(JavaTextEdit.removeTree(tree)) + .build(); +reportIssue(tree, "Message").withQuickFix(() -> quickFix); +``` + +### Dependency-Aware Rules + +When a rule should only apply when a specific library is present, or when its behaviour depends on a dependency version, implement `DependencyVersionAware` alongside the visitor base class: + +```java +@Rule(key = "S2230") +public class TransactionalMethodVisibilityCheck extends IssuableSubscriptionVisitor implements DependencyVersionAware { + + private boolean isSpring6OrLater = false; + + /** Called before analysis. Return false to disable the rule entirely when required dependencies are absent. */ + @Override + public boolean isCompatibleWithDependencies(Function> dependencyFinder) { + Optional springTxVersion = dependencyFinder.apply("spring-tx"); + Optional springContextVersion = dependencyFinder.apply("spring-context"); + if (springTxVersion.isEmpty() && springContextVersion.isEmpty()) { + return false; // rule is disabled — dependency not on classpath + } + isSpring6OrLater = springContextVersion + .or(() -> springTxVersion) + .map(v -> v.isGreaterThanOrEqualTo("6.0")) + .orElse(false); + return true; + } + + @Override + public void visitNode(Tree tree) { + // use isSpring6OrLater to adjust rule behaviour + } +} +``` + +Key points: +- `dependencyFinder.apply("artifact-id")` looks up a dependency by its Maven artifact ID and returns its version if present. +- Returning `false` from `isCompatibleWithDependencies` disables the rule for the current project — use this when the rule only makes sense if a particular library is on the classpath. +- Store any version information in an instance field (e.g. `isSpring6OrLater`) so that `visitNode` can use it during the scan. + +--- + +# Understanding Rule Trigger Requirements + +When creating simple examples or fixing false positives, examine the rule's test files to understand what conditions trigger the rule: + +## AST Node Kinds + +Rules subscribe to specific tree kinds. Check `nodesToVisit()` to see which `Tree.Kind.*` values the rule listens to: +- `Tree.Kind.METHOD` — Method declarations +- `Tree.Kind.METHOD_INVOCATION` — Method calls +- `Tree.Kind.IF_STATEMENT` — If statements +- `Tree.Kind.FOR_STATEMENT` — For loops +- `Tree.Kind.VARIABLE` — Variable declarations + +## Semantic Model + +SonarJava provides a semantic model to resolve types and symbols: +- **`symbol.type()`** — Resolved type of symbol +- **`symbol.owner()`** — Enclosing symbol (e.g., class owning a method) +- **`expressionTree.symbolType()`** — Type of expression +- **`type.isSubtypeOf("java.util.Collection")`** — Type hierarchy checks + +## Java Version Context + +Rules may behave differently depending on the Java language level: +- Some rules target Java 8+ features (lambdas, streams, `Optional`) +- Some handle Java 11+ or Java 17+ features (var, records, sealed classes) + +--- + +# Test File Conventions + +The file general structure for a rule implementation and its tests is as follows: +- Rule class: `java-checks/src/main/java/org/sonar/java/checks/{RuleId}Check.java` +- Test class: `java-checks/src/test/java/org/sonar/java/checks/{RuleId}CheckTest.java` +- Test samples: `java-checks-test-sources/default/src/main/files/checks/{RuleId}CheckSample.java` + +## Test Class + +The test class should use `CheckVerifier` and test the rule both with and without semantic. +This is to prevent false positives when the rule is applied to code that does not have the required dependencies on the classpath. +Example: + +```java + +class AbsOnNegativeCheckTest { + + @Test + void test() { + CheckVerifier.newVerifier() + .onFile("src/test/files/checks/AbsOnNegative.java") + .withCheck(new AbsOnNegativeCheck()) + .verifyIssues(); + } + + @Test + void test_without_semantic() { + CheckVerifier.newVerifier() + .onFile("src/test/files/checks/AbsOnNegative.java") + .withCheck(new AbsOnNegativeCheck()) + .withoutSemantic() + .verifyIssues(); + } +} +``` + +## Test samples +Test resource files use inline comment markers to declare expected issues: + +```java +int x = foo(); // Noncompliant +int y = bar(); // Noncompliant {{Expected message text}} +int z = baz(); // compliant +``` + +- `// Noncompliant` — The analyzer must report an issue on this line +- `// Noncompliant {{message}}` — Issue must have this exact message +- `// compliant` — Optional annotation for clarity; no issue expected + +Secondary locations use `[[...]]` inline markers when needed. + +--- + +# Ruling Tests + +Ruling tests validate that rule fixes don't break existing true positives. They run the analyzer against real-world Java projects (`guava`, `commons-beanutils`, `eclipse-jetty`, `sonar-server`, `jboss-ejb3-tutorial`, `regex-examples`) and compare results to expected baselines. + +- Expected ruling files: `its/ruling/src/test/resources//java-.json` +- Actual ruling output: `its/ruling/target/actual//java-.json` +- Format: `{"group:artifact:path/to/File.java": [line1, line2, ...], ...}` +- Ruling sources: `its/sources//` + +Do not try to run these locally. A PR should be created automatically when they fail on CI. + +--- diff --git a/CLAUDE.md b/CLAUDE.md new file mode 120000 index 00000000000..47dc3e3d863 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1 @@ +AGENTS.md \ No newline at end of file From 3e3272a7b3d9b6255c373f4ddaae2d31fcbc9d66 Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Mon, 24 Aug 2026 11:22:05 +0200 Subject: [PATCH 2/4] Guidelines about not using abstract classes --- AGENTS.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 05ca2482590..f5575d2d7cb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -43,6 +43,9 @@ Rules are Java classes annotated with `@Rule(key = "SXXXX")` that extend one of: - **`BaseTreeVisitor`** — Visitor pattern: override `visit*` methods; combined with `JavaFileScanner` to report issues - **`JavaFileScanner`** — Full scan: override `scanFile` to traverse the entire compilation unit +To avoid coupling between rules, never introduce new abstract classes from which rules would inherit. +To avoid duplicating code, you may use utility classes. + ### Example: IssuableSubscriptionVisitor ```java From f5055431e5c47bb5fa5d68075420022701afe32a Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Mon, 24 Aug 2026 11:24:12 +0200 Subject: [PATCH 3/4] Address PR review feedback on AGENTS.md - Fix non-existent TreeUtils.firstAncestorOfKind to ExpressionUtils.getParentOfType - Add missing nodesToVisit() override in DependencyVersionAware example - Fix test sample path from files/checks/ to java/checks/ - Update test example to use TestUtils.mainCodeSourcesPath() - Add pointer to .claude/skills/new-rule/SKILL.md for metadata guidance Co-Authored-By: Claude Opus 4.6 --- AGENTS.md | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index f5575d2d7cb..1daaec495e0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -4,6 +4,8 @@ SonarJava is a static code analyzer for Java. The rules are implemented in **Java** using the SonarJava API. Test resources are **Java** files annotated with special comment markers. +For step-by-step guidance on implementing a new rule (including metadata generation), see `.claude/skills/new-rule/SKILL.md`. + --- @@ -94,7 +96,7 @@ public class MyCheck extends BaseTreeVisitor implements JavaFileScanner { - **`reportIssue(tree, message)`** — Report an issue at a specific AST node - **`reportIssue(tree, message, secondaries, cost)`** — Report with secondary locations - **`Tree.Kind.*`** — AST node type enumeration (e.g., `Tree.Kind.METHOD`, `Tree.Kind.IF_STATEMENT`, `Tree.Kind.METHOD_INVOCATION`) -- **`TreeUtils.firstAncestorOfKind(tree, Tree.Kind.METHOD)`** — Navigate up the AST +- **`ExpressionUtils.getParentOfType(tree, Tree.Kind.METHOD)`** — Navigate up the AST - **`ExpressionUtils`**, **`MethodMatchers`** — Utility classes for common analysis patterns ### MethodMatchers @@ -133,6 +135,11 @@ public class TransactionalMethodVisibilityCheck extends IssuableSubscriptionVisi private boolean isSpring6OrLater = false; + @Override + public List nodesToVisit() { + return Collections.singletonList(Tree.Kind.METHOD); + } + /** Called before analysis. Return false to disable the rule entirely when required dependencies are absent. */ @Override public boolean isCompatibleWithDependencies(Function> dependencyFinder) { @@ -196,7 +203,7 @@ Rules may behave differently depending on the Java language level: The file general structure for a rule implementation and its tests is as follows: - Rule class: `java-checks/src/main/java/org/sonar/java/checks/{RuleId}Check.java` - Test class: `java-checks/src/test/java/org/sonar/java/checks/{RuleId}CheckTest.java` -- Test samples: `java-checks-test-sources/default/src/main/files/checks/{RuleId}CheckSample.java` +- Test samples: `java-checks-test-sources/default/src/main/java/checks/{RuleId}CheckSample.java` ## Test Class @@ -205,13 +212,14 @@ This is to prevent false positives when the rule is applied to code that does no Example: ```java +import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; class AbsOnNegativeCheckTest { @Test void test() { CheckVerifier.newVerifier() - .onFile("src/test/files/checks/AbsOnNegative.java") + .onFile(mainCodeSourcesPath("checks/AbsOnNegativeCheckSample.java")) .withCheck(new AbsOnNegativeCheck()) .verifyIssues(); } @@ -219,7 +227,7 @@ class AbsOnNegativeCheckTest { @Test void test_without_semantic() { CheckVerifier.newVerifier() - .onFile("src/test/files/checks/AbsOnNegative.java") + .onFile(mainCodeSourcesPath("checks/AbsOnNegativeCheckSample.java")) .withCheck(new AbsOnNegativeCheck()) .withoutSemantic() .verifyIssues(); From 437387fa2e065e86b54fe50628847dd26820b02d Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Mon, 24 Aug 2026 13:48:50 +0200 Subject: [PATCH 4/4] Fix incorrect test sample path in new-rule skill The path used `main/files/checks/` instead of `main/java/checks/` for the normal test sample location. Co-Authored-By: Claude Opus 4.6 --- .claude/skills/new-rule/SKILL.md | 2 +- AGENTS.md | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/.claude/skills/new-rule/SKILL.md b/.claude/skills/new-rule/SKILL.md index 9bcaca11d2b..6334afdf79f 100644 --- a/.claude/skills/new-rule/SKILL.md +++ b/.claude/skills/new-rule/SKILL.md @@ -72,7 +72,7 @@ if (MY_MATCHER.matches(methodInvocationTree)) { ### Rule Implementation - Rule class: `java-checks/src/main/java/org/sonar/java/checks/{RuleId}Check.java` - Test class: `java-checks/src/test/java/org/sonar/java/checks/{RuleId}CheckTest.java` -- Test samples: `java-checks-test-sources/default/src/main/files/checks/{RuleId}CheckSample.java` +- Test samples: `java-checks-test-sources/default/src/main/java/checks/{RuleId}CheckSample.java` ### Metadata - HTML description: `sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/{RULE_ID}.html` diff --git a/AGENTS.md b/AGENTS.md index 1daaec495e0..a42996c6d09 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -250,6 +250,11 @@ int z = baz(); // compliant Secondary locations use `[[...]]` inline markers when needed. +The precise locations of the issues are underlined using `^` characters. To produce that underline use a script like: +```shell +str=">"; sub=""; pre="${str%%"$sub"*}" suf="${str#*"$sub"}"; echo "${pre//?/ }${sub//?/^}" +``` + --- # Ruling Tests