Skip to content

Commit

Permalink
Merge pull request #296 from jborgers/kotlin/AvoidRecompilingPatterns
Browse files Browse the repository at this point in the history
added AvoidRecompilingPatterns (Kotlin) including test
  • Loading branch information
stokpop committed Mar 15, 2024
2 parents ea072ac + a1a3807 commit 2c52958
Show file tree
Hide file tree
Showing 4 changed files with 161 additions and 5 deletions.
4 changes: 0 additions & 4 deletions rulesets/kotlin/jpinpoint-kotlin-rules.xml
Expand Up @@ -78,7 +78,6 @@ class Foo {
<priority>2</priority>
<properties>
<property name="tag" value="jpinpoint-rule" type="String" description="for-sonar"/>
<property name="version" value="3.1"/>
<property name="xpath">
<value><![CDATA[
//ImportHeader[.//T-Identifier[@Text='java'] and .//T-Identifier[@Text='io']][1]/ancestor::KotlinFile
Expand Down Expand Up @@ -168,7 +167,6 @@ class AvoidRecompilingXPathExpressionKotlin {
<priority>3</priority>
<properties>
<property name="tag" value="jpinpoint-rule" type="String" description="for-sonar"/>
<property name="version" value="3.1"/>
<property name="xpath">
<value><![CDATA[
//VariableDeclaration/Type//T-Identifier[@Text='StringBuffer'],
Expand Down Expand Up @@ -205,7 +203,6 @@ class AvoidStringBuffer {
<priority>3</priority>
<properties>
<property name="tag" value="jpinpoint-rule" type="String" description="for-sonar"/>
<property name="version" value="3.1"/>
<property name="xpath">
<value><![CDATA[
//ImportHeader[starts-with(@joinTokenText, 'importcom.netflix.hystrix')]
Expand All @@ -232,7 +229,6 @@ class AvoidStringBuffer {
<priority>2</priority>
<properties>
<property name="tag" value="jpinpoint-rule" type="String" description="for-sonar"/>
<property name="version" value="3.1"/>
<property name="xpath">
<value><![CDATA[
(:locally created http client builder without disableConnectionState :)
Expand Down
56 changes: 55 additions & 1 deletion src/main/resources/category/kotlin/common.xml
Expand Up @@ -102,7 +102,7 @@ class AvoidStringBuffer {
(: filter matches in functions :)
//ClassDeclaration//PrimaryExpression//SimpleIdentifier//T-Identifier[
(@Text="DecimalFormat" or @Text="ChoiceFormat")
and not(ancestor::FunctionDeclaration)
and not(ancestor::FunctionBody)
]
]]>
</value>
Expand Down Expand Up @@ -130,6 +130,60 @@ class Foo {
</example>
</rule>

<rule name="AvoidRecompilingPatterns"
class="net.sourceforge.pmd.lang.rule.XPathRule"
dfa="false"
language="kotlin"
message="Pattern.compile is used in a method. Compiling a regex pattern can be expensive, make it a constant or instance field."
typeResolution="true"
externalInfoUrl="${doc_root}/JavaCodePerformance.md#ireu02">
<description>A regular expression is compiled on every invocation. Problem: this can be expensive, depending on the length of the regular expression.&#13;
Solution: Usually a pattern is a literal, not dynamic and can be compiled only once. Assign it to a private constant or instance field.
java.util.Pattern objects are thread-safe so they can be shared among threads. (jpinpoint-rules)</description>
<priority>2</priority>
<properties>
<property name="tag" value="jpinpoint-rule" type="String" description="for-sonar"/>
<property name="xpath">
<value><![CDATA[
(: check if java.util.regex imports 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']][1]/ancestor::KotlinFile
//FunctionBody//T-Identifier[@Text='Pattern' and ../../../PostfixUnarySuffix//T-Identifier[@Text='compile']
(: check for variables $action or ${action} in String of compile() call (first in possible call chain): allow dynamic input :)
and not(((../../..//ValueArgument)[1]//PostfixUnaryExpression//T-Identifier|(../../..//ValueArgument)[1]//T-LineStrRef)[
@Text=ancestor::FunctionDeclaration//FunctionValueParameter/Parameter/SimpleIdentifier/T-Identifier/@Text
or @Text=concat('$', ancestor::FunctionDeclaration//FunctionValueParameter/Parameter/SimpleIdentifier/T-Identifier/@Text)])
]
]]>
</value>
</property>
</properties>
<example>
<![CDATA[
import java.util.regex.Pattern
internal object Bad {
const val STR_PAT1: String = "[A-Z][a-z]+"
fun bad() {
val p1: Pattern = Pattern.compile(STR_PAT1) // bad
val p2: Pattern = Pattern.compile("(?=\\p{Lu})") // bad
val b: Boolean = p1.matcher("Start ").matches()
}
}
internal object Good {
val PAT1: Pattern = Pattern.compile("[A-Z][a-z]+")
val PAT2: Pattern = Pattern.compile("(?=\\p{Lu})")
fun good() {
val b: Boolean = PAT1.matcher("Start ").matches()
}
}
]]>
</example>
</rule>

<rule name="AvoidRecompilingXPathExpression" class="net.sourceforge.pmd.lang.rule.XPathRule"
language="kotlin"
message="XPathExpression is created and compiled every time. Beware it is thread-unsafe."
Expand Down
@@ -0,0 +1,6 @@
package com.jpinpoint.perf.lang.kotlin.ruleset.common;

import net.sourceforge.pmd.testframework.PmdRuleTst;

public class AvoidRecompilingPatternsTest extends PmdRuleTst {
}
@@ -0,0 +1,100 @@
<?xml version="1.0" encoding="UTF-8"?>
<test-data
xmlns="http://pmd.sourceforge.net/rule-tests"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests http://pmd.sourceforge.net/rule-tests_1_0_0.xsd">
<test-code>
<description>violation: Avoid recompiling patterns</description>
<expected-problems>3</expected-problems>
<expected-linenumbers>5,12,13</expected-linenumbers>
<code><![CDATA[
import java.util.regex.Pattern
class Foo {
fun createNameFromAction(action: String?) {
val p = Pattern.compile("(?=\\p{Lu})") // bad
}
companion object {
const val PATTERN: String = "^\\s*%s=(.*)"
fun methodBad(action: String?) {
val p = Pattern.compile("(?=\\p{Lu})") // bad: violation, should not be inside method, make static final
val s = Pattern.compile(PATTERN) // bad: violation
}
}
}
]]></code>
</test-code>
<test-code>
<description>no violation: Avoid recompiling patterns</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
import java.util.ArrayList;
import java.util.List;
import java.util.regex.Pattern;
object Bar {
const val PATTERN: String = "^\\s*%s=(.*)"
fun createEventNameFromAction(action: String): List<*> {
val q1 = Pattern.compile("($action?=\\p{Lu})") // ok, dynamic -- TODO make function for this?
val q2 = Pattern.compile("(" + action + "?=\\p{Lu})") // ok, dynamic
val q3 = Pattern.compile("(${action}?=\\p{Lu})") // ok, dynamic
val r = Pattern.compile(String.format(PATTERN, action)) // ok, dynamic
val t = Pattern.compile(action) // dynamic, so ok (fix for JPCC-63)
val actionSplit = q.split(action) // ok
val list: MutableList<Array<String>> = ArrayList()
list.add(actionSplit)
return list
}
}
]]></code>
</test-code>

<test-code>
<description>no violation: Avoid recompiling patterns, other compile</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
class GoodHere {
fun otherCompile() {
MyCompiler.compile("stuff")
}
}
internal object MyCompiler {
fun compile(s: String?) {
}
}
]]></code>
</test-code>

<test-code>
<description>violation: Avoid recompiling patterns, with chained operations, issue #180</description>
<expected-problems>2</expected-problems>
<expected-linenumbers>5,10</expected-linenumbers>
<code><![CDATA[
import java.util.regex.Pattern;
class Foo {
private fun isAmountAllowed(amount: String): Boolean {
return LIMIT > Pattern.compile(SEP).matcher(amount).replaceAll("").length() // bad
}
private val isAmountAllowed2: Boolean
get() {
val bla: String = Pattern.compile("[.|,]").matcher("5.000,00").replaceAll("") // bad
val b = (11 > bla.length)
return b
}
companion object {
private const val LIMIT = 11
private const val SEP = "[.|,]"
}
}
]]></code>
</test-code>

</test-data>

0 comments on commit 2c52958

Please sign in to comment.