From ae9dec9c5ee80d8ca85a683724e523fae75dda43 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 13 Feb 2020 18:40:25 +0100 Subject: [PATCH 01/21] [doc] Add github issue templates Refs #2249 --- .../bug_report.md} | 25 ++++++++++++++-- .github/ISSUE_TEMPLATE/config.yml | 8 +++++ .github/ISSUE_TEMPLATE/feature_request.md | 20 +++++++++++++ .github/ISSUE_TEMPLATE/new_rule.md | 27 +++++++++++++++++ .github/ISSUE_TEMPLATE/question.md | 14 +++++++++ .github/ISSUE_TEMPLATE/rule_violation.md | 29 +++++++++++++++++++ 6 files changed, 120 insertions(+), 3 deletions(-) rename .github/{ISSUE_TEMPLATE.md => ISSUE_TEMPLATE/bug_report.md} (54%) create mode 100644 .github/ISSUE_TEMPLATE/config.yml create mode 100644 .github/ISSUE_TEMPLATE/feature_request.md create mode 100644 .github/ISSUE_TEMPLATE/new_rule.md create mode 100644 .github/ISSUE_TEMPLATE/question.md create mode 100644 .github/ISSUE_TEMPLATE/rule_violation.md diff --git a/.github/ISSUE_TEMPLATE.md b/.github/ISSUE_TEMPLATE/bug_report.md similarity index 54% rename from .github/ISSUE_TEMPLATE.md rename to .github/ISSUE_TEMPLATE/bug_report.md index f71366f7bf..f667e0584b 100644 --- a/.github/ISSUE_TEMPLATE.md +++ b/.github/ISSUE_TEMPLATE/bug_report.md @@ -1,20 +1,39 @@ +--- +name: Bug report +about: Create a report to help us improve +title: '' +labels: bug +assignees: '' + +--- - - **Affects PMD Version:** -**Rule:** +Make sure, to test with the latest PMD version. **Description:** +A clear and concise description of what the bug is. + +**Exception Stacktrace:** + +``` +# Copy-paste the stack trace here +``` + **Code Sample demonstrating the issue:** ``` ``` +**Steps to reproduce:** + +1. Compile the project: `mvn verify` +2. Run PMD: `run.sh pmd -d src -f xml -R ruleset.xml` + **Running PMD through:** *[CLI | Ant | Maven | Gradle | Designer | Other]* diff --git a/.github/ISSUE_TEMPLATE/config.yml b/.github/ISSUE_TEMPLATE/config.yml new file mode 100644 index 0000000000..56a5a13ccb --- /dev/null +++ b/.github/ISSUE_TEMPLATE/config.yml @@ -0,0 +1,8 @@ +blank_issues_enabled: false +contact_links: + - name: PMD Designer Issues + url: https://github.com/pmd/pmd-designer/issues + about: Issues about the rule designer + - name: PMD Eclipse Plugin Issues + url: https://github.com/pmd/pmd-eclipse-plugin/issues + about: Issues about the PMD Eclipse Plugin diff --git a/.github/ISSUE_TEMPLATE/feature_request.md b/.github/ISSUE_TEMPLATE/feature_request.md new file mode 100644 index 0000000000..5efb987e38 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/feature_request.md @@ -0,0 +1,20 @@ +--- +name: Feature request +about: Suggest an idea for this project +title: '' +labels: enhancement +assignees: '' + +--- + +**Is your feature request related to a problem? Please describe.** +A clear and concise description of what the problem is. Ex. I'm always frustrated when [...] + +**Describe the solution you'd like** +A clear and concise description of what you want to happen. + +**Describe alternatives you've considered** +A clear and concise description of any alternative solutions or features you've considered. + +**Additional context** +Add any other context about the feature request here. diff --git a/.github/ISSUE_TEMPLATE/new_rule.md b/.github/ISSUE_TEMPLATE/new_rule.md new file mode 100644 index 0000000000..ec48bd982c --- /dev/null +++ b/.github/ISSUE_TEMPLATE/new_rule.md @@ -0,0 +1,27 @@ +--- +name: New Rule +about: You have an idea for a new rule? Great! +title: '' +labels: new-rule +assignees: '' + +--- + + +**Proposed Rule Name:** + +**Proposed Category:** One of [Best Practices | Code Style | Design | Documentation | Error Prone | Multithreading | Performance | Security] + +**Description:** + +**Code Sample:** This should include code, that should be flagged by the rule. If possible, the "correct" code +according to this new rule should also be demonstrated. + +``` + +``` + +**Possible Properties:** + +* Should this rule be customizable via properties? + diff --git a/.github/ISSUE_TEMPLATE/question.md b/.github/ISSUE_TEMPLATE/question.md new file mode 100644 index 0000000000..9389f8c3ba --- /dev/null +++ b/.github/ISSUE_TEMPLATE/question.md @@ -0,0 +1,14 @@ +--- +name: Question +about: Feel free to ask any question about PMD and its usage +title: '' +labels: question +assignees: '' + +--- + + + +**Description:** + diff --git a/.github/ISSUE_TEMPLATE/rule_violation.md b/.github/ISSUE_TEMPLATE/rule_violation.md new file mode 100644 index 0000000000..3a13db905b --- /dev/null +++ b/.github/ISSUE_TEMPLATE/rule_violation.md @@ -0,0 +1,29 @@ +--- +name: Rule violation +about: Let us know about a false positive/false negative +title: '' +labels: bug +assignees: '' + +--- + + +**Affects PMD Version:** + +**Rule:** + +**Description:** + +**Code Sample demonstrating the issue:** + +``` + +``` + +**Expected outcome:** + +* Does PMD report a violation, where there shouldn't be one? -> false-positive +* Is PMD missing to report a violation, where there should be one? -> false-negative + + +**Running PMD through:** *[CLI | Ant | Maven | Gradle | Designer | Other]* From 9cec4c3ac716c9feb3c25720cba5500e400f9b5d Mon Sep 17 00:00:00 2001 From: Pham Hai Trung Date: Thu, 27 Feb 2020 09:38:43 +0100 Subject: [PATCH 02/21] Add version to plugin The maven configuration will generate an issue if we don't specify the maven-pmd-plugin's version. Without the version, maven wouldn't be able to find the pmd plugin in central. --- docs/pages/pmd/userdocs/tools/maven.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/pages/pmd/userdocs/tools/maven.md b/docs/pages/pmd/userdocs/tools/maven.md index 2d07e1a07c..267c0fc429 100644 --- a/docs/pages/pmd/userdocs/tools/maven.md +++ b/docs/pages/pmd/userdocs/tools/maven.md @@ -58,6 +58,7 @@ PMD finds some violations. Therefore the `check` goal is used: org.apache.maven.plugins maven-pmd-plugin + 3.8 true true From 843f473252031ee9ccce6f999c36f34a533fab50 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Sat, 21 Mar 2020 11:27:40 +0100 Subject: [PATCH 03/21] Fix formatting --- docs/pages/pmd/projectdocs/trivia/news.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/pages/pmd/projectdocs/trivia/news.md b/docs/pages/pmd/projectdocs/trivia/news.md index 2729f89bcb..52f114c4af 100644 --- a/docs/pages/pmd/projectdocs/trivia/news.md +++ b/docs/pages/pmd/projectdocs/trivia/news.md @@ -11,7 +11,7 @@ author: Tom Copeland * March 2020 - [Helping Salesforce developers create readable and maintainable Apex code](https://gearset.com/blog/helping-sf-developers-create-readable-and-maintainable-apex-code) -* July 2019 - [Apex PMD | Static code analysis - Apex Hours](https://youtu.be/34PxAHtAavU) +* July 2019 - [Apex PMD \| Static code analysis - Apex Hours](https://youtu.be/34PxAHtAavU) * June 2019 - [Pluralsight](https://www.pluralsight.com/authors/don-robins) Course about leveraging PMD usage for Salesforce by [Robert Sösemann](https://github.com/rsoesemann) (Apex Language Module Contributor) [Play by Play: Automated Code Analysis in Salesforce - a Tools Deep-Dive](https://www.pluralsight.com/courses/play-by-play-automated-code-analysis-in-salesforce) From 40d7d27665659bf12b9aece15cd66b8da6cb76e1 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Sat, 21 Mar 2020 11:44:22 +0100 Subject: [PATCH 04/21] [doc] Update issue template for bug reports and rule violations --- .github/ISSUE_TEMPLATE/bug_report.md | 6 ++++-- .github/ISSUE_TEMPLATE/rule_violation.md | 3 +++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/.github/ISSUE_TEMPLATE/bug_report.md b/.github/ISSUE_TEMPLATE/bug_report.md index f667e0584b..0bd4d2f99f 100644 --- a/.github/ISSUE_TEMPLATE/bug_report.md +++ b/.github/ISSUE_TEMPLATE/bug_report.md @@ -31,8 +31,10 @@ A clear and concise description of what the bug is. **Steps to reproduce:** -1. Compile the project: `mvn verify` -2. Run PMD: `run.sh pmd -d src -f xml -R ruleset.xml` +Please provide detailed steps for how we can reproduce the bug. + +1. ... (e.g. if you're using maven: `mvn clean verify`) +2. ... **Running PMD through:** *[CLI | Ant | Maven | Gradle | Designer | Other]* diff --git a/.github/ISSUE_TEMPLATE/rule_violation.md b/.github/ISSUE_TEMPLATE/rule_violation.md index 3a13db905b..92404bd43b 100644 --- a/.github/ISSUE_TEMPLATE/rule_violation.md +++ b/.github/ISSUE_TEMPLATE/rule_violation.md @@ -12,6 +12,9 @@ assignees: '' **Rule:** +Please provide the rule name and a link to the rule documentation: + + **Description:** **Code Sample demonstrating the issue:** From 4f392c9d255950187eb5022fd80f8a44fe2645d4 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Sat, 21 Mar 2020 12:12:24 +0100 Subject: [PATCH 05/21] [doc] maven - add "Choosing the plugin version" --- docs/pages/pmd/userdocs/tools/maven.md | 57 +++++++++++++++++++++++--- 1 file changed, 51 insertions(+), 6 deletions(-) diff --git a/docs/pages/pmd/userdocs/tools/maven.md b/docs/pages/pmd/userdocs/tools/maven.md index 267c0fc429..0a1d45691b 100644 --- a/docs/pages/pmd/userdocs/tools/maven.md +++ b/docs/pages/pmd/userdocs/tools/maven.md @@ -2,7 +2,8 @@ title: Maven PMD Plugin tags: [userdocs, tools] permalink: pmd_userdocs_tools_maven.html -last_updated: August 2017 +last_updated: March 2020 +mpmd_version: 3.13.0 author: > Miguel Griffa , Romain PELISSE , @@ -13,6 +14,36 @@ author: > ### Running the pmd plugin +#### Choosing the plugin version + +When adding the maven-pmd-plugin to your pom.xml, you need to select a version. To figure out the +latest available version, have a look at the official [maven-pmd-plugin documentation](https://maven.apache.org/plugins/maven-pmd-plugin/). + +As of {{ page.last_updated }}, the current plugin version is **{{ page.mpmd_version }}**. + +The version of the plugin should be specified in `` and if using the project +report additionally in `` elements. Here's an example for the pluginManagement +section: + +```xml + + + + + org.apache.maven.plugins + maven-pmd-plugin + {{ page.mpmd_version }} + + + + +``` + +When defining the version in the pluginManagment section, then it doesn't need to be specified in the normal plugins +section. However, it should additionally be specified in the reporting section. + +More information, see [Guide to Configuring Plugin-ins](https://maven.apache.org/guides/mini/guide-configuring-plugins.html). + #### Generating a project report To include the PMD report in the project reports section add the following lines under @@ -26,6 +57,7 @@ the reports element in your pom.xml: org.apache.maven.plugins maven-pmd-plugin + {{ page.mpmd_version }} @@ -58,9 +90,11 @@ PMD finds some violations. Therefore the `check` goal is used: org.apache.maven.plugins maven-pmd-plugin - 3.8 + {{ page.mpmd_version }} - true + + true + true @@ -88,11 +122,13 @@ you add `cpd-check` as a goal. To specify a ruleset, simply edit the previous configuration: +``` xml org.apache.maven.plugins maven-pmd-plugin + {{ page.mpmd_version }} /rulesets/java/braces.xml @@ -104,6 +140,7 @@ To specify a ruleset, simply edit the previous configuration: +``` The value of the 'ruleset' element can either be a relative address, an absolute address or even an url. @@ -119,23 +156,30 @@ will be able to resolve those other ruleset references. When using the Maven PMD plugin 3.8 or later along with PMD 5.6.0 or later, you can enable incremental analysis to speed up PMD's execution while retaining the quality of the analysis. You can additionally customize where the cache is stored:: +```xml org.apache.maven.plugins maven-pmd-plugin + {{ page.mpmd_version }} - true - ${project.build.directory}/pmd/pmd.cache + + true + + ${project.build.directory}/pmd/pmd.cache +``` #### Other configurations The Maven PMD plugin allows you to configure CPD, targetJDK, and the use of XRef to link the report to html source files, and the file encoding: +```xml org.apache.maven.plugins maven-pmd-plugin + {{ page.mpmd_version }} true ISO-8859-1 @@ -143,6 +187,7 @@ the report to html source files, and the file encoding: 1.4 +``` #### Upgrading the PMD version at runtime @@ -164,7 +209,7 @@ Maven plugin will use and benefit from the latest bugfixes and enhancements: org.apache.maven.plugins maven-pmd-plugin - 3.8 + {{ page.mpmd_version }} net.sourceforge.pmd From ce07c9b7570c5b683154944173be8ed44e68e854 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Sat, 21 Mar 2020 12:13:23 +0100 Subject: [PATCH 06/21] [doc] Update release notes, refs #2314 --- docs/pages/release_notes.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index a6b5973f9e..055c82fa00 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -135,6 +135,7 @@ methods on {% jdoc apex::lang.apex.ast.ApexParserVisitor %} and its implementati * [#2278](https://github.com/pmd/pmd/pull/2278): \[java] fix UnusedImports rule for ambiguous static on-demand imports - [Kris Scheibe](https://github.com/kris-scheibe) * [#2279](https://github.com/pmd/pmd/pull/2279): \[apex] Add support for suppressing violations using the // NOPMD comment - [Gwilym Kuiper](https://github.com/gwilymatgearset) * [#2297](https://github.com/pmd/pmd/pull/2297): \[apex] Cognitive complexity metrics - [Gwilym Kuiper](https://github.com/gwilymatgearset) +* [#2314](https://github.com/pmd/pmd/pull/2314): \[doc] maven integration - Add version to plugin - [Pham Hai Trung](https://github.com/gpbp) {% endtocmaker %} From 0280668a514aa90288b2e0537f5828b173506a0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Sat, 21 Mar 2020 20:04:38 +0100 Subject: [PATCH 07/21] [apex] Replace usages of AbstractApexNode in rules by ApexNode --- .../sourceforge/pmd/lang/apex/ast/AbstractApexNode.java | 1 + .../java/net/sourceforge/pmd/lang/apex/ast/ApexNode.java | 2 ++ .../pmd/lang/apex/rule/documentation/ApexDocRule.java | 5 ++--- .../rule/errorprone/AvoidNonExistentAnnotationsRule.java | 4 ++-- .../pmd/lang/apex/rule/security/ApexBadCryptoRule.java | 4 ++-- .../lang/apex/rule/security/ApexInsecureEndpointRule.java | 8 ++++---- .../pmd/lang/apex/rule/security/ApexOpenRedirectRule.java | 3 +-- .../lang/apex/rule/security/ApexSOQLInjectionRule.java | 6 +++--- .../apex/rule/security/ApexSuggestUsingNamedCredRule.java | 6 +++--- .../lang/apex/rule/security/ApexXSSFromURLParamRule.java | 8 ++++---- 10 files changed, 24 insertions(+), 23 deletions(-) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/AbstractApexNode.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/AbstractApexNode.java index 9e5063b7df..9fb1539229 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/AbstractApexNode.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/AbstractApexNode.java @@ -59,6 +59,7 @@ public abstract class AbstractApexNode extends AbstractApexNo return node; } + @Override public boolean hasRealLoc() { try { Location loc = node.getLoc(); diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/ApexNode.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/ApexNode.java index e0b4ae004e..c7889f6beb 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/ApexNode.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/ApexNode.java @@ -49,4 +49,6 @@ public interface ApexNode extends Node { @Override ApexNode getParent(); + + boolean hasRealLoc(); } diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/documentation/ApexDocRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/documentation/ApexDocRule.java index 0e8764829b..ef44fde9eb 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/documentation/ApexDocRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/documentation/ApexDocRule.java @@ -18,7 +18,6 @@ import net.sourceforge.pmd.lang.apex.ast.ASTParameter; import net.sourceforge.pmd.lang.apex.ast.ASTProperty; import net.sourceforge.pmd.lang.apex.ast.ASTUserClass; import net.sourceforge.pmd.lang.apex.ast.ASTUserInterface; -import net.sourceforge.pmd.lang.apex.ast.AbstractApexNode; import net.sourceforge.pmd.lang.apex.ast.ApexNode; import net.sourceforge.pmd.lang.apex.rule.AbstractApexRule; @@ -107,7 +106,7 @@ public class ApexDocRule extends AbstractApexRule { return data; } - private void handleClassOrInterface(AbstractApexNode node, Object data) { + private void handleClassOrInterface(ApexNode node, Object data) { ApexDocComment comment = getApexDocComment(node); if (comment == null) { if (shouldHaveApexDocs(node)) { @@ -120,7 +119,7 @@ public class ApexDocRule extends AbstractApexRule { } } - private boolean shouldHaveApexDocs(AbstractApexNode node) { + private boolean shouldHaveApexDocs(ApexNode node) { if (!node.hasRealLoc()) { return false; } diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/errorprone/AvoidNonExistentAnnotationsRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/errorprone/AvoidNonExistentAnnotationsRule.java index 59a8b63e09..d90c5d0a38 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/errorprone/AvoidNonExistentAnnotationsRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/errorprone/AvoidNonExistentAnnotationsRule.java @@ -12,7 +12,7 @@ import net.sourceforge.pmd.lang.apex.ast.ASTProperty; import net.sourceforge.pmd.lang.apex.ast.ASTUserClass; import net.sourceforge.pmd.lang.apex.ast.ASTUserEnum; import net.sourceforge.pmd.lang.apex.ast.ASTUserInterface; -import net.sourceforge.pmd.lang.apex.ast.AbstractApexNode; +import net.sourceforge.pmd.lang.apex.ast.ApexNode; import net.sourceforge.pmd.lang.apex.rule.AbstractApexRule; /** @@ -58,7 +58,7 @@ public class AvoidNonExistentAnnotationsRule extends AbstractApexRule { return checkForNonExistentAnnotation(node, node.getModifiers(), data); } - private Object checkForNonExistentAnnotation(final AbstractApexNode node, final ASTModifierNode modifierNode, final Object data) { + private Object checkForNonExistentAnnotation(final ApexNode node, final ASTModifierNode modifierNode, final Object data) { for (ASTAnnotation annotation : modifierNode.findChildrenOfType(ASTAnnotation.class)) { if (!annotation.isResolved()) { addViolationWithMessage(data, node, "Use of non existent annotations will lead to broken Apex code which will not compile in the future."); diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexBadCryptoRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexBadCryptoRule.java index e1656d4c94..5e6c322f3c 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexBadCryptoRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexBadCryptoRule.java @@ -13,7 +13,7 @@ import net.sourceforge.pmd.lang.apex.ast.ASTMethodCallExpression; import net.sourceforge.pmd.lang.apex.ast.ASTUserClass; import net.sourceforge.pmd.lang.apex.ast.ASTVariableDeclaration; import net.sourceforge.pmd.lang.apex.ast.ASTVariableExpression; -import net.sourceforge.pmd.lang.apex.ast.AbstractApexNode; +import net.sourceforge.pmd.lang.apex.ast.ApexNode; import net.sourceforge.pmd.lang.apex.rule.AbstractApexRule; import net.sourceforge.pmd.lang.apex.rule.internal.Helper; @@ -71,7 +71,7 @@ public class ApexBadCryptoRule extends AbstractApexRule { return data; } - private void findSafeVariables(AbstractApexNode var) { + private void findSafeVariables(ApexNode var) { ASTMethodCallExpression methodCall = var.getFirstChildOfType(ASTMethodCallExpression.class); if (methodCall != null && Helper.isMethodName(methodCall, BLOB, VALUE_OF)) { ASTVariableExpression variable = var.getFirstChildOfType(ASTVariableExpression.class); diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexInsecureEndpointRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexInsecureEndpointRule.java index 26dc570a41..fe4e8abec6 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexInsecureEndpointRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexInsecureEndpointRule.java @@ -15,7 +15,7 @@ import net.sourceforge.pmd.lang.apex.ast.ASTLiteralExpression; import net.sourceforge.pmd.lang.apex.ast.ASTMethodCallExpression; import net.sourceforge.pmd.lang.apex.ast.ASTVariableDeclaration; import net.sourceforge.pmd.lang.apex.ast.ASTVariableExpression; -import net.sourceforge.pmd.lang.apex.ast.AbstractApexNode; +import net.sourceforge.pmd.lang.apex.ast.ApexNode; import net.sourceforge.pmd.lang.apex.rule.AbstractApexRule; import net.sourceforge.pmd.lang.apex.rule.internal.Helper; @@ -56,7 +56,7 @@ public class ApexInsecureEndpointRule extends AbstractApexRule { return data; } - private void findInsecureEndpoints(AbstractApexNode node) { + private void findInsecureEndpoints(ApexNode node) { ASTVariableExpression variableNode = node.getFirstChildOfType(ASTVariableExpression.class); findInnerInsecureEndpoints(node, variableNode); @@ -67,7 +67,7 @@ public class ApexInsecureEndpointRule extends AbstractApexRule { } - private void findInnerInsecureEndpoints(AbstractApexNode node, ASTVariableExpression variableNode) { + private void findInnerInsecureEndpoints(ApexNode node, ASTVariableExpression variableNode) { ASTLiteralExpression literalNode = node.getFirstChildOfType(ASTLiteralExpression.class); if (literalNode != null && variableNode != null) { @@ -100,7 +100,7 @@ public class ApexInsecureEndpointRule extends AbstractApexRule { } - private void runChecks(AbstractApexNode node, Object data) { + private void runChecks(ApexNode node, Object data) { ASTLiteralExpression literalNode = node.getFirstChildOfType(ASTLiteralExpression.class); if (literalNode != null && literalNode.isString()) { String literal = literalNode.getImage(); diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexOpenRedirectRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexOpenRedirectRule.java index 7be2a9bb46..d7aa86a537 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexOpenRedirectRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexOpenRedirectRule.java @@ -17,7 +17,6 @@ import net.sourceforge.pmd.lang.apex.ast.ASTNewObjectExpression; import net.sourceforge.pmd.lang.apex.ast.ASTUserClass; import net.sourceforge.pmd.lang.apex.ast.ASTVariableDeclaration; import net.sourceforge.pmd.lang.apex.ast.ASTVariableExpression; -import net.sourceforge.pmd.lang.apex.ast.AbstractApexNode; import net.sourceforge.pmd.lang.apex.ast.ApexNode; import net.sourceforge.pmd.lang.apex.rule.AbstractApexRule; import net.sourceforge.pmd.lang.apex.rule.internal.Helper; @@ -69,7 +68,7 @@ public class ApexOpenRedirectRule extends AbstractApexRule { return data; } - private void findSafeLiterals(AbstractApexNode node) { + private void findSafeLiterals(ApexNode node) { ASTBinaryExpression binaryExp = node.getFirstChildOfType(ASTBinaryExpression.class); if (binaryExp != null) { findSafeLiterals(binaryExp); diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexSOQLInjectionRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexSOQLInjectionRule.java index 0c818e5c7c..1e31919133 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexSOQLInjectionRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexSOQLInjectionRule.java @@ -23,7 +23,7 @@ import net.sourceforge.pmd.lang.apex.ast.ASTStandardCondition; import net.sourceforge.pmd.lang.apex.ast.ASTUserClass; import net.sourceforge.pmd.lang.apex.ast.ASTVariableDeclaration; import net.sourceforge.pmd.lang.apex.ast.ASTVariableExpression; -import net.sourceforge.pmd.lang.apex.ast.AbstractApexNode; +import net.sourceforge.pmd.lang.apex.ast.ApexNode; import net.sourceforge.pmd.lang.apex.rule.AbstractApexRule; import net.sourceforge.pmd.lang.apex.rule.internal.Helper; @@ -124,7 +124,7 @@ public class ApexSOQLInjectionRule extends AbstractApexRule { } - private void findSanitizedVariables(AbstractApexNode node) { + private void findSanitizedVariables(ApexNode node) { final ASTVariableExpression left = node.getFirstChildOfType(ASTVariableExpression.class); final ASTLiteralExpression literal = node.getFirstChildOfType(ASTLiteralExpression.class); final ASTMethodCallExpression right = node.getFirstChildOfType(ASTMethodCallExpression.class); @@ -171,7 +171,7 @@ public class ApexSOQLInjectionRule extends AbstractApexRule { } } - private void findSelectContainingVariables(AbstractApexNode node) { + private void findSelectContainingVariables(ApexNode node) { final ASTVariableExpression left = node.getFirstChildOfType(ASTVariableExpression.class); final ASTBinaryExpression right = node.getFirstChildOfType(ASTBinaryExpression.class); if (left != null && right != null) { diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexSuggestUsingNamedCredRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexSuggestUsingNamedCredRule.java index 1d967faff6..e7aa416651 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexSuggestUsingNamedCredRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexSuggestUsingNamedCredRule.java @@ -15,7 +15,7 @@ import net.sourceforge.pmd.lang.apex.ast.ASTMethodCallExpression; import net.sourceforge.pmd.lang.apex.ast.ASTUserClass; import net.sourceforge.pmd.lang.apex.ast.ASTVariableDeclaration; import net.sourceforge.pmd.lang.apex.ast.ASTVariableExpression; -import net.sourceforge.pmd.lang.apex.ast.AbstractApexNode; +import net.sourceforge.pmd.lang.apex.ast.ApexNode; import net.sourceforge.pmd.lang.apex.rule.AbstractApexRule; import net.sourceforge.pmd.lang.apex.rule.internal.Helper; @@ -86,7 +86,7 @@ public class ApexSuggestUsingNamedCredRule extends AbstractApexRule { } - private void findAuthLiterals(final AbstractApexNode node) { + private void findAuthLiterals(final ApexNode node) { ASTLiteralExpression literal = node.getFirstChildOfType(ASTLiteralExpression.class); if (literal != null) { ASTVariableExpression variable = node.getFirstChildOfType(ASTVariableExpression.class); @@ -98,7 +98,7 @@ public class ApexSuggestUsingNamedCredRule extends AbstractApexRule { } } - private void runChecks(final AbstractApexNode node, Object data) { + private void runChecks(final ApexNode node, Object data) { ASTLiteralExpression literalNode = node.getFirstChildOfType(ASTLiteralExpression.class); if (literalNode != null) { if (isAuthorizationLiteral(literalNode)) { diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexXSSFromURLParamRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexXSSFromURLParamRule.java index fe4813f12a..9eafb2f5a1 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexXSSFromURLParamRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexXSSFromURLParamRule.java @@ -17,7 +17,7 @@ import net.sourceforge.pmd.lang.apex.ast.ASTReturnStatement; import net.sourceforge.pmd.lang.apex.ast.ASTUserClass; import net.sourceforge.pmd.lang.apex.ast.ASTVariableDeclaration; import net.sourceforge.pmd.lang.apex.ast.ASTVariableExpression; -import net.sourceforge.pmd.lang.apex.ast.AbstractApexNode; +import net.sourceforge.pmd.lang.apex.ast.ApexNode; import net.sourceforge.pmd.lang.apex.rule.AbstractApexRule; import net.sourceforge.pmd.lang.apex.rule.internal.Helper; @@ -164,7 +164,7 @@ public class ApexXSSFromURLParamRule extends AbstractApexRule { } - private void findTaintedVariables(AbstractApexNode node, Object data) { + private void findTaintedVariables(ApexNode node, Object data) { final ASTMethodCallExpression right = node.getFirstChildOfType(ASTMethodCallExpression.class); // Looks for: (String) foo = // ApexPages.currentPage().getParameters().get(..) @@ -209,7 +209,7 @@ public class ApexXSSFromURLParamRule extends AbstractApexRule { } - private void processVariableAssignments(AbstractApexNode node, Object data, final boolean reverseOrder) { + private void processVariableAssignments(ApexNode node, Object data, final boolean reverseOrder) { ASTMethodCallExpression methodCallAssignment = node.getFirstChildOfType(ASTMethodCallExpression.class); if (methodCallAssignment != null) { @@ -252,7 +252,7 @@ public class ApexXSSFromURLParamRule extends AbstractApexRule { } - private void processBinaryExpression(AbstractApexNode node, Object data) { + private void processBinaryExpression(ApexNode node, Object data) { ASTBinaryExpression nestedBinaryExpression = node.getFirstChildOfType(ASTBinaryExpression.class); if (nestedBinaryExpression != null) { processBinaryExpression(nestedBinaryExpression, data); From 89b3b2f49f32dff27f97a61782a3d0d4a8b669e7 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 26 Mar 2020 11:57:50 +0100 Subject: [PATCH 08/21] [core] Make eclipse jdt happy see https://bugs.eclipse.org/bugs/show_bug.cgi?id=561482 --- .../pmd/lang/ast/internal/IteratorBasedNStream.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/internal/IteratorBasedNStream.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/internal/IteratorBasedNStream.java index 0df1d9c8a9..bfa4ea3dec 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/internal/IteratorBasedNStream.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/internal/IteratorBasedNStream.java @@ -52,8 +52,12 @@ abstract class IteratorBasedNStream implements NodeStream { return mapIter(iter -> IteratorUtil.flatMap(iter, mapper.andThen(IteratorBasedNStream::safeMap))); } - private static @NonNull Iterator safeMap(@Nullable NodeStream ns) { - return ns == null ? Collections.emptyIterator() : ns.iterator(); + // see https://bugs.eclipse.org/bugs/show_bug.cgi?id=561482 + // return type should be Iterator, but that doesn't compile with JDT, if used as a method + // reference in flatMap above. + @SuppressWarnings("unchecked") + private static @NonNull Iterator safeMap(@Nullable NodeStream ns) { + return ns == null ? Collections.emptyIterator() : (Iterator) ns.iterator(); } @Override From 1f8a3cbf0b76a3139c4efd52d0ac2a6e0f6be548 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Fri, 27 Mar 2020 10:20:44 +0100 Subject: [PATCH 09/21] [core] Improve workaround for ejc/jdt see 89b3b2f49f32dff27f97a61782a3d0d4a8b669e7 see https://bugs.eclipse.org/bugs/show_bug.cgi?id=561482 --- .../pmd/lang/ast/internal/IteratorBasedNStream.java | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/internal/IteratorBasedNStream.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/internal/IteratorBasedNStream.java index bfa4ea3dec..339a38d407 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/internal/IteratorBasedNStream.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/internal/IteratorBasedNStream.java @@ -49,15 +49,14 @@ abstract class IteratorBasedNStream implements NodeStream { @Override public NodeStream flatMap(Function> mapper) { - return mapIter(iter -> IteratorUtil.flatMap(iter, mapper.andThen(IteratorBasedNStream::safeMap))); + // Note temporary function is complete typing is needed so that it compiles with ejc + // see https://bugs.eclipse.org/bugs/show_bug.cgi?id=561482 + Function> mapped = mapper.andThen(IteratorBasedNStream::safeMap); + return mapIter(iter -> IteratorUtil.flatMap(iter, mapped)); } - // see https://bugs.eclipse.org/bugs/show_bug.cgi?id=561482 - // return type should be Iterator, but that doesn't compile with JDT, if used as a method - // reference in flatMap above. - @SuppressWarnings("unchecked") - private static @NonNull Iterator safeMap(@Nullable NodeStream ns) { - return ns == null ? Collections.emptyIterator() : (Iterator) ns.iterator(); + private static @NonNull Iterator safeMap(@Nullable NodeStream ns) { + return ns == null ? Collections.emptyIterator() : ns.iterator(); } @Override From a0e1e40bcb75762d58bb8da7c7e07d16e9190d6e Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Fri, 27 Mar 2020 12:58:38 +0100 Subject: [PATCH 10/21] [core] saxon rulechain: don't use rule chain for other path expressions --- .../lang/rule/xpath/SaxonXPathRuleQuery.java | 2 +- .../xpath/internal/ExpressionPrinter.java | 6 ++++ .../xpath/internal/RuleChainAnalyzer.java | 10 ++++++ .../pmd/lang/rule/xpath/internal/Visitor.java | 8 +++++ .../pmd/lang/DummyLanguageModule.java | 11 +++++- .../rule/xpath/SaxonXPathRuleQueryTest.java | 35 ++++++++++++++++--- 6 files changed, 65 insertions(+), 7 deletions(-) diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQuery.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQuery.java index 73cf58eb02..3f4178a754 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQuery.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQuery.java @@ -77,7 +77,7 @@ public class SaxonXPathRuleQuery extends AbstractXPathRuleQuery { /** * Representation of an XPath query, created at {@link #initializeXPathExpression()} using {@link #xpath}. */ - private XPathExpression xpathExpression; + XPathExpression xpathExpression; /** * Holds the static context later used to match the variables in the dynamic context in diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/ExpressionPrinter.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/ExpressionPrinter.java index d1cd1d3e45..144b11ddd8 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/ExpressionPrinter.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/ExpressionPrinter.java @@ -13,6 +13,12 @@ import net.sf.saxon.om.Axis; /** * Simple printer for saxon expressions. Might be useful for debugging / during development. + * + *

Example: + * 

+ * ExpressionPrinter printer = new ExpressionPrinter();
+ * printer.visit(query.xpathExpression.getInternalExpression());
+ *
*/ public class ExpressionPrinter extends Visitor { private int depth = 0; diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/RuleChainAnalyzer.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/RuleChainAnalyzer.java index d621878023..69bb522a01 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/RuleChainAnalyzer.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/RuleChainAnalyzer.java @@ -13,6 +13,7 @@ import net.sf.saxon.Configuration; import net.sf.saxon.expr.AxisExpression; import net.sf.saxon.expr.Expression; import net.sf.saxon.expr.FilterExpression; +import net.sf.saxon.expr.LazyExpression; import net.sf.saxon.expr.PathExpression; import net.sf.saxon.expr.RootExpression; import net.sf.saxon.om.Axis; @@ -96,6 +97,15 @@ public class RuleChainAnalyzer extends Visitor { return super.visit(e); } + @Override + public Expression visit(LazyExpression e) { + if (e.getBaseExpression() instanceof PathExpression) { + this.rootElement = null; + return e; + } + return super.visit(e); + } + public static Comparator documentOrderComparator() { return net.sourceforge.pmd.lang.rule.xpath.internal.DocumentSorter.INSTANCE; } diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/Visitor.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/Visitor.java index 088724df7e..47c100880d 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/Visitor.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/Visitor.java @@ -7,6 +7,7 @@ package net.sourceforge.pmd.lang.rule.xpath.internal; import net.sf.saxon.expr.AxisExpression; import net.sf.saxon.expr.Expression; import net.sf.saxon.expr.FilterExpression; +import net.sf.saxon.expr.LazyExpression; import net.sf.saxon.expr.LetExpression; import net.sf.saxon.expr.PathExpression; import net.sf.saxon.expr.QuantifiedExpression; @@ -63,6 +64,11 @@ abstract class Visitor { return result; } + public Expression visit(LazyExpression e) { + Expression base = visit(e.getBaseExpression()); + return LazyExpression.makeLazyExpression(base); + } + public Expression visit(Expression expr) { Expression result; if (expr instanceof DocumentSorter) { @@ -81,6 +87,8 @@ abstract class Visitor { result = visit((QuantifiedExpression) expr); } else if (expr instanceof LetExpression) { result = visit((LetExpression) expr); + } else if (expr instanceof LazyExpression) { + result = visit((LazyExpression) expr); } else { result = expr; } diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/lang/DummyLanguageModule.java b/pmd-core/src/test/java/net/sourceforge/pmd/lang/DummyLanguageModule.java index 7176915c8f..f956cbf85e 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/lang/DummyLanguageModule.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/lang/DummyLanguageModule.java @@ -19,11 +19,13 @@ import net.sourceforge.pmd.RuleViolation; import net.sourceforge.pmd.lang.ast.DummyNode; import net.sourceforge.pmd.lang.ast.Node; import net.sourceforge.pmd.lang.ast.ParseException; +import net.sourceforge.pmd.lang.ast.xpath.AbstractASTXPathHandler; import net.sourceforge.pmd.lang.ast.xpath.DocumentNavigator; import net.sourceforge.pmd.lang.rule.AbstractRuleChainVisitor; import net.sourceforge.pmd.lang.rule.AbstractRuleViolationFactory; import net.sourceforge.pmd.lang.rule.ParametricRuleViolation; +import net.sf.saxon.expr.XPathContext; import net.sf.saxon.sxpath.IndependentContext; /** @@ -67,11 +69,18 @@ public class DummyLanguageModule extends BaseLanguageModule { } public static class Handler extends AbstractLanguageVersionHandler { + public static class TestFunctions { + public static boolean typeIs(final XPathContext context, final String fullTypeName) { + return false; + } + } + @Override public XPathHandler getXPathHandler() { - return new XPathHandler() { + return new AbstractASTXPathHandler() { @Override public void initialize(IndependentContext context) { + super.initialize(context, LanguageRegistry.getLanguage(DummyLanguageModule.NAME), TestFunctions.class); } @Override diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQueryTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQueryTest.java index c2c48be729..b27861afe7 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQueryTest.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQueryTest.java @@ -18,7 +18,6 @@ import net.sourceforge.pmd.properties.PropertyDescriptor; import net.sourceforge.pmd.properties.PropertyFactory; import net.sf.saxon.expr.Expression; -import net.sf.saxon.expr.XPathContext; public class SaxonXPathRuleQueryTest { @@ -64,10 +63,36 @@ public class SaxonXPathRuleQueryTest { assertExpression("((((/)/descendant::element(dummyNode, xs:anyType))[QuantifiedExpression(Atomizer(attribute::attribute(Test2, xs:anyAtomicType)), ($qq:qq1741979653 singleton eq true()))])[QuantifiedExpression(Atomizer(attribute::attribute(Test1, xs:anyAtomicType)), ($qq:qq1529060733 singleton eq false()))])", query.nodeNameToXPaths.get(SaxonXPathRuleQuery.AST_ROOT).get(0)); } - public static class TestFunctions { - public static boolean typeIs(final XPathContext context, final String fullTypeName) { - return false; - } + @Test + public void ruleChainVisitsCustomFunctions() { + SaxonXPathRuleQuery query = createQuery("//dummyNode[pmd-dummy:typeIs(@Image)]"); + List ruleChainVisits = query.getRuleChainVisits(); + Assert.assertEquals(1, ruleChainVisits.size()); + Assert.assertTrue(ruleChainVisits.contains("dummyNode")); + Assert.assertEquals(2, query.nodeNameToXPaths.size()); + System.out.println(query.nodeNameToXPaths); + assertExpression("(self::node()[pmd-dummy:typeIs(CardinalityChecker(ItemChecker(UntypedAtomicConverter(Atomizer(attribute::attribute(Image, xs:anyAtomicType))))))])", query.nodeNameToXPaths.get("dummyNode").get(0)); + assertExpression("DocumentSorter((((/)/descendant-or-self::node())/(child::element(dummyNode, xs:anyType)[pmd-dummy:typeIs(CardinalityChecker(ItemChecker(UntypedAtomicConverter(Atomizer(attribute::attribute(Image, xs:anyAtomicType))))))])))", query.nodeNameToXPaths.get(SaxonXPathRuleQuery.AST_ROOT).get(0)); + } + + /** + * If a query contains another unbounded path expression other than the first one, it must be + * excluded from rule chain execution. Saxon itself optimizes this quite good already. + */ + @Test + public void ruleChainVisitsUnboundedPathExpressions() { + SaxonXPathRuleQuery query = createQuery("//dummyNode[//ClassOrInterfaceType]"); + List ruleChainVisits = query.getRuleChainVisits(); + Assert.assertEquals(0, ruleChainVisits.size()); + Assert.assertEquals(1, query.nodeNameToXPaths.size()); + assertExpression("LetExpression(LazyExpression(((/)/descendant::element(ClassOrInterfaceType, xs:anyType))), (((/)/descendant::element(dummyNode, xs:anyType))[$zz:zz771775563]))", query.nodeNameToXPaths.get(SaxonXPathRuleQuery.AST_ROOT).get(0)); + + // second sample, more complex + query = createQuery("//dummyNode[ancestor::ClassOrInterfaceDeclaration[//ClassOrInterfaceType]]"); + ruleChainVisits = query.getRuleChainVisits(); + Assert.assertEquals(0, ruleChainVisits.size()); + Assert.assertEquals(1, query.nodeNameToXPaths.size()); + assertExpression("LetExpression(LazyExpression(((/)/descendant::element(ClassOrInterfaceType, xs:anyType))), (((/)/descendant::element(dummyNode, xs:anyType))[(ancestor::element(ClassOrInterfaceDeclaration, xs:anyType)[$zz:zz106374177])]))", query.nodeNameToXPaths.get(SaxonXPathRuleQuery.AST_ROOT).get(0)); } @Test From 8ffe160f12fcef076265e73eb2a3eea899bf78c9 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Fri, 27 Mar 2020 13:35:51 +0100 Subject: [PATCH 11/21] [core] saxon rulechain: consider boolean expr --- .../xpath/internal/RuleChainAnalyzer.java | 21 ++++++++++++------- .../pmd/lang/rule/xpath/internal/Visitor.java | 10 +++++++++ .../rule/xpath/SaxonXPathRuleQueryTest.java | 8 ++++++- 3 files changed, 31 insertions(+), 8 deletions(-) diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/RuleChainAnalyzer.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/RuleChainAnalyzer.java index 69bb522a01..0055c02b0a 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/RuleChainAnalyzer.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/RuleChainAnalyzer.java @@ -38,13 +38,18 @@ public class RuleChainAnalyzer extends Visitor { private final Configuration configuration; private String rootElement; private boolean rootElementReplaced; + private boolean insideLazyExpression; + private boolean foundPathInsideLazy; public RuleChainAnalyzer(Configuration currentConfiguration) { this.configuration = currentConfiguration; } public String getRootElement() { - return rootElement; + if (!foundPathInsideLazy && rootElementReplaced) { + return rootElement; + } + return null; } @Override @@ -56,7 +61,7 @@ public class RuleChainAnalyzer extends Visitor { @Override public Expression visit(PathExpression e) { - if (rootElement == null) { + if (!insideLazyExpression && rootElement == null) { Expression result = super.visit(e); if (rootElement != null && !rootElementReplaced) { if (result instanceof PathExpression) { @@ -80,6 +85,9 @@ public class RuleChainAnalyzer extends Visitor { } return result; } else { + if (insideLazyExpression) { + foundPathInsideLazy = true; + } return super.visit(e); } } @@ -99,11 +107,10 @@ public class RuleChainAnalyzer extends Visitor { @Override public Expression visit(LazyExpression e) { - if (e.getBaseExpression() instanceof PathExpression) { - this.rootElement = null; - return e; - } - return super.visit(e); + insideLazyExpression = true; + Expression result = super.visit(e); + insideLazyExpression = false; + return result; } public static Comparator documentOrderComparator() { diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/Visitor.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/Visitor.java index 47c100880d..d67c34b3b2 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/Visitor.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/Visitor.java @@ -5,6 +5,7 @@ package net.sourceforge.pmd.lang.rule.xpath.internal; import net.sf.saxon.expr.AxisExpression; +import net.sf.saxon.expr.BooleanExpression; import net.sf.saxon.expr.Expression; import net.sf.saxon.expr.FilterExpression; import net.sf.saxon.expr.LazyExpression; @@ -69,6 +70,13 @@ abstract class Visitor { return LazyExpression.makeLazyExpression(base); } + public Expression visit(BooleanExpression e) { + final Expression[] operands = e.getOperands(); + Expression operand0 = visit(operands[0]); + Expression operand1 = visit(operands[1]); + return new BooleanExpression(operand0, e.getOperator(), operand1); + } + public Expression visit(Expression expr) { Expression result; if (expr instanceof DocumentSorter) { @@ -89,6 +97,8 @@ abstract class Visitor { result = visit((LetExpression) expr); } else if (expr instanceof LazyExpression) { result = visit((LazyExpression) expr); + } else if (expr instanceof BooleanExpression) { + result = visit((BooleanExpression) expr); } else { result = expr; } diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQueryTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQueryTest.java index b27861afe7..93b7d4e3a3 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQueryTest.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQueryTest.java @@ -70,7 +70,6 @@ public class SaxonXPathRuleQueryTest { Assert.assertEquals(1, ruleChainVisits.size()); Assert.assertTrue(ruleChainVisits.contains("dummyNode")); Assert.assertEquals(2, query.nodeNameToXPaths.size()); - System.out.println(query.nodeNameToXPaths); assertExpression("(self::node()[pmd-dummy:typeIs(CardinalityChecker(ItemChecker(UntypedAtomicConverter(Atomizer(attribute::attribute(Image, xs:anyAtomicType))))))])", query.nodeNameToXPaths.get("dummyNode").get(0)); assertExpression("DocumentSorter((((/)/descendant-or-self::node())/(child::element(dummyNode, xs:anyType)[pmd-dummy:typeIs(CardinalityChecker(ItemChecker(UntypedAtomicConverter(Atomizer(attribute::attribute(Image, xs:anyAtomicType))))))])))", query.nodeNameToXPaths.get(SaxonXPathRuleQuery.AST_ROOT).get(0)); } @@ -93,6 +92,13 @@ public class SaxonXPathRuleQueryTest { Assert.assertEquals(0, ruleChainVisits.size()); Assert.assertEquals(1, query.nodeNameToXPaths.size()); assertExpression("LetExpression(LazyExpression(((/)/descendant::element(ClassOrInterfaceType, xs:anyType))), (((/)/descendant::element(dummyNode, xs:anyType))[(ancestor::element(ClassOrInterfaceDeclaration, xs:anyType)[$zz:zz106374177])]))", query.nodeNameToXPaths.get(SaxonXPathRuleQuery.AST_ROOT).get(0)); + + // third example, with boolean expr + query = createQuery("//dummyNode[//ClassOrInterfaceType or //OtherNode]"); + ruleChainVisits = query.getRuleChainVisits(); + Assert.assertEquals(0, ruleChainVisits.size()); + Assert.assertEquals(1, query.nodeNameToXPaths.size()); + assertExpression("LetExpression(LazyExpression((((/)/descendant::element(ClassOrInterfaceType, xs:anyType)) or ((/)/descendant::element(OtherNode, xs:anyType)))), (((/)/descendant::element(dummyNode, xs:anyType))[$zz:zz1364913072]))", query.nodeNameToXPaths.get(SaxonXPathRuleQuery.AST_ROOT).get(0)); } @Test From 22c66fde275ee55c56088213e39a298e6a315a93 Mon Sep 17 00:00:00 2001 From: Gwilym Kuiper Date: Fri, 27 Mar 2020 16:44:36 +0000 Subject: [PATCH 12/21] Add returns to setPhoneNumberIfNotExisting in documentation --- pmd-apex/src/main/resources/category/apex/design.xml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pmd-apex/src/main/resources/category/apex/design.xml b/pmd-apex/src/main/resources/category/apex/design.xml index ffac0783e4..54970eb192 100644 --- a/pmd-apex/src/main/resources/category/apex/design.xml +++ b/pmd-apex/src/main/resources/category/apex/design.xml @@ -121,7 +121,10 @@ public class Foo { if (a.Phone == null) { // +1 a.Phone = phone; update a; + return true; } + + return false; } // Has a cognitive complexity of 5 From 263c6e0d091cc9e0f292b538e51ebdcf83dbc6c6 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Fri, 27 Mar 2020 17:46:07 +0100 Subject: [PATCH 13/21] [core] Apply PR review suggestions, refs #2382 --- .../pmd/lang/rule/xpath/internal/ExpressionPrinter.java | 2 +- .../pmd/lang/rule/xpath/internal/RuleChainAnalyzer.java | 5 +++-- .../xpath/internal/{Visitor.java => SaxonExprVisitor.java} | 2 +- .../pmd/lang/rule/xpath/internal/SplitUnions.java | 2 +- 4 files changed, 6 insertions(+), 5 deletions(-) rename pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/{Visitor.java => SaxonExprVisitor.java} (99%) diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/ExpressionPrinter.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/ExpressionPrinter.java index 144b11ddd8..7719642a03 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/ExpressionPrinter.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/ExpressionPrinter.java @@ -20,7 +20,7 @@ import net.sf.saxon.om.Axis; * printer.visit(query.xpathExpression.getInternalExpression()); * */ -public class ExpressionPrinter extends Visitor { +public class ExpressionPrinter extends SaxonExprVisitor { private int depth = 0; private void print(String s) { diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/RuleChainAnalyzer.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/RuleChainAnalyzer.java index 0055c02b0a..9c9bcdb855 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/RuleChainAnalyzer.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/RuleChainAnalyzer.java @@ -34,7 +34,7 @@ import net.sf.saxon.type.Type; *

DocumentSorter expression is removed. The sorting of the resulting nodes needs to be done * after all (sub)expressions have been executed. */ -public class RuleChainAnalyzer extends Visitor { +public class RuleChainAnalyzer extends SaxonExprVisitor { private final Configuration configuration; private String rootElement; private boolean rootElementReplaced; @@ -107,9 +107,10 @@ public class RuleChainAnalyzer extends Visitor { @Override public Expression visit(LazyExpression e) { + boolean prevCtx = insideLazyExpression; insideLazyExpression = true; Expression result = super.visit(e); - insideLazyExpression = false; + insideLazyExpression = prevCtx; return result; } diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/Visitor.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/SaxonExprVisitor.java similarity index 99% rename from pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/Visitor.java rename to pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/SaxonExprVisitor.java index d67c34b3b2..d1dc8dd873 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/Visitor.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/SaxonExprVisitor.java @@ -16,7 +16,7 @@ import net.sf.saxon.expr.RootExpression; import net.sf.saxon.expr.VennExpression; import net.sf.saxon.sort.DocumentSorter; -abstract class Visitor { +abstract class SaxonExprVisitor { public Expression visit(DocumentSorter e) { Expression base = visit(e.getBaseExpression()); return new DocumentSorter(base); diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/SplitUnions.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/SplitUnions.java index 20120c65ef..7d45b544b3 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/SplitUnions.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/SplitUnions.java @@ -17,7 +17,7 @@ import net.sf.saxon.expr.VennExpression; * *

E.g. "//A | //B | //C" will result in 3 expressions "//A", "//B", and "//C". */ -class SplitUnions extends Visitor { +class SplitUnions extends SaxonExprVisitor { private List expressions = new ArrayList<>(); @Override From 1e286ed28158dee715d1d08cc1d964a69b14b5e0 Mon Sep 17 00:00:00 2001 From: Gwilym Kuiper Date: Fri, 27 Mar 2020 16:46:10 +0000 Subject: [PATCH 14/21] Fixup indentation in apex/bestpractices.xml --- .../resources/category/apex/bestpractices.xml | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/pmd-apex/src/main/resources/category/apex/bestpractices.xml b/pmd-apex/src/main/resources/category/apex/bestpractices.xml index 4c43789af4..9d2913a771 100644 --- a/pmd-apex/src/main/resources/category/apex/bestpractices.xml +++ b/pmd-apex/src/main/resources/category/apex/bestpractices.xml @@ -24,7 +24,7 @@ improves the readability of test output. @@ -106,12 +106,12 @@ Apex unit tests should not use @isTest(seeAllData=true) because it opens up the From 4139fd4dd2bcfee12b707fe78103470bd1171448 Mon Sep 17 00:00:00 2001 From: Gwilym Kuiper Date: Fri, 27 Mar 2020 16:47:18 +0000 Subject: [PATCH 15/21] Integer rather than int --- pmd-apex/src/main/resources/category/apex/design.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pmd-apex/src/main/resources/category/apex/design.xml b/pmd-apex/src/main/resources/category/apex/design.xml index 54970eb192..c06b2c5953 100644 --- a/pmd-apex/src/main/resources/category/apex/design.xml +++ b/pmd-apex/src/main/resources/category/apex/design.xml @@ -192,7 +192,7 @@ same datatype. These situations usually denote the need for new objects to wrap Date: Fri, 27 Mar 2020 16:47:51 +0000 Subject: [PATCH 16/21] Fix typo in method --- pmd-apex/src/main/resources/category/apex/design.xml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pmd-apex/src/main/resources/category/apex/design.xml b/pmd-apex/src/main/resources/category/apex/design.xml index c06b2c5953..987dc9aac3 100644 --- a/pmd-apex/src/main/resources/category/apex/design.xml +++ b/pmd-apex/src/main/resources/category/apex/design.xml @@ -275,8 +275,8 @@ lines of code that are split are counted as one. Date: Fri, 27 Mar 2020 16:48:59 +0000 Subject: [PATCH 17/21] int -> Integer and float -> Double --- pmd-apex/src/main/resources/category/apex/design.xml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pmd-apex/src/main/resources/category/apex/design.xml b/pmd-apex/src/main/resources/category/apex/design.xml index 987dc9aac3..140c816b72 100644 --- a/pmd-apex/src/main/resources/category/apex/design.xml +++ b/pmd-apex/src/main/resources/category/apex/design.xml @@ -385,11 +385,11 @@ city/state/zip fields could park them within a single Address field. Date: Fri, 27 Mar 2020 16:50:15 +0000 Subject: [PATCH 18/21] Fixup whitespace --- .../resources/category/apex/errorprone.xml | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/pmd-apex/src/main/resources/category/apex/errorprone.xml b/pmd-apex/src/main/resources/category/apex/errorprone.xml index c96110f960..2c323d192d 100644 --- a/pmd-apex/src/main/resources/category/apex/errorprone.xml +++ b/pmd-apex/src/main/resources/category/apex/errorprone.xml @@ -72,7 +72,7 @@ Avoid directly accessing Trigger.old and Trigger.new as it can lead to a bug. Tr trigger AccountTrigger on Account (before insert, before update) { Account a = Trigger.new[0]; //Bad: Accessing the trigger array directly is not recommended. - for ( Account a : Trigger.new ){ + for ( Account a : Trigger.new ) { //Good: Iterate through the trigger.new array instead. } } @@ -96,9 +96,9 @@ the logic can dynamically identify the proper data to operate against and not fa public without sharing class Foo { void foo() { //Error - hardcoded the record type id - if(a.RecordTypeId == '012500000009WAr'){ + if (a.RecordTypeId == '012500000009WAr') { //do some logic here..... - } else if(a.RecordTypeId == '0123000000095Km'){ + } else if (a.RecordTypeId == '0123000000095Km') { //do some logic here for a different record type... } } @@ -131,12 +131,12 @@ or reported. @@ -165,11 +165,11 @@ Empty If Statement finds instances where a condition is checked but nothing is d From 954f6b09d5432fc250d4560837521adc7cb7a78d Mon Sep 17 00:00:00 2001 From: Gwilym Kuiper Date: Fri, 27 Mar 2020 16:50:53 +0000 Subject: [PATCH 19/21] int -> Integer in apex/errorprone.xml --- pmd-apex/src/main/resources/category/apex/errorprone.xml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pmd-apex/src/main/resources/category/apex/errorprone.xml b/pmd-apex/src/main/resources/category/apex/errorprone.xml index 2c323d192d..fb4b39c19b 100644 --- a/pmd-apex/src/main/resources/category/apex/errorprone.xml +++ b/pmd-apex/src/main/resources/category/apex/errorprone.xml @@ -199,9 +199,9 @@ Empty block statements serve no purpose and should be removed. Date: Fri, 27 Mar 2020 22:35:06 +0100 Subject: [PATCH 20/21] [doc] Update release notes, refs #2383, fixes #2358 --- docs/pages/release_notes.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 82bdf4d4c2..9a265e1482 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -36,6 +36,7 @@ Note that XPath 1.0 support, the default XPath version, is deprecated since PMD * apex * [#2210](https://github.com/pmd/pmd/issues/2210): \[apex] ApexCRUDViolation: Support WITH SECURITY_ENFORCED + * [#2358](https://github.com/pmd/pmd/issues/2358): \[apex] Invalid Apex in Cognitive Complexity tests ### API Changes @@ -101,6 +102,7 @@ implementations, and their corresponding Parser if it exists (in the same packag * [#2312](https://github.com/pmd/pmd/pull/2312): \[apex] Update ApexCRUDViolation Rule - [Joshua S Arquilevich](https://github.com/jarquile) * [#2314](https://github.com/pmd/pmd/pull/2314): \[doc] maven integration - Add version to plugin - [Pham Hai Trung](https://github.com/gpbp) * [#2353](https://github.com/pmd/pmd/pull/2353): \[plsql] xmlforest with optional AS - [Piotr Szymanski](https://github.com/szyman23) +* [#2383](https://github.com/pmd/pmd/pull/2383): \[apex] Fix invalid apex in documentation - [Gwilym Kuiper](https://github.com/gwilymatgearset) {% endtocmaker %} From e1a4b8ae97e5e2fc0e530bd32297b16b1a1ae8b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Sat, 28 Mar 2020 11:42:16 +0100 Subject: [PATCH 21/21] Forbid type params for record ctors --- pmd-java/etc/grammar/Java.jjt | 3 +-- .../pmd/lang/java/ast/ASTRecordConstructorDeclaration.java | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/pmd-java/etc/grammar/Java.jjt b/pmd-java/etc/grammar/Java.jjt index 80186c015e..2bbda165a6 100644 --- a/pmd-java/etc/grammar/Java.jjt +++ b/pmd-java/etc/grammar/Java.jjt @@ -1160,7 +1160,7 @@ void RecordBodyDeclaration() #void : private void RecordCtorLookahead() #void: {} { - Modifiers() [ TypeParameters() ] ("throws" | "{") + Modifiers() "{" } void RecordConstructorDeclaration(): @@ -1169,7 +1169,6 @@ void RecordConstructorDeclaration(): } { modifiers = Modifiers() { jjtThis.setModifiers(modifiers); } - [TypeParameters()] { jjtThis.setImage(token.image); } Block() } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTRecordConstructorDeclaration.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTRecordConstructorDeclaration.java index f262de9ea9..753f40239d 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTRecordConstructorDeclaration.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTRecordConstructorDeclaration.java @@ -14,7 +14,6 @@ import net.sourceforge.pmd.annotation.Experimental; * * RecordConstructorDeclaration ::= ({@linkplain ASTAnnotation Annotation})* * RecordModifiers - * {@linkplain ASTTypeParameters TypeParameters}? * <IDENTIFIER> * {@link ASTBlock Block} *