Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

added AvoidRecompilingPatterns (Kotlin) including test #296

Merged
merged 22 commits into from Mar 15, 2024
Merged

added AvoidRecompilingPatterns (Kotlin) including test #296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
de508ae
fix for multi match because of multiple import matches
Feb 18, 2024
cc5687a
AvoidRecompilingXPathExpression. Convert applicable rules to Kotlin #185
jborgers Mar 1, 2024
3d3e39d
Merge branch 'master' into kotlin/AvoidRecompilingXPathExpression
jborgers Mar 8, 2024
7da8d59
Convert applicable rules to Kotlin #185,
jborgers Mar 8, 2024
7824328
Add TODO to EvaluateRules doc
jborgers Mar 8, 2024
1ed00dd
Merge remote-tracking branch 'origin/kotlin/AvoidDecimalAndChoiceForm…
jborgers Mar 11, 2024
99198af
Added good ThreadLocal example and removed 'TODO'
jborgers Mar 11, 2024
e921a27
added braces around 'x and y' for clarity
jborgers Mar 11, 2024
ae12b77
added AvoidRecompilingPatterns including test (Convert applicable rul…
Mar 12, 2024
7af02b9
Merge branch 'master' into kotlin/AvoidRecompilingPatterns
Mar 12, 2024
ac8957a
update kotlin progress table
Mar 12, 2024
58c759f
removed xpath version
jborgers Mar 14, 2024
7452395
removed other xpath version
jborgers Mar 14, 2024
74d572f
added testkt script
jborgers Mar 14, 2024
aca86ac
Merge remote-tracking branch 'origin/master' into kotlin/AvoidRecompi…
jborgers Mar 14, 2024
22eec3b
Merge pull request #295 from jborgers/kotlin/AvoidRecompilingXPathExp…
jborgers Mar 14, 2024
e12d3ca
update EvaluateRulesForKotlin
jborgers Mar 14, 2024
00adc32
Fix AvoidRecompilingXPathExpression Java Good example.
jborgers Mar 14, 2024
1f61132
Update EvaluateRulesForKotlin
jborgers Mar 14, 2024
c221ab7
added AvoidRecompilingPatterns including test (Convert applicable rul…
Mar 12, 2024
25bc928
remove deprecated version tags
Mar 15, 2024
a1a3807
Merge remote-tracking branch 'origin/kotlin/AvoidRecompilingPatterns'…
Mar 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 19 additions & 19 deletions docs/EvaluateRulesForKotlin.md
View file
Open in desktop
Expand Up @@ -2,23 +2,23 @@ Evaluation of Java PMD rules for use as Kotlin rules
---
### Common

| # | Who does | Rule name | useful | complexity | used by sponsors | importance | already available | note / to investigate |
|-----|----------|------------------------------------------|--------|------------|------------------|-------------|-------------------|-------------------------------------------------------------------------------------------------------------------------|
| 1. | | AvoidCDIReferenceLeak | Yes | Medium | No | Low | Not found | Kotlin mostly not used with Java/JakartaEE |
| 2. | | AvoidConstantsInInterface | ? | Low? | Yes | Low | Not found | To investiate |
| 3. | PP-1-M | AvoidDecimalAndChoiceFormatAsField | Yes? | Low | Yes | High | Not found | NumberFormat/DateFormat not included? |
| 4. | | AvoidDuplicateAssignmentsInCases | Yes | Medium | Yes | Low/Medium | Partly found | Detekt:DuplicateCaseInWhenExpression has overlap but is not the same. Add example, doc, Questionable if occuring often. |
| 5. | PP-6 | AvoidImplicitlyRecompilingRegex | Yes | High | Yes | High | Not found | Kotlin has own String/regex, also occurs here? support both? |
| 6. | JB-1-M | AvoidInMemoryStreamingDefaultConstructor | Yes | Low | Yes | High | Not found | Kotlin types? -> No |
| 7. | JB-5 | AvoidMultipleConcatStatements | Yes | Medium | Yes | High | Not found | How concat in Kotlin? Seems like Java |
| 8. | PP-3-WIP | AvoidRecompilingPatterns | Yes | Low/Medium | Yes | High | Not found | Kotlin version? |
| 9. | JB-3-WIP | AvoidRecompilingXPathExpression | Yes | Low | Yes | Medium/High | Not found | Good example ThreadLocal in Kotlin? |
| 10. | PP-4 | AvoidRecreatingDateTimeFormatter | Yes | Medium | Yes | High | Not found | - |
| 11. | | AvoidReflectionInToStringAndHashCode | Yes | Low/Medium | Yes | Low/Medium | Not found | - |
| 12. | PP-2-PR | AvoidSimpleDateFormat | Yes | Low | Yes | Medium | Not found | |
| 13. | JB-2-M | AvoidStringBuffer | Yes | Low | Yes | Low/Medium | Not found | |
| 14. | JB-7 | AvoidUnconditionalBuiltLogStrings | Yes | High | Yes | Medium | Not found | |
| 15. | PP-5 | AvoidWideScopeXPathExpression | Yes | Low | Yes | Medium | Not found | |
| 16. | JB-6 | AvoidXPathAPIUsage | Yes | Low | Yes | Medium | Not found | remove VTD reference?, seems old, better alternatives? |
| 17. | JB-4 | Kotlin function hasImport | | | | | | |
| # | Who does | Rule name | useful | complexity | used by sponsors | importance | already available | note / to investigate |
|-----|-------------|------------------------------------------|--------|------------|------------------|-------------|-------------------|-------------------------------------------------------------------------------------------------------------------------|
| 1. | | AvoidCDIReferenceLeak | Yes | Medium | No | Low | Not found | Kotlin mostly not used with Java/JakartaEE |
| 2. | | AvoidConstantsInInterface | ? | Low? | Yes | Low | Not found | To investiate |
| 3. | PP-1-M | AvoidDecimalAndChoiceFormatAsField | Yes? | Low | Yes | High | Not found | NumberFormat/DateFormat not included? |
| 4. | | AvoidDuplicateAssignmentsInCases | Yes | Medium | Yes | Low/Medium | Partly found | Detekt:DuplicateCaseInWhenExpression has overlap but is not the same. Add example, doc, Questionable if occuring often. |
| 5. | PP-6 | AvoidImplicitlyRecompilingRegex | Yes | High | Yes | High | Not found | Kotlin has own String/regex, also occurs here? support both? |
| 6. | JB-1-M | AvoidInMemoryStreamingDefaultConstructor | Yes | Low | Yes | High | Not found | Kotlin types? -> No |
| 7. | JB-5-WIP | AvoidMultipleConcatStatements | Yes | Medium | Yes | High | Not found | How concat in Kotlin? Seems like Java |
| 8. | PP-3-PR | AvoidRecompilingPatterns | Yes | Low/Medium | Yes | High | Not found | Kotlin version? |
| 9. | JB-3-M | AvoidRecompilingXPathExpression | Yes | Low | Yes | Medium/High | Not found | Good example ThreadLocal in Kotlin - Done |
| 10. | PP-4-WIP | AvoidRecreatingDateTimeFormatter | Yes | Medium | Yes | High | Not found | - |
| 11. | | AvoidReflectionInToStringAndHashCode | Yes | Low/Medium | Yes | Low/Medium | Not found | - |
| 12. | PP-2-M | AvoidSimpleDateFormat | Yes | Low | Yes | Medium | Not found | |
| 13. | JB-2-M | AvoidStringBuffer | Yes | Low | Yes | Low/Medium | Not found | |
| 14. | JB-7 | AvoidUnconditionalBuiltLogStrings | Yes | High | Yes | Medium | Not found | |
| 15. | PP-5 | AvoidWideScopeXPathExpression | Yes | Low | Yes | Medium | Not found | |
| 16. | JB-6 | AvoidXPathAPIUsage | Yes | Low | Yes | Medium | Not found | remove VTD reference?, seems old, better alternatives? |
| 17. | JB-4-PR-WIP | Kotlin function hasImport | | | | | | source to review, not tested, to build+test (how?) |

22 changes: 13 additions & 9 deletions docs/JavaCodePerformance.md
View file
Open in desktop
Expand Up @@ -1729,16 +1729,20 @@ class Bad {
}

class Good {
private static final ThreadLocal<XPathFactory> tlFac = ThreadLocal.withInitial(XPathFactory::newInstance);
private static final ThreadLocal<XPathExpression> tlExpr;
static {
XPath xpath = tlFac.get().newXPath();
XPathExpression expr = xpath.compile("//book[author='Isaac Asimov']/title/text()"); // good
tlExpr = ThreadLocal.withInitial(expr);
}
public static NodeList good(Document doc) {
return (NodeList) tlExpr.get().evaluate(doc, XPathConstants.NODESET); // good
private static final ThreadLocal<XPathFactory> tlFac = ThreadLocal.withInitial(XPathFactory::newInstance);
private static final ThreadLocal<XPathExpression> tlExpr;
static {
XPath xpath = tlFac.get().newXPath();
try {
XPathExpression expr = xpath.compile("//book[author='Isaac Asimov']/title/text()");
tlExpr = ThreadLocal.withInitial(() -> expr); // good
} catch (XPathExpressionException e) {
throw new RuntimeException(e);
}
}
public static NodeList good(Document doc) throws XPathExpressionException {
return (NodeList) tlExpr.get().evaluate(doc, XPathConstants.NODESET); // good
}
}
```

Expand Down
106 changes: 101 additions & 5 deletions rulesets/kotlin/jpinpoint-kotlin-rules.xml
View file
Open in desktop
Expand Up @@ -11,6 +11,58 @@
<!-- IMPORTANT NOTICE: The content of this file is generated. Do not edit this file directly since changes may be lost when this file is regenerated! -->

<!-- BEGIN Included file 'common.xml' -->
<rule name="AvoidDecimalAndChoiceFormatAsField"
since="7.0"
language="kotlin"
message="Avoid using NumberFormat, DecimalFormat or ChoiceFormat as field since it is thread-unsafe."
class="net.sourceforge.pmd.lang.rule.XPathRule"
typeResolution="true"
externalInfoUrl="https://github.com/jborgers/PMD-jPinpoint-rules/tree/master/docs/JavaCodePerformance.md#isio01">
<description>
Problem: java.text.DecimalFormat and java.text.ChoiceFormat are thread-unsafe.
Solution: usual solution is to create a new local one when needed in a method.
(jpinpoint-rules)
(jpinpoint-kotlin-rules)</description>
<priority>1</priority>
<properties>
<property name="tag" value="jpinpoint-rule" type="String" description="for-sonar"/>
<property name="version" value="3.1"/>
<property name="xpath">
<value><![CDATA[
(: check if java.text imports exists as filter for same named classes in other packages :)
//ImportHeader[.//T-Identifier[@Text='java'] and .//T-Identifier[@Text='text']]/ancestor::KotlinFile
(: PrimaryExpression finds the constructors instead of type declarations :)
(: filter matches in functions :)
//ClassDeclaration//PrimaryExpression//SimpleIdentifier//T-Identifier[
(@Text="DecimalFormat" or @Text="ChoiceFormat")
and not(ancestor::FunctionDeclaration)
]
]]>
</value>
</property>
</properties>
<example><![CDATA[
class Foo {

companion object {
val NUMBER_FORMAT: DecimalFormat = DecimalFormat("###.###") // bad
}

private val numFormat: NumberFormat = DecimalFormat("###.###") //bad

private val limits = doubleArrayOf(1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0)
private val dayOfWeekNames = arrayOf("Sun", "Mon", "Tue", "Wed", "Thur", "Fri", "Sat")
private val form = ChoiceFormat(limits, dayOfWeekNames) // bad

fun shouldNotMatchInsideMethod() {
val format: NumberFormat = DecimalFormat("##.##")
val choiceFormat = ChoiceFormat(limits, dayOfWeekNames)
}
}
]]>
</example>
</rule>

<rule name="AvoidInMemoryStreamingDefaultConstructor"
since="7.0"
language="kotlin"
Expand All @@ -26,10 +78,9 @@
<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']]/ancestor::KotlinFile
//ImportHeader[.//T-Identifier[@Text='java'] and .//T-Identifier[@Text='io']][1]/ancestor::KotlinFile
//Expression//T-Identifier[(@Text='ByteArrayOutputStream' and ../../../
PostfixUnarySuffix//ValueArguments[not(ValueArgument) or ValueArgument//T-IntegerLiteral[number(@Text)<=32]])
or (@Text='StringWriter' and ../../../
Expand Down Expand Up @@ -57,6 +108,54 @@ class AvoidInMemoryStreamingDefaultConstructor {
</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."
typeResolution="true"
externalInfoUrl="https://github.com/jborgers/PMD-jPinpoint-rules/tree/master/docs/JavaCodePerformance.md#ux02">
<description>XPathExpression is created and compiled on every method call, compiled possibly implicitly by XPath::evaluate.
Problem: Creation of XPath and compilation of XPathExpression takes time. It may slow down your application. &#13;
Solution: 1. Avoid XPath usage. 2. Compile the xpath expression as String into a XPathExpression. However, since XPath and XPathExpression classes are thread-unsafe, they are not easily cached. Caching it in a ThreadLocal may be a solution (sure? - TODO) .
(jpinpoint-rules)</description>
<priority>2</priority>
<properties>
<property name="tag" value="jpinpoint-rule" type="String" description="for-sonar"/>
<property name="version" value="2.0"/>
<property name="xpath">
<value><![CDATA[
//ImportHeader[.//T-Identifier[@Text='javax'] and .//T-Identifier[@Text='xml'] and .//T-Identifier[@Text='xpath']]/ancestor::KotlinFile
//FunctionBody//T-Identifier[@Text='newXPath']
/ancestor::FunctionBody//T-Identifier[@Text='compile'
or @Text='evaluate' and ancestor::PostfixUnarySuffix/..//ValueArguments[count(./ValueArgument) = 3]]
(: note on limitation, the 2-argument version is not found, difficult to do the typeIs checking like in Java :)
]]></value>
</property>
</properties>
<example>
<![CDATA[
import org.w3c.dom.Document
import org.w3c.dom.NodeList
import javax.xml.xpath.*

class AvoidRecompilingXPathExpressionKotlin {
fun bad1(doc: Document?): NodeList {
val xpath: XPath = XPathFactory.newInstance().newXPath()
val expr: XPathExpression = xpath.compile("//book[author='Isaac Asimov']/title/text()") // bad
return expr.evaluate(doc, XPathConstants.NODESET) as NodeList
}

@Throws(XPathExpressionException::class)
fun bad2(doc: Document?): NodeList {
val xpath: XPath = XPathFactory.newInstance().newXPath()
val xPathQuery = "//book[author='Isaac Asimov']/title/text()"
return xpath.evaluate(xPathQuery, doc, XPathConstants.NODESET) as NodeList // bad
}
}
// TODO Good example
]]>
</example>
</rule>

<rule name="AvoidStringBuffer" class="net.sourceforge.pmd.lang.rule.XPathRule"
since="7.0"
language="kotlin"
Expand All @@ -68,7 +167,6 @@ class AvoidInMemoryStreamingDefaultConstructor {
<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 @@ -105,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 @@ -132,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
13 changes: 8 additions & 5 deletions src/main/resources/category/java/common.xml
View file
Open in desktop
Expand Up @@ -378,13 +378,16 @@ class Good {
private static final ThreadLocal<XPathExpression> tlExpr;
static {
XPath xpath = tlFac.get().newXPath();
XPathExpression expr = xpath.compile("//book[author='Isaac Asimov']/title/text()"); // good
tlExpr = ThreadLocal.withInitial(expr); // good
try {
XPathExpression expr = xpath.compile("//book[author='Isaac Asimov']/title/text()");
tlExpr = ThreadLocal.withInitial(() -> expr); // good
} catch (XPathExpressionException e) {
throw new RuntimeException(e);
}
}
public static NodeList good(Document doc) {
return tlExpr.get().evaluate(doc, XPathConstants.NODESET); // good
public static NodeList good(Document doc) throws XPathExpressionException {
return (NodeList) tlExpr.get().evaluate(doc, XPathConstants.NODESET); // good
}
}
]]>
</example>

Expand Down