From ab0ccc145cd3284d5253ee1dc8176d9ca2b0cc5e Mon Sep 17 00:00:00 2001 From: Andrea Aime Date: Sat, 3 Apr 2021 14:58:38 +0200 Subject: [PATCH 1/9] #3190, UseStandardCharsets, java, new rule --- .../resources/category/java/bestpractices.xml | 44 ++++- .../UseStandardCharsetsTest.java | 11 ++ .../bestpractices/xml/UseStandardCharsets.xml | 158 ++++++++++++++++++ 3 files changed, 212 insertions(+), 1 deletion(-) create mode 100644 pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UseStandardCharsetsTest.java create mode 100644 pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UseStandardCharsets.xml diff --git a/pmd-java/src/main/resources/category/java/bestpractices.xml b/pmd-java/src/main/resources/category/java/bestpractices.xml index 2cc342f80d..495a111624 100644 --- a/pmd-java/src/main/resources/category/java/bestpractices.xml +++ b/pmd-java/src/main/resources/category/java/bestpractices.xml @@ -1831,7 +1831,49 @@ public class Foo { - + + StandardCharsets has constants for various common character + sets, they should be used instead of calling "Charset.forName(name)" + + 3 + + + + + + + + + + + + + + + + + + + fail, US-ASCII + 1 + 7 + + + + + fail, ISO-8859-1 + 1 + 7 + + + + + fail, UTF-8 + 1 + 7 + + + + + fail, UTF-16BE + 1 + 7 + + + + + fail, UTF-16LE + 1 + 7 + + + + + fail, UTF-16 + 1 + 7 + + + + + pass, ISO-8859-2, no constant for it + 0 + + + + + pass, UTF-8 from StadardCharsets + 0 + + + + From 3b75c31c2868582c886f2e2cd2fe9b83d38995d6 Mon Sep 17 00:00:00 2001 From: Andrea Aime Date: Mon, 5 Apr 2021 10:38:20 +0200 Subject: [PATCH 2/9] #3190, UseStandardCharsets, java, new rule. Handling eviewer's feedback --- .../resources/category/java/bestpractices.xml | 35 ++++++++++--------- .../bestpractices/xml/UseStandardCharsets.xml | 6 ++-- 2 files changed, 21 insertions(+), 20 deletions(-) diff --git a/pmd-java/src/main/resources/category/java/bestpractices.xml b/pmd-java/src/main/resources/category/java/bestpractices.xml index 495a111624..d05f2a4a48 100644 --- a/pmd-java/src/main/resources/category/java/bestpractices.xml +++ b/pmd-java/src/main/resources/category/java/bestpractices.xml @@ -1838,23 +1838,24 @@ public class Foo { message="Please use StandardCharsets constants" class="net.sourceforge.pmd.lang.rule.XPathRule" externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_bestpractices.html#usestandardcharsets"> - - StandardCharsets has constants for various common character - sets, they should be used instead of calling "Charset.forName(name)" - - 3 - - - - - +Starting with Java 7, StandardCharsets provides constants for common Charset objects, such as UTF-8. +Using the constants is less error prone, and can provide a small performance advantage compared to `Charset.forName(...)` +since no scan across the internal `Charset` caches is needed. + + 3 + + + + + - - - - - + + + + - + - pass, UTF-8 from StadardCharsets + pass, UTF-8 from StandardCharsets 0 Date: Mon, 5 Apr 2021 18:22:49 +0200 Subject: [PATCH 3/9] Add test case for #2757 --- .../errorprone/closeresource/FakeContext.java | 23 +++++++++++++++++++ .../rule/errorprone/xml/CloseResource.xml | 21 +++++++++++++++++ 2 files changed, 44 insertions(+) create mode 100644 pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/closeresource/FakeContext.java diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/closeresource/FakeContext.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/closeresource/FakeContext.java new file mode 100644 index 0000000000..c676e72651 --- /dev/null +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/closeresource/FakeContext.java @@ -0,0 +1,23 @@ +/* + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.java.rule.errorprone.closeresource; + +import java.io.Closeable; +import java.io.IOException; + +/** + * Helper class for #2757 + */ +public class FakeContext implements Closeable, AutoCloseable { + + public T getBean(Class klass) { + return null; + } + + @Override + public void close() throws IOException { + + } +} diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml index 4b20baa3c6..d5cfebcf84 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml @@ -1659,4 +1659,25 @@ public class FalsePositive { } ]]> + + #2757 PMD 6.27.0 break support for Lombok's Cleanup annotation + 0 + + From 9af635047eb2cd614d2189b3c4682e652a666a53 Mon Sep 17 00:00:00 2001 From: Jonathan Wiesel Date: Mon, 5 Apr 2021 18:23:11 +0200 Subject: [PATCH 4/9] Add more limit consuming static method invocations --- .../OperationWithLimitsInLoopRule.java | 57 ++++++--- .../xml/OperationWithLimitsInLoop.xml | 109 ++++++++++++++++++ 2 files changed, 153 insertions(+), 13 deletions(-) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/performance/OperationWithLimitsInLoopRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/performance/OperationWithLimitsInLoopRule.java index 79dbe344e1..ebb6344f4e 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/performance/OperationWithLimitsInLoopRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/performance/OperationWithLimitsInLoopRule.java @@ -11,6 +11,7 @@ import net.sourceforge.pmd.lang.apex.ast.ASTDmlUndeleteStatement; import net.sourceforge.pmd.lang.apex.ast.ASTDmlUpdateStatement; import net.sourceforge.pmd.lang.apex.ast.ASTDmlUpsertStatement; import net.sourceforge.pmd.lang.apex.ast.ASTMethodCallExpression; +import net.sourceforge.pmd.lang.apex.ast.ASTRunAsBlockStatement; import net.sourceforge.pmd.lang.apex.ast.ASTSoqlExpression; import net.sourceforge.pmd.lang.apex.ast.ASTSoslExpression; import net.sourceforge.pmd.lang.apex.rule.internal.Helper; @@ -19,6 +20,14 @@ import net.sourceforge.pmd.lang.apex.rule.internal.Helper; * Warn users when code that could trigger governor limits is executing within a looping construct. */ public class OperationWithLimitsInLoopRule extends AbstractAvoidNodeInLoopsRule { + + private static final String APPROVAL_CLASS_NAME = "Approval"; + private static final String MESSAGING_CLASS_NAME = "Messaging"; + private static final String SYSTEM_CLASS_NAME = "System"; + + private static final String[] MESSAGING_LIMIT_METHODS = new String[] { "renderEmailTemplate", "renderStoredEmailTemplate", "sendEmail" }; + private static final String[] SYSTEM_LIMIT_METHODS = new String[] { "enqueueJob", "schedule", "scheduleBatch" }; + public OperationWithLimitsInLoopRule() { setProperty(CODECLIMATE_CATEGORIES, "Performance"); // Note: Often more complicated as just moving a few lines. @@ -33,12 +42,13 @@ public class OperationWithLimitsInLoopRule extends AbstractAvoidNodeInLoopsRule addRuleChainVisit(ASTDmlUndeleteStatement.class); addRuleChainVisit(ASTDmlUpdateStatement.class); addRuleChainVisit(ASTDmlUpsertStatement.class); - // Database methods - addRuleChainVisit(ASTMethodCallExpression.class); // SOQL addRuleChainVisit(ASTSoqlExpression.class); // SOSL addRuleChainVisit(ASTSoslExpression.class); + // Other limit consuming methods + addRuleChainVisit(ASTRunAsBlockStatement.class); + addRuleChainVisit(ASTMethodCallExpression.class); } // Begin DML Statements @@ -73,17 +83,6 @@ public class OperationWithLimitsInLoopRule extends AbstractAvoidNodeInLoopsRule } // End DML Statements - // Begin Database method invocations - @Override - public Object visit(ASTMethodCallExpression node, Object data) { - if (Helper.isAnyDatabaseMethodCall(node)) { - return checkForViolation(node, data); - } else { - return data; - } - } - // End Database method invocations - // Begin SOQL method invocations @Override public Object visit(ASTSoqlExpression node, Object data) { @@ -97,4 +96,36 @@ public class OperationWithLimitsInLoopRule extends AbstractAvoidNodeInLoopsRule return checkForViolation(node, data); } // End SOSL method invocations + + // Begin general method invocations + + @Override + public Object visit(ASTRunAsBlockStatement node, Object data) { + return checkForViolation(node, data); + } + + @Override + public Object visit(ASTMethodCallExpression node, Object data) { + if (Helper.isAnyDatabaseMethodCall(node) + || Helper.isMethodName(node, APPROVAL_CLASS_NAME, Helper.ANY_METHOD) + || checkLimitClassMethods(node, MESSAGING_CLASS_NAME, MESSAGING_LIMIT_METHODS) + || checkLimitClassMethods(node, SYSTEM_CLASS_NAME, SYSTEM_LIMIT_METHODS)) { + + return checkForViolation(node, data); + } else { + return data; + } + } + + private boolean checkLimitClassMethods(ASTMethodCallExpression node, String className, String[] methodNames) { + + for (String method : methodNames) { + if (Helper.isMethodName(node, className, method)) { + return true; + } + } + + return false; + } + // End general method invocations } diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/performance/xml/OperationWithLimitsInLoop.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/performance/xml/OperationWithLimitsInLoop.xml index 6c26bfb6c0..6079c4e408 100644 --- a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/performance/xml/OperationWithLimitsInLoop.xml +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/performance/xml/OperationWithLimitsInLoop.xml @@ -410,4 +410,113 @@ public class Foo { ]]> + + + + Problematic approval actions in loop + 3 + + + + Best Practice: Batch up data into a list and invoke your Approval actions once on that list of data. + 0 + accounts = new List(); + List reqs = new List(); + while(true) { + Account account = new Account(); + accounts.add(account); + + Approval.ProcessSubmitRequest req = new Approval.ProcessSubmitRequest(); + req.setObjectId(account.id); + reqs.add(req); + } + + Approval.process(reqs, true); + Approval.unlock(accounts); + Approval.lock(accounts); + } +} + ]]> + + + + + + Problematic messaging actions in loop + 3 + renderedRes = Messaging.renderEmailTemplate(cont.Id, acc.Id, new List()); + Messaging.SingleEmailMessage renderedMail = Messaging.renderStoredEmailTemplate(null, cont.Id, acc.Id); + + Messaging.sendEmail(new Messaging.SingleEmailMessage[]{email}); + } while(true); + } +} + ]]> + + + + + + Problematic system actions in loop + 3 + + + + + Problematic system.runAs action in loop + 1 + + + From cfb6b3d57bfb808a1267084eb1b8ee95649b8cbc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Mon, 5 Apr 2021 18:34:54 +0200 Subject: [PATCH 5/9] CloseResource: Add support for @lombok.Cleanup Fixes #2757 --- .../pmd/lang/java/ast/ASTLocalVariableDeclaration.java | 6 ++++++ .../pmd/lang/java/rule/errorprone/CloseResourceRule.java | 5 +++++ 2 files changed, 11 insertions(+) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTLocalVariableDeclaration.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTLocalVariableDeclaration.java index a5e241d58c..d7d71c7d28 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTLocalVariableDeclaration.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTLocalVariableDeclaration.java @@ -5,6 +5,7 @@ package net.sourceforge.pmd.lang.java.ast; import java.util.Iterator; +import java.util.List; import net.sourceforge.pmd.Rule; import net.sourceforge.pmd.annotation.InternalApi; @@ -45,6 +46,11 @@ public class ASTLocalVariableDeclaration extends AbstractJavaAccessNode implemen return visitor.visit(this, data); } + @Override + public List getDeclaredAnnotations() { + return findChildrenOfType(ASTAnnotation.class); + } + @Override public boolean hasSuppressWarningsAnnotationFor(Rule rule) { for (int i = 0; i < getNumChildren(); i++) { diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java index 038ccf99d5..f44a486833 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java @@ -45,6 +45,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTStatementExpression; import net.sourceforge.pmd.lang.java.ast.ASTTryStatement; import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclarator; import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId; +import net.sourceforge.pmd.lang.java.ast.Annotatable; import net.sourceforge.pmd.lang.java.ast.JavaNode; import net.sourceforge.pmd.lang.java.ast.TypeNode; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; @@ -187,6 +188,10 @@ public class CloseResourceRule extends AbstractJavaRule { List vars = method.findDescendantsOfType(ASTVariableDeclarator.class); Map resVars = new HashMap<>(); for (ASTVariableDeclarator var : vars) { + if (var.getParent() instanceof Annotatable + && ((Annotatable) var.getParent()).isAnnotationPresent("lombok.Cleanup")) { + continue; // auto cleaned up + } TypeNode varType = getTypeOfVariable(var); if (varType != null && isResourceTypeOrSubtype(varType)) { resVars.put(var, wrappedResourceTypeOrReturn(var, varType)); From 18c55199b9278d36ee06d4af49037e247f36f10c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Mon, 5 Apr 2021 18:37:29 +0200 Subject: [PATCH 6/9] Rename test --- .../pmd/lang/java/rule/errorprone/xml/CloseResource.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml index d5cfebcf84..a037edd9a5 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml @@ -1660,7 +1660,7 @@ public class FalsePositive { ]]> - #2757 PMD 6.27.0 break support for Lombok's Cleanup annotation + #2757 support @lombok.Cleanup annotation 0 Date: Sat, 10 Apr 2021 16:55:27 +0200 Subject: [PATCH 7/9] [doc] Update release notes, refs #3190, refs #3193 --- docs/pages/release_notes.md | 12 +++++ .../main/resources/rulesets/releases/6340.xml | 13 +++++ .../resources/category/java/bestpractices.xml | 48 ++++++++++--------- .../resources/rulesets/java/quickstart.xml | 1 + 4 files changed, 51 insertions(+), 23 deletions(-) create mode 100644 pmd-core/src/main/resources/rulesets/releases/6340.xml diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index b8f8783555..9a9c0e66fe 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -14,11 +14,23 @@ This is a {{ site.pmd.release_type }} release. ### New and noteworthy +#### New rules + +* The new Java rule {% rule "java/bestpractices/UseStandardCharsets" %} finds usages of `Charset.forName`, + where `StandardCharsets` can be used instead. + + This rule is also part of the Quickstart Ruleset (`rulesets/java/quickstart.xml`) for Java. + ### Fixed Issues +* java-bestpractices + * [#3190](https://github.com/pmd/pmd/issues/3190): \[java] Use StandardCharsets instead of Charset.forName + ### API Changes ### External Contributions +* [#3193](https://github.com/pmd/pmd/pull/3193): \[java] New rule: UseStandardCharsets - [Andrea Aime](https://github.com/aaime) + {% endtocmaker %} diff --git a/pmd-core/src/main/resources/rulesets/releases/6340.xml b/pmd-core/src/main/resources/rulesets/releases/6340.xml new file mode 100644 index 0000000000..2b6a6d29b0 --- /dev/null +++ b/pmd-core/src/main/resources/rulesets/releases/6340.xml @@ -0,0 +1,13 @@ + + + + +This ruleset contains links to rules that are new in PMD v6.34.0 + + + + + diff --git a/pmd-java/src/main/resources/category/java/bestpractices.xml b/pmd-java/src/main/resources/category/java/bestpractices.xml index d05f2a4a48..f255ecc758 100644 --- a/pmd-java/src/main/resources/category/java/bestpractices.xml +++ b/pmd-java/src/main/resources/category/java/bestpractices.xml @@ -1831,34 +1831,36 @@ public class Foo { - - + + Starting with Java 7, StandardCharsets provides constants for common Charset objects, such as UTF-8. Using the constants is less error prone, and can provide a small performance advantage compared to `Charset.forName(...)` since no scan across the internal `Charset` caches is needed. - - 3 - - - - + + 3 + + + + - - - - + + + + - - + + - + From 872b69d62bc604ac0712b4c22c942fd09d6c102d Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Sat, 10 Apr 2021 19:10:36 +0200 Subject: [PATCH 8/9] [doc] Update release notes, refs #2757, refs #3197 --- docs/pages/release_notes.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index b8f8783555..a6417c758b 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -16,6 +16,9 @@ This is a {{ site.pmd.release_type }} release. ### Fixed Issues +* java-errorprone + * [#2757](https://github.com/pmd/pmd/issues/2757): \[java] CloseResource: support Lombok's @Cleanup annotation + ### API Changes ### External Contributions From a23fa44028d05c92c889b7df063e1af10f7121c0 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Sat, 10 Apr 2021 19:23:35 +0200 Subject: [PATCH 9/9] [doc] Update release notes, refs #3198 --- docs/pages/release_notes.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index b8f8783555..a9e1521147 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -16,9 +16,14 @@ This is a {{ site.pmd.release_type }} release. ### Fixed Issues +* apex-performance + * [#3198](https://github.com/pmd/pmd/pull/3198): \[apex] OperationWithLimitsInLoopRule: Support more limit consuming static method invocations + ### API Changes ### External Contributions +* [#3198](https://github.com/pmd/pmd/pull/3198): \[apex] OperationWithLimitsInLoopRule: Support more limit consuming static method invocations - [Jonathan Wiesel](https://github.com/jonathanwiesel) + {% endtocmaker %}