Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions .github/copilot-instructions.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# Copilot Instructions for PMD-jPinpoint-rules

## Build, Test, and Lint Commands

- **Build and test all rules:**
- `./mvnw clean test`
- `./test` (shortcut for `./mvnw test`)
- **Run a single test class:**
- `./mvnw test -Dtest=YourRuleTest`
- **Merge rule categories into combined rulesets:**
- `./merge` (Java)
- `./merge kotlin` (Kotlin)

## High-Level Architecture

- **Rule definitions:**
- Rules are defined as XPath expressions in XML files under `src/main/resources/category/{java,kotlin}/`.
- Each category is registered in `categories.properties`.
- Combined rulesets (`rulesets/java/jpinpoint-rules.xml`, `rulesets/kotlin/jpinpoint-kotlin-rules.xml`) are generated by the merger tool.
- **Tests:**
- Each rule has an empty test class (extends `PmdRuleTst`) and a test data XML file with `<test-code>` blocks.
- Test classes: `src/test/java/com/jpinpoint/perf/lang/{java,kotlin}/ruleset/{category}/{RuleName}Test.java`
- Test data: `src/test/resources/com/jpinpoint/perf/lang/{java,kotlin}/ruleset/{category}/xml/{RuleName}.xml`
- **Merger tool:**
- Located in `rulesets-merger/src/main/java/com/jpinpoint/perf/tools/RulesetMerger.java`.
- Merges category XMLs into combined rulesets. Build with `cd rulesets-merger && mvn clean install`.

## Key Conventions

- **Rule XML files:**
- Edit category XMLs, not the combined ruleset files.
- Register new categories in `categories.properties`.
- **Test data XML:**
- Use `<description>` starting with `violation:` or `no violation:`.
- Use class names like `Foo`, method names like `bad`/`good`, and `//bad` comments on flagged lines.
- **Kotlin rules:**
- Complex rules can be implemented as Java classes extending `AbstractKotlinRule`.
- Use text comparison for token checks (e.g., `t.getText().equals("+=")`) (this is due to a little bug in pmd-core lib, waiting to be fixed).
- **Indentation:**
- Use spaces only (no tabs).

## Documentation

- Rule documentation is in `docs/` (e.g., `JavaCodePerformance.md`, `JavaCodeQuality.md`, `JavaDataAccessPerformance.md`).

---

Let me know if you want to adjust anything or add coverage for areas I may have missed.
92 changes: 92 additions & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
# CLAUDE.md

This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository.

## Project Overview

PMD-jPinpoint-rules is a collection of custom PMD static analysis rules for Java and Kotlin, focused on performance, sustainability, multi-threading, data mix-up, and code quality. Rules are implemented as XPath expressions inside XML rule definition files (no custom Java rule classes).

## Commands

### Build and test
```sh
mvn clean test # run all unit tests
./test # shorthand script (runs ./mvnw test)
```

### Run a single test class
```sh
mvn test -Dtest=AvoidSpringMVCMemoryLeaksTest
```

### Merge category XML files into the combined ruleset
```sh
./merge # merges Java categories → rulesets/java/jpinpoint-rules.xml
./merge kotlin # merges Kotlin categories → rulesets/kotlin/jpinpoint-kotlin-rules.xml
```
The merge step is required after editing any `src/main/resources/category/**/*.xml` file so that the combined `rulesets/` files stay up to date.

## Architecture

### Rule definitions (source of truth)
Rules live in per-category XML files under `src/main/resources/category/`:

| Language | Category files |
|----------|---------------|
| Java | `java/common.xml`, `java/common_std.xml`, `java/concurrent.xml`, `java/enterprise.xml`, `java/remoting.xml`, `java/spring.xml`, `java/sql.xml` |
| Kotlin | `kotlin/common.xml`, `kotlin/remoting.xml` |

Each category is registered in `src/main/resources/category/{java,kotlin}/categories.properties`.

Most rules use `class="net.sourceforge.pmd.lang.rule.xpath.XPathRule"` — rules are XPath 2.0 queries over the PMD AST. Complex rules that are hard to express in XPath can be implemented as Java classes (see below).

### Combined rulesets (generated)
`rulesets/java/jpinpoint-rules.xml` and `rulesets/kotlin/jpinpoint-kotlin-rules.xml` are **generated** by the merger tool from the category files. Edit the category files, not these.

### Tests
Each rule has exactly two files:

1. **Test class** — `src/test/java/com/jpinpoint/perf/lang/{java,kotlin}/ruleset/{category}/{RuleName}Test.java`
Always empty except for extending `PmdRuleTst`. The package path encodes the language and category, which PMD uses to locate the rule XML and test data.

2. **Test data** — `src/test/resources/com/jpinpoint/perf/lang/{java,kotlin}/ruleset/{category}/xml/{RuleName}.xml`
Contains `<test-code>` blocks with:
- `<description>` starting with `violation:` or `no violation:`
- `<expected-problems>` (count)
- `<expected-linenumbers>`
- `<code>` (CDATA) — use class names `Foo`, method names `bad`/`good`, and `//bad` comments on flagged lines

### Merger tool
`rulesets-merger/src/main/java/com/jpinpoint/perf/tools/RulesetMerger.java` — standalone Maven module that merges category XMLs into the combined ruleset files.

## Adding a New Rule

1. Add the rule definition to the appropriate `src/main/resources/category/{java,kotlin}/{category}.xml`
2. Create the empty test class in `src/test/java/com/jpinpoint/perf/lang/{java,kotlin}/ruleset/{category}/{RuleName}Test.java`
3. Create the test data XML in `src/test/resources/com/jpinpoint/perf/lang/{java,kotlin}/ruleset/{category}/xml/{RuleName}.xml`
4. Document the rule in the appropriate `docs/` markdown file
5. Run `./merge` to regenerate the combined ruleset
6. Run `./test` to verify

For a new category, also add an entry to `categories.properties`.

### Java-based Kotlin rules

For complex logic that is hard to express in XPath, rules can be implemented as Java classes:

- **Location**: `src/main/java/com/jpinpoint/perf/lang/kotlin/rule/{category}/{RuleName}Rule.java`
- **Pattern**: extend `AbstractKotlinRule`, override `buildTargetSelector()` and `buildVisitor()`
- **Visitor**: inner class extending `KotlinVisitorBase<RuleContext, Void>`, override `visitXxx()` methods
- **pmd-kotlin dependency**: must be `compile`-scoped (not `test`) so the rule class can import from `net.sourceforge.pmd.lang.kotlin.*`
- **XML registration**: use `class="com.jpinpoint.perf.lang.kotlin.rule.common.MyRule"` instead of `class="net.sourceforge.pmd.lang.rule.xpath.XPathRule"`; keep `language="kotlin"` attribute

**Key API notes** (PMD 7.x ANTLR-based Kotlin AST):
- `node.descendants(KtFoo.class)` / `node.ancestors(KtFoo.class)` / `node.children(KtFoo.class)` return `NodeStream<T>`
- `NodeStream` methods: `.any(Predicate)`, `.filter(Predicate)`, `.first()`, `.toList()`, `.nonEmpty()`, `.isEmpty()`
- `KotlinTerminalNode.getText()` returns the token image (e.g. `"String"`, `"+="`, `"+"`)
- **Do NOT use** `node.getToken(type, idx)` or `terminalNode.ADD()` / `terminalNode.ADD_ASSIGNMENT()` style methods — `BaseAntlrTerminalNode.getTokenKind()` returns the token stream index, not the token type, causing these to always return null. Use **text comparison** instead: `t.getText().equals("+=")`
- Reference implementation in pmd-kotlin jar: `net.sourceforge.pmd.lang.kotlin.rule.errorprone.OverrideBothEqualsAndHashcodeRule`

## Code Style

- Indentation: **spaces only** (no tabs)
59 changes: 32 additions & 27 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,21 @@
<properties>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<spring.version>6.2.10</spring.version>
<pmd.version>7.16.0</pmd.version>
<pmd.version>7.21.0</pmd.version>
</properties>

<dependencyManagement>
<dependencies>
<dependency>
<groupId>org.junit</groupId>
<artifactId>junit-bom</artifactId>
<version>5.14.2</version>
<type>pom</type>
<scope>import</scope>
</dependency>
</dependencies>
</dependencyManagement>

<build>
<plugins>
<plugin>
Expand All @@ -27,19 +39,14 @@
<configuration>
<source>11</source>
<target>11</target>
<!-- no need to compile this project -->
<skipMain>true</skipMain>
</configuration>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<version>3.5.2</version>
<version>3.5.4</version>
<configuration>
<includes>
<include>**/kotlin/**</include>
<include>**/java/**</include>
</includes>
<!-- Use Surefire defaults; ensure provider picks up JUnit Platform -->
</configuration>
</plugin>

Expand Down Expand Up @@ -74,6 +81,18 @@
<version>${pmd.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>net.sourceforge.pmd</groupId>
<artifactId>pmd-lang-test</artifactId>
<version>${pmd.version}</version>
<scope>test</scope>
<exclusions>
<exclusion>
<groupId>io.kotest</groupId>
<artifactId>*</artifactId>
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>net.sourceforge.pmd</groupId>
<artifactId>pmd-java</artifactId>
Expand All @@ -84,12 +103,12 @@
<groupId>net.sourceforge.pmd</groupId>
<artifactId>pmd-kotlin</artifactId>
<version>${pmd.version}</version>
<scope>test</scope>
</dependency>
<!-- Hamcrest for assertThat, matchers used in some tests -->
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<version>4.13.2</version>
<groupId>org.hamcrest</groupId>
<artifactId>hamcrest</artifactId>
<version>2.2</version>
<scope>test</scope>
</dependency>
<dependency>
Expand Down Expand Up @@ -223,12 +242,6 @@
<version>1.18.34</version>
<!--scope>provided</scope-->
</dependency>
<!-- https://mvnrepository.com/artifact/org.apache.maven.plugins/maven-surefire-plugin -->
<dependency>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<version>3.3.1</version>
</dependency>
<!-- https://mvnrepository.com/artifact/io.github.resilience4j/resilience4j-core -->
<dependency>
<groupId>io.github.resilience4j</groupId>
Expand Down Expand Up @@ -258,14 +271,6 @@
<version>2.20.0</version>
<scope>compile</scope>
</dependency>

</dependencies>
<repositories>
<repository>
<id>maven_central</id>
<name>Maven Central</name>
<url>https://repo.maven.apache.org/maven2/</url>
</repository>
</repositories>
</project>

</project>
101 changes: 6 additions & 95 deletions rulesets/kotlin/jpinpoint-kotlin-rules.xml
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ class Foo {
</rule>

<rule name="AvoidImplicitlyRecompilingRegex"
class="net.sourceforge.pmd.lang.rule.xpath.XPathRule"
class="com.jpinpoint.perf.lang.kotlin.rule.common.AvoidImplicitlyRecompilingRegexRule"
language="kotlin"
message="String regex method, Pattern.matches or FileSystem.getPathMatcher is used. Implicitly compiles a regex pattern, can be expensive."
externalInfoUrl="https://github.com/jborgers/PMD-jPinpoint-rules/tree/pmd7/docs/JavaCodePerformance.md#ireu01">
Expand All @@ -71,80 +71,6 @@ class Foo {
<priority>2</priority>
<properties>
<property name="tag" value="jpinpoint-rule" type="String" description="for-sonar"/>
<property name="xpath">
<value><![CDATA[
//FunctionDeclaration//PostfixUnaryExpression//SimpleIdentifier/T-Identifier[
@Text='Regex'
(: not called on fun parameter :)
and not(ancestor::PostfixUnaryExpression//CallSuffix//PrimaryExpression//T-Identifier[
@Text = ancestor::FunctionDeclaration//FunctionValueParameter//Parameter/SimpleIdentifier/T-Identifier/@Text])
(: not called on 'dynamic' function call :)
and not(ancestor::PostfixUnaryExpression/PostfixUnarySuffix/CallSuffix//CallSuffix//CallSuffix)
(: not if based on local variable initialized by fun parameter :)
and not(ancestor::CallSuffix//PostfixUnaryExpression/PrimaryExpression/SimpleIdentifier/T-Identifier[
@Text = ancestor::FunctionBody//PropertyDeclaration/VariableDeclaration/SimpleIdentifier/T-Identifier/@Text])
(: not param in String template in fun parameter :)
and not(../../../../PostfixUnaryExpression//CallSuffix/ValueArguments/ValueArgument//LineStringContent/T-LineStrRef[
@Text = ancestor::FunctionDeclaration//Parameter/SimpleIdentifier/T-Identifier/concat('$', @Text)])
(: not param in String template of class var field :)
and not(../../../../PostfixUnaryExpression//CallSuffix/ValueArguments/ValueArgument//LineStringContent/T-LineStrRef[
@Text = ancestor::ClassDeclaration//PropertyDeclaration[T-VAR]//SimpleIdentifier/T-Identifier/concat('$', @Text)])
(: not param in String concatenation of class var field :)
and not(../../../../PostfixUnaryExpression//CallSuffix/ValueArguments/ValueArgument[//LineStringLiteral]//AdditiveExpression/MultiplicativeExpression//SimpleIdentifier/T-Identifier[
@Text = ancestor::ClassDeclaration//PropertyDeclaration[T-VAR]//SimpleIdentifier/T-Identifier/@Text])
]

|

//FunctionDeclaration//PostfixUnaryExpression/PostfixUnarySuffix/NavigationSuffix/SimpleIdentifier/T-Identifier[
@Text='toRegex'
(: not called on function parameter :)
and not(../../../../PrimaryExpression//T-Identifier[
@Text = ancestor::FunctionDeclaration//FunctionValueParameter//Parameter/SimpleIdentifier/T-Identifier/@Text])
(: not called on 'dynamic' function call :)
and not(ancestor::PostfixUnaryExpression/PostfixUnarySuffix[1]/CallSuffix)
(: not if based on local variable initialized by fun parameter :)
and not(../../../../../PostfixUnaryExpression/PrimaryExpression/SimpleIdentifier/T-Identifier[
@Text = ancestor::FunctionBody//PropertyDeclaration/VariableDeclaration/SimpleIdentifier/T-Identifier/@Text])
]

|

(: check if java.util.regex.Pattern import exists as filter for same named classes in other packages :)
//ImportHeader[
(.//T-Identifier[@Text='java'] and .//T-Identifier[@Text='util'] and .//T-Identifier[@Text='regex'] and .//T-Identifier[@Text='Pattern'] or .//T-MULT)
][1]/ancestor::KotlinFile
//SimpleIdentifier/T-Identifier[@Text='Pattern'
and ../../../PostfixUnarySuffix//T-Identifier[@Text='matches']
(: not if first param is fun parameter :)
and not(../../../PostfixUnarySuffix/CallSuffix/ValueArguments/ValueArgument[1]//SimpleIdentifier/T-Identifier[
@Text = ancestor::FunctionDeclaration//Parameter/SimpleIdentifier/T-Identifier/@Text])
]

|

(: check if java.nio.file.FileSystems or java.nio.file.Path imports exists as filter for same named classes in other packages :)
(: TODO is this a bug: having to use T-FILE for 'file' :)
//ImportHeader[
(.//T-Identifier[@Text='java']
and .//T-Identifier[@Text='nio'] and .//T-FILE[@Text='file'] and .//T-Identifier[@Text='FileSystems' or @Text='Path'] or .//T-MULT)
][1]/ancestor::KotlinFile
(: added [ancestor::NavigationSuffix] to avoid PAT_STRING param from being selected too :)
//FunctionDeclaration//PostfixUnarySuffix//T-Identifier[ancestor::NavigationSuffix][@Text='getPathMatcher'
and (ancestor::PostfixUnaryExpression/PrimaryExpression//T-Identifier[@Text=
ancestor::Statement/preceding-sibling::Statement//VariableDeclaration[../Expression//T-Identifier[@Text='FileSystems']]//T-Identifier/@Text]
or ancestor::PostfixUnaryExpression/PrimaryExpression/SimpleIdentifier/T-Identifier[@Text='FileSystems']

)
(: not if first param is fun parameter :)
and not(ancestor::PrefixUnaryExpression//CallSuffix/ValueArguments/ValueArgument//SimpleIdentifier/T-Identifier[
@Text = ancestor::FunctionDeclaration//Parameter/SimpleIdentifier/T-Identifier/@Text])
(: not param in String template in fun parameter :)
and not(ancestor::PrefixUnaryExpression//CallSuffix/ValueArguments/ValueArgument//LineStringContent/T-LineStrRef[
@Text = ancestor::FunctionDeclaration//Parameter/SimpleIdentifier/T-Identifier/concat('$', @Text)])
]
]]></value>
</property>
</properties>
<example>
class Foo {
Expand Down Expand Up @@ -218,34 +144,19 @@ class AvoidInMemoryStreamingDefaultConstructor {
</example>
</rule>

<rule name="AvoidMultipleConcatStatements" class="net.sourceforge.pmd.lang.rule.xpath.XPathRule" language="kotlin" message="Multiple statements concatenate to the same String. Use StringBuilder append." externalInfoUrl="https://github.com/jborgers/PMD-jPinpoint-rules/tree/pmd7/docs/JavaCodePerformance.md#isu02">
<rule name="AvoidMultipleConcatStatements"
language="kotlin"
message="Multiple statements concatenate to the same String. Use StringBuilder append."
externalInfoUrl="https://github.com/jborgers/PMD-jPinpoint-rules/tree/pmd7/docs/JavaCodePerformance.md#isu02"
class="com.jpinpoint.perf.lang.kotlin.rule.common.AvoidMultipleConcatStatementsRule">
<description>Multiple statements concatenate to the same String. Problem: Each statement with one or more +-operators creates a hidden temporary StringBuilder, a char[] and a new String object, which all have to be garbage collected.&#13;
Solution: Use StringBuilder.append.
(jpinpoint-rules)</description>
<priority>2</priority>
<properties>
<property name="tag" value="jpinpoint-rule" type="String" description="for-sonar"/>
<property name="xpath">
<value><![CDATA[
//FunctionBody[count(.//Assignment[.//(AssignmentAndOperator/T-ADD_ASSIGNMENT|AdditiveOperator/T-ADD)]/(AssignableExpression|DirectlyAssignableExpression)//T-Identifier[@Text=
(: property a string literal :)
ancestor::FunctionBody//PropertyDeclaration[(
Expression[not(.//Expression)]//StringLiteral or
(: or explicit type String:)
VariableDeclaration/Type//T-Identifier[@Text='String'] or
(: or assigned parameter of type String :)
Expression[.//T-Identifier[@Text = ancestor::FunctionDeclaration/FunctionValueParameters/FunctionValueParameter[Parameter/Type//T-Identifier[@Text='String']]//T-Identifier/@Text]] or
(: or assigned non-nested function of explicit type String :) (: known issue: ignores parameters, may give false positives with overloaded methods :)
Expression[.//T-Identifier[not(ancestor::CallSuffix)][@Text = ancestor::ClassMemberDeclarations//FunctionDeclaration[Type//T-Identifier[@Text='String']]//T-Identifier/@Text]]
)]
/VariableDeclaration//T-Identifier/@Text]) > 1]
//Statement[Assignment//(T-ADD_ASSIGNMENT|T-ADD)][position()=last()]

]]></value>
</property>
</properties>
<example>

</example>
</rule>

Expand Down
Loading