From cffb453276f896f900895f76ba190a4861f2e3ee Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Fri, 20 Mar 2020 14:56:41 +0100 Subject: [PATCH 01/56] [doc] Improve documentation about incremental analysis Fixes #2355 --- .../pmd/userdocs/incremental_analysis.md | 80 ++++++++++++++++++- docs/pages/release_notes.md | 2 + 2 files changed, 81 insertions(+), 1 deletion(-) diff --git a/docs/pages/pmd/userdocs/incremental_analysis.md b/docs/pages/pmd/userdocs/incremental_analysis.md index 701fa19da3..4f54ecfb2f 100644 --- a/docs/pages/pmd/userdocs/incremental_analysis.md +++ b/docs/pages/pmd/userdocs/incremental_analysis.md @@ -24,7 +24,7 @@ untouched, files with violations will be listed with full detail. Therefore, its Incremental analysis is enabled automatically once a location to store the cache has been defined. From command-line that is done through the [`-cache`](pmd_userdocs_cli_reference.html#cache) argument, but support for the feature is available for tools integrating PMD such as [Ant](pmd_userdocs_tools_ant.html), -[Maven](pmd_userdocs_tools_maven.html), and Gradle. +[Maven](pmd_userdocs_tools_maven.html), and [Gradle](pmd_userdocs_tools_gradle.html). ### Disabling incremental analysis @@ -32,3 +32,81 @@ available for tools integrating PMD such as [Ant](pmd_userdocs_tools_ant.html), By default, PMD will suggest to use an analysis cache by logging a warning. If you'd like to disable this warning, or ignore the analysis cache for a few runs, you can use the [`-no-cache`](pmd_userdocs_cli_reference.html#no-cache) switch. + + +### FAQ + +#### When is the cache invalidated? + +On the following reasons, the complete cache file is considered invalid: + +* The PMD version differs. Since each PMD version might have fixed some false-positives or false-negatives for rules, + a cache file created with a different version is considered invalid. The version comparison is exact. +* The used ruleset has been changed. If the ruleset is changed in any way (e.g. adding/removing rules, changing + rule properties, ...), the cache is considered invalid. +* The [`auxclasspath`](pmd_userdocs_cli_reference.html#auxclasspath) changed. The auxclasspath is used during + type resolution. A changed auxclasspath can result for rules, that use type resolution, in different + violations. Usually, if the auxclasspath is correct and type resolution works, the rules report less false-positives. + To make sure, the correct violations are reported, the cache is considered invalid, if the auxclasspath has changed. +* The execution classpath has been changed. On the execution classpath not only the PMD classes are located, but also + the implementation of e.g. custom rules. If any jar file/class file on the execution classpath is changed, then + the cache is considered invalid as well. + +#### What is stored in the cache file? + +The cache file consists of a header and a body. The header stores the information which is used to decided +whether the whole cache file is valid or not (see above). The following information is stored: + +* PMD Version +* Ruleset checksum +* Auxclasspath checksum +* Execution classpath checksum + +The body contains an entry for every file that has been analyzed. For every file, the following information +is stored: + +* The full (absolute) pathname of the file +* The checksum of the file itself +* 0 or more rule violations with all the info (line number, etc.) + +You can think of the cache as a Map where the filepath is used as the key +and the violations found in previous runs are the value. + +The cache is in the end just a file with serialized data (binary). The implementation is +{% jdoc core::cache.FileAnalysisCache %}. + +#### How does PMD detect whether a file has been changed? + +When analyzing a file, PMD records the checksum of the file content and stores this +together with the violations in the cache file. When running PMD with the cache file, +PMD looks up the file in the cache and compares the checksums. +If the checksums match, then the file is not even parsed, the rules +are not executed and the violations for this file are entirely used from the cache. +If the checksum doesn't match, then the cached violations are discarded (if there are any) +and the file is fully processed: the file is parsed and all the rules are run for it. +After we are done, the cache is updated with the new violations. + +#### Can I reuse a cache created on branch A for analyzing my project on branch B? + +This is possible. As long as the same PMD version and same ruleset is used on both branches. +Also note, that if the branch uses a different dependencies, the auxclasspath is different on both +classes, which invalidates the cache completely. + +If files have been renamed on the branch, these files will be analyzed again since PMD uses +the file names to assign existing rule violations from the cache. Also, if the full path name +of the file changes, because the other branch is checked out at a different location, then all +the cached files don't match. + +Apart from these restrictions, PMD will only analyze files that changed between runs. +If your previous run was on branch A and then you run on branch B using the same cache file, +it will only look at files that are different between the 2 branches. + +#### Can I reuse a cache file across different machines? + +This is only possible, if the other machine uses the exact same path names. That means that +your project needs to be checked out into the same directory structure. + +Additionally, all the other restrictions apply (same PMD version, same ruleset, same auxclasspath, +same execution classpath). + +See also issue [#2063 [core] Support sharing incremental analysis cache file across different machines](https://github.com/pmd/pmd/issues/2063). diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 2aa1a36bd6..70f480e74d 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -18,6 +18,8 @@ This is a {{ site.pmd.release_type }} release. * apex * [#2210](https://github.com/pmd/pmd/issues/2210): \[apex] ApexCRUDViolation: Support WITH SECURITY_ENFORCED +* core + * [#2355](https://github.com/pmd/pmd/issues/2355): \[doc] Improve documentation about incremental analysis ### API Changes From 30886544c53df5c9bd99a5e6fd70a2ce4609c2e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Fri, 20 Mar 2020 18:00:16 +0100 Subject: [PATCH 02/56] Deprecations to clean up CPD --- .../src/main/java/net/sourceforge/pmd/lang/Parser.java | 2 ++ .../main/java/net/sourceforge/pmd/lang/TokenManager.java | 5 +++++ .../sourceforge/pmd/lang/ast/AbstractTokenManager.java | 8 ++++++++ 3 files changed, 15 insertions(+) diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/Parser.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/Parser.java index f4d7e4e280..7ece070e1a 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/Parser.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/Parser.java @@ -29,7 +29,9 @@ public interface Parser { * @param source * Reader that provides the source code to tokenize. * @return A TokenManager for reading token. + * @deprecated For removal in 7.0.0 */ + @Deprecated TokenManager getTokenManager(String fileName, Reader source); diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/TokenManager.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/TokenManager.java index e0e67c78cc..edb26311d1 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/TokenManager.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/TokenManager.java @@ -11,5 +11,10 @@ public interface TokenManager { // TODO : Change the return to GenericToken in 7.0.0 - maybe even use generics TokenManager Object getNextToken(); + + /** + * @deprecated For removal in 7.0.0 + */ + @Deprecated void setFileName(String fileName); } diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/AbstractTokenManager.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/AbstractTokenManager.java index 96c8e60c3d..c40f925ab0 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/AbstractTokenManager.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/AbstractTokenManager.java @@ -19,10 +19,18 @@ public abstract class AbstractTokenManager { protected Map suppressMap = new HashMap<>(); protected String suppressMarker = PMD.SUPPRESS_MARKER; + /** + * @deprecated For removal in 7.0.0 + */ + @Deprecated public static void setFileName(String fileName) { AbstractTokenManager.fileName.set(fileName); } + /** + * @deprecated For removal in 7.0.0 + */ + @Deprecated public static String getFileName() { String fileName = AbstractTokenManager.fileName.get(); return fileName == null ? "(no file name provided)" : fileName; From 153d669d02ebf41d15a0f2163af0d63bfdd650f1 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Sat, 21 Mar 2020 09:35:30 +0100 Subject: [PATCH 03/56] [doc] Add example with Maven dependency management --- docs/pages/pmd/userdocs/incremental_analysis.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/pages/pmd/userdocs/incremental_analysis.md b/docs/pages/pmd/userdocs/incremental_analysis.md index 4f54ecfb2f..5ba251e901 100644 --- a/docs/pages/pmd/userdocs/incremental_analysis.md +++ b/docs/pages/pmd/userdocs/incremental_analysis.md @@ -90,7 +90,9 @@ After we are done, the cache is updated with the new violations. This is possible. As long as the same PMD version and same ruleset is used on both branches. Also note, that if the branch uses a different dependencies, the auxclasspath is different on both -classes, which invalidates the cache completely. +classes, which invalidates the cache completely. If you project uses e.g. Maven for dependency +management and your branch uses different dependencies (either different version or completely different +artifacts), then the auxclasspath is changed. If files have been renamed on the branch, these files will be analyzed again since PMD uses the file names to assign existing rule violations from the cache. Also, if the full path name From 16162a113532a75a6c5d27519e0ebf5e9a71952f Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Sat, 21 Mar 2020 11:24:42 +0100 Subject: [PATCH 04/56] [doc] Add missing doc about pmd.github.io Fixes #2356 --- docs/_data/sidebars/pmd_sidebar.yml | 3 + .../committers/main_landing_page.md | 85 +++++++++++++++++++ .../pmd/projectdocs/committers/releasing.md | 2 +- docs/pages/release_notes.md | 2 + 4 files changed, 91 insertions(+), 1 deletion(-) create mode 100644 docs/pages/pmd/projectdocs/committers/main_landing_page.md diff --git a/docs/_data/sidebars/pmd_sidebar.yml b/docs/_data/sidebars/pmd_sidebar.yml index b2c7740c6c..0d6145cd6c 100644 --- a/docs/_data/sidebars/pmd_sidebar.yml +++ b/docs/_data/sidebars/pmd_sidebar.yml @@ -430,4 +430,7 @@ entries: - title: Merging pull requests url: /pmd_projectdocs_committers_merging_pull_requests.html output: web, pdf + - title: Main Landing page + url: /pmd_projectdocs_committers_main_landing_page.html + output: web, pdf diff --git a/docs/pages/pmd/projectdocs/committers/main_landing_page.md b/docs/pages/pmd/projectdocs/committers/main_landing_page.md new file mode 100644 index 0000000000..a0f2515068 --- /dev/null +++ b/docs/pages/pmd/projectdocs/committers/main_landing_page.md @@ -0,0 +1,85 @@ +--- +title: Main Landing Page +permalink: pmd_projectdocs_committers_main_landing_page.html +last_updated: March 2020 +author: Andreas Dangel +--- + +The main homepage of PMD is hosted by Github Pages. + +The repository is . + +It uses [Jekyll](https://jekyllrb.com/) to generate the static html pages. Jekyll is +executed by github for every push to the repository. Please note, that it takes some time +until Jekyll has been executed and due to caching, the homepage is not updated immediately. +It usually takes 15 minutes. + + +## Contents + +* Main page - aka "Landing page": + * Layout: [_layouts/default.html](https://github.com/pmd/pmd.github.io/blob/master/_layouts/default.html). + It includes all the sub section, which can be found in the includes directory [_includes/](https://github.com/pmd/pmd.github.io/tree/master/_includes) + * The latest PMD version is configured in `_config.yml` and the variables `site.pmd.latestVersion` are used + e.g. in [_includes/home.html](https://github.com/pmd/pmd.github.io/blob/master/_includes/home.html). +* Blog - aka "News": + * This is a section on main page. It shows the 5 latest news. See [_includes/news.html](https://github.com/pmd/pmd.github.io/blob/master/_includes/news.html). + * There is also a sub page "news" which lists all news. + * Layout: [_layouts/news.html](https://github.com/pmd/pmd.github.io/blob/master/_layouts/news.html) + * Page (which is pretty empty): [news.html](https://github.com/pmd/pmd.github.io/blob/master/news.html) +* Documentation for the latest release: + * The PMD documentation of the latest release is simply copied as static html into the folder [latest/](https://github.com/pmd/pmd.github.io/tree/master/latest). + This makes the latest release documentation available under the stable URL + . This URL is also used for the [sitemap.xml](https://github.com/pmd/pmd.github.io/blob/master/sitemap.xml). +* Documentation for previous releases are still being kept under the folders `pmd-/`. + + +## Building the page locally + +Since the repository contains the documentation for many old PMD releases, it is quite big. When executing +Jekyll to generate the site, it copies all the files to the folder `_site/` - and this can take a while. + +In order to speed things up locally, consider to add `pmd-*` to the exclude patterns in `_config.yml`. See +also the comments in this file. + +Then it is a matter of simply executing `bundle exec jekyll serve`. This will generate the site and host +it on localhost, so you can test the page at . + + +## Updates during a release + +When creating a new PMD release, some content of the main page need to be updated as well. +This done as part of the [Release process](pmd_projectdocs_committers_releasing.html), but is +summarized here as well: + +* The versions (e.g. `pmd.latestVersion`) needs to be updated in `_config.yml` + * This is needed to generate the correct links and texts for the latest version on landing page +* The new PMD documentation needs to be copied to `/pmd-/` +* Then this folder needs to copied to `/latest/`, actually replacing the old version. +* A new blog post with release notes is added: `/_posts/YYYY-mm-dd-PMD-.md` +* The sitemap `sitemap.xml` is regenerated + +Some of these steps are automated through `do-release.sh` (like blog post), some are manual steps +(updating the version in _config.yml) and other steps are done on the travis-ci-build (like +copying the new documentation). + +## Adding a new blog post + +Adding a new blog post is as easy as: + +* Creating a new file in the folder "_posts": `/_posts/YYYY-mm-dd-.md` +* The file name needs to fit this pattern. The date of the blog post is taken from the file name. The "<title>" + is used for the url. +* The file is a markdown file starting with a frontmatter for jekyll. Just use this template for the new file: + +``` +--- +layout: post +title: Title +--- + +Here comes the text +``` + +Once you commit and push it, Github will run Jekyll and update the page. The Jekyll templates take care that +the new post is recognized and added to the news section and also on the news subpage. diff --git a/docs/pages/pmd/projectdocs/committers/releasing.md b/docs/pages/pmd/projectdocs/committers/releasing.md index 16a64bc112..0951ac7e16 100644 --- a/docs/pages/pmd/projectdocs/committers/releasing.md +++ b/docs/pages/pmd/projectdocs/committers/releasing.md @@ -1,5 +1,5 @@ --- -title: Releasing +title: Release process permalink: pmd_projectdocs_committers_releasing.html author: Romain Pelisse <rpelisse@users.sourceforge.net>, Andreas Dangel <adangel@users.sourceforge.net> --- diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index bb755f4f26..5a2003419c 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -34,6 +34,8 @@ Note that XPath 1.0 support, the default XPath version, is deprecated since PMD ### Fixed Issues +* core + * [#2356](https://github.com/pmd/pmd/issues/2356): \[doc] Add missing doc about pmd.github.io * apex * [#2210](https://github.com/pmd/pmd/issues/2210): \[apex] ApexCRUDViolation: Support WITH SECURITY_ENFORCED From 96532f26cccb6cd2d9f2d878e7bc168556477d60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= <clement.fournier76@gmail.com> Date: Sun, 22 Mar 2020 21:52:21 +0100 Subject: [PATCH 05/56] Make typeIs XPath functions use the typeres cache --- .../pmd/lang/java/ast/ASTCompilationUnit.java | 5 + .../pmd/lang/java/ast/AbstractJavaNode.java | 9 ++ .../pmd/lang/java/ast/JavaNode.java | 1 + .../rule/codestyle/DuplicateImportsRule.java | 2 +- .../typeresolution/ClassTypeResolver.java | 14 ++- .../lang/java/typeresolution/TypeHelper.java | 110 ++++++++++++++---- .../internal/NullableClassLoader.java | 29 +++++ 7 files changed, 143 insertions(+), 27 deletions(-) create mode 100644 pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/internal/NullableClassLoader.java diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTCompilationUnit.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTCompilationUnit.java index 5fbd67f90d..d8f2f532a1 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTCompilationUnit.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTCompilationUnit.java @@ -60,6 +60,11 @@ public class ASTCompilationUnit extends AbstractJavaTypeNode implements RootNode return null; } + @Override + public ASTCompilationUnit getRoot() { + return this; + } + /** * Returns the package name of this compilation unit. If this is in * the default package, returns the empty string. diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/AbstractJavaNode.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/AbstractJavaNode.java index 413c5d53b0..92027f5095 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/AbstractJavaNode.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/AbstractJavaNode.java @@ -15,6 +15,7 @@ public abstract class AbstractJavaNode extends AbstractJjtreeNode<JavaNode> impl protected JavaParser parser; private Scope scope; private Comment comment; + private ASTCompilationUnit root; @InternalApi @Deprecated @@ -64,6 +65,14 @@ public abstract class AbstractJavaNode extends AbstractJjtreeNode<JavaNode> impl return data; } + @Override + public ASTCompilationUnit getRoot() { + if (root == null) { + root = getParent().getRoot(); + } + return root; + } + @Override public Scope getScope() { if (scope == null) { diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/JavaNode.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/JavaNode.java index b8310ea936..443c956810 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/JavaNode.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/JavaNode.java @@ -41,6 +41,7 @@ public interface JavaNode extends ScopedNode { @Deprecated Object childrenAccept(JavaParserVisitor visitor, Object data); + ASTCompilationUnit getRoot(); @Override JavaNode getChild(int index); diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/DuplicateImportsRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/DuplicateImportsRule.java index 655cd8c600..ee26f49b20 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/DuplicateImportsRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/DuplicateImportsRule.java @@ -66,7 +66,7 @@ public class DuplicateImportsRule extends AbstractJavaRule { return true; } } else { - Class<?> importClass = node.getClassTypeResolver().loadClass(thisImportOnDemand.getName()); + Class<?> importClass = node.getClassTypeResolver().loadClassOrNull(thisImportOnDemand.getName()); if (importClass != null) { for (Method m : importClass.getMethods()) { if (Modifier.isStatic(m.getModifiers()) && m.getName().equals(singleTypeName)) { diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/ClassTypeResolver.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/ClassTypeResolver.java index fb57427a60..98a1858c85 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/ClassTypeResolver.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/ClassTypeResolver.java @@ -99,6 +99,7 @@ import net.sourceforge.pmd.lang.java.ast.JavaParserVisitorAdapter; import net.sourceforge.pmd.lang.java.ast.TypeNode; import net.sourceforge.pmd.lang.java.symboltable.ClassScope; import net.sourceforge.pmd.lang.java.symboltable.VariableNameDeclaration; +import net.sourceforge.pmd.lang.java.typeresolution.internal.NullableClassLoader; import net.sourceforge.pmd.lang.java.typeresolution.typedefinition.JavaTypeDefinition; import net.sourceforge.pmd.lang.symboltable.NameOccurrence; import net.sourceforge.pmd.lang.symboltable.Scope; @@ -112,7 +113,7 @@ import net.sourceforge.pmd.lang.symboltable.Scope; @Deprecated @InternalApi -public class ClassTypeResolver extends JavaParserVisitorAdapter { +public class ClassTypeResolver extends JavaParserVisitorAdapter implements NullableClassLoader { private static final Logger LOG = Logger.getLogger(ClassTypeResolver.class.getName()); @@ -1487,10 +1488,15 @@ public class ClassTypeResolver extends JavaParserVisitorAdapter { return pmdClassLoader.loadClassOrNull(fullyQualifiedClassName) != null; } - public Class<?> loadClass(String fullyQualifiedClassName) { + @Override + public Class<?> loadClassOrNull(String fullyQualifiedClassName) { return pmdClassLoader.loadClassOrNull(fullyQualifiedClassName); } + public Class<?> loadClass(String fullyQualifiedClassName) { + return loadClassOrNull(fullyQualifiedClassName); + } + private Class<?> processOnDemand(String qualifiedName) { for (String entry : importedOnDemand) { String fullClassName = entry + "." + qualifiedName; @@ -1533,12 +1539,12 @@ public class ClassTypeResolver extends JavaParserVisitorAdapter { String strPackage = anImportDeclaration.getPackageName(); if (anImportDeclaration.isStatic()) { if (anImportDeclaration.isImportOnDemand()) { - importOnDemandStaticClasses.add(JavaTypeDefinition.forClass(loadClass(strPackage))); + importOnDemandStaticClasses.add(JavaTypeDefinition.forClass(loadClassOrNull(strPackage))); } else { // not import on-demand String strName = anImportDeclaration.getImportedName(); String fieldName = strName.substring(strName.lastIndexOf('.') + 1); - Class<?> staticClassWithField = loadClass(strPackage); + Class<?> staticClassWithField = loadClassOrNull(strPackage); if (staticClassWithField != null) { JavaTypeDefinition typeDef = getFieldType(JavaTypeDefinition.forClass(staticClassWithField), fieldName, currentAcu.getType()); diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java index aa76317c80..653e3e352f 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java @@ -4,7 +4,12 @@ package net.sourceforge.pmd.lang.java.typeresolution; -import org.apache.commons.lang3.ClassUtils; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; + +import org.apache.commons.lang3.StringUtils; +import org.apache.commons.lang3.Validate; import net.sourceforge.pmd.lang.java.ast.ASTAnnotationTypeDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration; @@ -13,9 +18,48 @@ import net.sourceforge.pmd.lang.java.ast.ASTEnumDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTImplementsList; import net.sourceforge.pmd.lang.java.ast.TypeNode; import net.sourceforge.pmd.lang.java.symboltable.TypedNameDeclaration; +import net.sourceforge.pmd.lang.java.typeresolution.internal.NullableClassLoader; +import net.sourceforge.pmd.lang.java.typeresolution.internal.NullableClassLoader.ClassLoaderWrapper; public final class TypeHelper { + + /** Maps a primitive class name to its corresponding abbreviation used in array class names. */ + private static final Map<String, String> abbreviationMap; + /** Maps names of primitives to their corresponding primitive {@code Class}es. */ + private static final Map<String, Class<?>> namePrimitiveMap = new HashMap<>(); + + + static { + namePrimitiveMap.put("boolean", Boolean.TYPE); + namePrimitiveMap.put("byte", Byte.TYPE); + namePrimitiveMap.put("char", Character.TYPE); + namePrimitiveMap.put("short", Short.TYPE); + namePrimitiveMap.put("int", Integer.TYPE); + namePrimitiveMap.put("long", Long.TYPE); + namePrimitiveMap.put("double", Double.TYPE); + namePrimitiveMap.put("float", Float.TYPE); + namePrimitiveMap.put("void", Void.TYPE); + } + + static { + final Map<String, String> m = new HashMap<>(); + m.put("int", "I"); + m.put("boolean", "Z"); + m.put("float", "F"); + m.put("long", "J"); + m.put("short", "S"); + m.put("byte", "B"); + m.put("double", "D"); + m.put("char", "C"); + final Map<String, String> r = new HashMap<>(); + for (final Map.Entry<String, String> e : m.entrySet()) { + r.put(e.getValue(), e.getKey()); + } + abbreviationMap = Collections.unmodifiableMap(m); + } + + private TypeHelper() { // utility class } @@ -101,34 +145,56 @@ public final class TypeHelper { private static Class<?> loadClassWithNodeClassloader(final TypeNode n, final String clazzName) { if (n.getType() != null) { - return loadClass(n.getType().getClassLoader(), clazzName); + return loadClass(n.getRoot().getClassTypeResolver(), clazzName); } return null; } - private static Class<?> loadClass(final ClassLoader nullableClassLoader, final String clazzName) { - try { - ClassLoader classLoader = nullableClassLoader; - if (classLoader == null) { - // Using the system classloader then - classLoader = ClassLoader.getSystemClassLoader(); + private static Class<?> loadClass(NullableClassLoader ctr, String className) { + Class<?> clazz; + if (namePrimitiveMap.containsKey(className)) { + clazz = namePrimitiveMap.get(className); + } else { + clazz = ctr.loadClassOrNull(toCanonicalName(className)); + } + if (clazz != null) { + return clazz; + } + + // allow path separators (.) as inner class name separators + final int lastDotIndex = className.lastIndexOf('.'); + + if (lastDotIndex != -1) { + String asInner = className.substring(0, lastDotIndex) + + '$' + className.substring(lastDotIndex + 1); + return loadClass(ctr, asInner); + } + return null; + } + + + private static String toCanonicalName(String className) { + className = StringUtils.deleteWhitespace(className); + Validate.notNull(className, "className must not be null."); + if (className.endsWith("[]")) { + final StringBuilder classNameBuffer = new StringBuilder(); + while (className.endsWith("[]")) { + className = className.substring(0, className.length() - 2); + classNameBuffer.append("["); } - - // If the requested type is in the classpath, using the same classloader should work - return ClassUtils.getClass(classLoader, clazzName); - } catch (ClassNotFoundException ignored) { - // The requested type is not on the auxclasspath. This might happen, if the type node - // is probed for a specific type (e.g. is is a JUnit5 Test Annotation class). - // Failing to resolve clazzName does not necessarily indicate an incomplete auxclasspath. - } catch (final LinkageError expected) { - // We found the class but it's invalid / incomplete. This may be an incomplete auxclasspath - // if it was a NoClassDefFoundError. TODO : Report it? + final String abbreviation = abbreviationMap.get(className); + if (abbreviation != null) { + classNameBuffer.append(abbreviation); + } else { + classNameBuffer.append("L").append(className).append(";"); + } + className = classNameBuffer.toString(); } - - return null; + return className; } + /** @see #isA(TypeNode, String) */ public static boolean isA(TypeNode n, Class<?> clazz) { return subclasses(n, clazz); @@ -142,7 +208,7 @@ public final class TypeHelper { Class<?> type = vnd.getType(); for (final Class<?> clazz : clazzes) { if (type != null && type.equals(clazz) || type == null - && (clazz.getSimpleName().equals(vnd.getTypeImage()) || clazz.getName().equals(vnd.getTypeImage()))) { + && (clazz.getSimpleName().equals(vnd.getTypeImage()) || clazz.getName().equals(vnd.getTypeImage()))) { return true; } } @@ -192,7 +258,7 @@ public final class TypeHelper { public static boolean isA(TypedNameDeclaration vnd, String className) { Class<?> type = vnd.getType(); if (type != null) { - Class<?> clazz = loadClass(type.getClassLoader(), className); + Class<?> clazz = loadClass(new ClassLoaderWrapper(type.getClassLoader()), className); if (clazz != null) { return clazz.isAssignableFrom(type); } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/internal/NullableClassLoader.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/internal/NullableClassLoader.java new file mode 100644 index 0000000000..d298ab6e54 --- /dev/null +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/internal/NullableClassLoader.java @@ -0,0 +1,29 @@ +/* + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.java.typeresolution.internal; + +public interface NullableClassLoader { + + Class<?> loadClassOrNull(String binaryName); + + + class ClassLoaderWrapper implements NullableClassLoader { + + private final ClassLoader classLoader; + + public ClassLoaderWrapper(ClassLoader classLoader) { + this.classLoader = classLoader; + } + + @Override + public Class<?> loadClassOrNull(String binaryName) { + try { + return classLoader.loadClass(binaryName); + } catch (ClassNotFoundException e) { + return null; + } + } + } +} From a2994a97d93bab6a7e54df3bc05c562e14483cbb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= <clement.fournier76@gmail.com> Date: Mon, 23 Mar 2020 20:57:51 +0100 Subject: [PATCH 06/56] Fast path annotation types --- .../lang/java/typeresolution/TypeHelper.java | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java index 653e3e352f..78927e32c9 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java @@ -78,6 +78,10 @@ public final class TypeHelper { * @return <code>true</code> if type node n is of type clazzName or a subtype of clazzName */ public static boolean isA(final TypeNode n, final String clazzName) { + if (n.getType() != null && n.getType().isAnnotation()) { + return isAnnotationSubtype(n, clazzName); + } + final Class<?> clazz = loadClassWithNodeClassloader(n, clazzName); if (clazz != null || n.getType() != null) { @@ -87,6 +91,15 @@ public final class TypeHelper { return fallbackIsA(n, clazzName); } + private static boolean isAnnotationSubtype(TypeNode n, String clazzName) { + // then, the supertype may only be Object, j.l.Annotation, or the class name + // this avoids classloading altogether + // this is used e.g. by the typeIs function in XPath + return clazzName.equals("java.lang.annotation.Annotation") + || clazzName.equals("java.lang.Object") + || clazzName.equals(n.getType().getName()); + } + private static boolean fallbackIsA(TypeNode n, String clazzName) { if (clazzName.equals(n.getImage()) || clazzName.endsWith("." + n.getImage())) { return true; @@ -134,6 +147,11 @@ public final class TypeHelper { * @return <code>true</code> if type node n is exactly of type clazzName. */ public static boolean isExactlyA(final TypeNode n, final String clazzName) { + if (n.getType() != null && n.getType().getName().equals(clazzName)) { + // fast path avoiding classloading + return true; + } + final Class<?> clazz = loadClassWithNodeClassloader(n, clazzName); if (clazz != null) { From 633319015bf9c04480275507a00971cd062ed087 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= <clement.fournier76@gmail.com> Date: Mon, 23 Mar 2020 21:04:34 +0100 Subject: [PATCH 07/56] Fix when the type is nullable --- .../pmd/lang/java/typeresolution/TypeHelper.java | 2 +- .../typeresolution/internal/NullableClassLoader.java | 10 +++++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java index 78927e32c9..f0e20632b4 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java @@ -276,7 +276,7 @@ public final class TypeHelper { public static boolean isA(TypedNameDeclaration vnd, String className) { Class<?> type = vnd.getType(); if (type != null) { - Class<?> clazz = loadClass(new ClassLoaderWrapper(type.getClassLoader()), className); + Class<?> clazz = loadClass(ClassLoaderWrapper.wrapNullable(type.getClassLoader()), className); if (clazz != null) { return clazz.isAssignableFrom(type); } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/internal/NullableClassLoader.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/internal/NullableClassLoader.java index d298ab6e54..57df26c7ee 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/internal/NullableClassLoader.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/internal/NullableClassLoader.java @@ -13,7 +13,8 @@ public interface NullableClassLoader { private final ClassLoader classLoader; - public ClassLoaderWrapper(ClassLoader classLoader) { + private ClassLoaderWrapper(ClassLoader classLoader) { + assert classLoader != null : "Null classloader"; this.classLoader = classLoader; } @@ -25,5 +26,12 @@ public interface NullableClassLoader { return null; } } + + public static ClassLoaderWrapper wrapNullable(ClassLoader classLoader) { + if (classLoader == null) { + classLoader = ClassLoader.getSystemClassLoader(); + } + return new ClassLoaderWrapper(classLoader); + } } } From 549a49790cf8a3bc897335205b1b2f15c05e3301 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= <clement.fournier76@gmail.com> Date: Mon, 23 Mar 2020 21:44:10 +0100 Subject: [PATCH 08/56] Doc --- .../lang/java/typeresolution/PMDASMClassLoader.java | 4 +++- .../typeresolution/internal/NullableClassLoader.java | 12 ++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/PMDASMClassLoader.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/PMDASMClassLoader.java index 553a8ed672..40cf772f62 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/PMDASMClassLoader.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/PMDASMClassLoader.java @@ -15,6 +15,7 @@ import java.util.concurrent.ConcurrentMap; import org.objectweb.asm.ClassReader; import net.sourceforge.pmd.annotation.InternalApi; +import net.sourceforge.pmd.lang.java.typeresolution.internal.NullableClassLoader; import net.sourceforge.pmd.lang.java.typeresolution.visitors.PMDASMVisitor; /* @@ -36,7 +37,7 @@ import net.sourceforge.pmd.lang.java.typeresolution.visitors.PMDASMVisitor; */ @InternalApi @Deprecated -public final class PMDASMClassLoader extends ClassLoader { +public final class PMDASMClassLoader extends ClassLoader implements NullableClassLoader { private static PMDASMClassLoader cachedPMDASMClassLoader; private static ClassLoader cachedClassLoader; @@ -81,6 +82,7 @@ public final class PMDASMClassLoader extends ClassLoader { * Not throwing CNFEs to represent failure makes a huge performance * difference. Typeres as a whole is 2x faster. */ + @Override public Class<?> loadClassOrNull(String name) { if (dontBother.containsKey(name)) { return null; diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/internal/NullableClassLoader.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/internal/NullableClassLoader.java index 57df26c7ee..b7ba7d8b89 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/internal/NullableClassLoader.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/internal/NullableClassLoader.java @@ -4,8 +4,20 @@ package net.sourceforge.pmd.lang.java.typeresolution.internal; +import net.sourceforge.pmd.lang.java.typeresolution.PMDASMClassLoader; + +/** + * A classloader that doesn't throw a {@link ClassNotFoundException} + * to report unresolved classes. This is a big performance improvement + * for {@link PMDASMClassLoader}, which caches negative cases. + * + * <p>See https://github.com/pmd/pmd/pull/2236 + */ public interface NullableClassLoader { + /** + * Load a class from its binary name. Returns null if not found. + */ Class<?> loadClassOrNull(String binaryName); From acbecf3e65d11c3fd4f905ba9375c86a7b68752c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= <clement.fournier76@gmail.com> Date: Mon, 23 Mar 2020 22:05:29 +0100 Subject: [PATCH 09/56] Add tests --- .../lang/java/typeresolution/TypeHelper.java | 6 +++-- .../java/typeresolution/TypeHelperTest.java | 22 ++++++++++++++----- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java index f0e20632b4..f79499f5dd 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java @@ -130,9 +130,11 @@ public final class TypeHelper { return "java.lang.Enum".equals(clazzName) // supertypes of Enum || "java.lang.Comparable".equals(clazzName) - || "java.io.Serializable".equals(clazzName); + || "java.io.Serializable".equals(clazzName) + || "java.lang.Object".equals(clazzName); } else if (n instanceof ASTAnnotationTypeDeclaration) { - return "java.lang.annotation.Annotation".equals(clazzName); + return "java.lang.annotation.Annotation".equals(clazzName) + || "java.lang.Object".equals(clazzName); } return false; diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelperTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelperTest.java index c158e669dc..0d4abbbf5b 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelperTest.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelperTest.java @@ -13,6 +13,7 @@ import org.junit.Test; import net.sourceforge.pmd.lang.java.ast.ASTAnnotationTypeDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTEnumDeclaration; +import net.sourceforge.pmd.lang.java.ast.TypeNode; import net.sourceforge.pmd.lang.java.symboltable.BaseNonParserTest; public class TypeHelperTest extends BaseNonParserTest { @@ -46,11 +47,10 @@ public class TypeHelperTest extends BaseNonParserTest { Assert.assertNull(klass.getType()); Assert.assertTrue(TypeHelper.isA(klass, "org.FooBar")); - Assert.assertTrue(TypeHelper.isA(klass, "java.lang.Iterable")); - Assert.assertTrue(TypeHelper.isA(klass, Iterable.class)); - Assert.assertTrue(TypeHelper.isA(klass, Enum.class)); - Assert.assertTrue(TypeHelper.isA(klass, Serializable.class)); - Assert.assertTrue(TypeHelper.isA(klass, Comparable.class)); + assertIsA(klass, Iterable.class); + assertIsA(klass, Enum.class); + assertIsA(klass, Serializable.class); + assertIsA(klass, Object.class); } @Test @@ -64,8 +64,18 @@ public class TypeHelperTest extends BaseNonParserTest { Assert.assertNull(klass.getType()); Assert.assertTrue(TypeHelper.isA(klass, "org.FooBar")); - Assert.assertTrue(TypeHelper.isA(klass, Annotation.class)); + assertIsA(klass, Annotation.class); + assertIsA(klass, Object.class); } + private void assertIsA(TypeNode node, Class<?> type) { + Assert.assertTrue(TypeHelper.isA(node, type)); + Assert.assertTrue(TypeHelper.isA(node, type.getCanonicalName())); + } + + private void assertIsExactlyA(TypeNode node, Class<?> type) { + Assert.assertTrue(TypeHelper.isExactlyA(node, type.getCanonicalName())); + assertIsA(node, type); + } } From 105e48b8e126eea57a2e9d5c85f0a3c749e878a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= <clement.fournier76@gmail.com> Date: Mon, 23 Mar 2020 22:54:05 +0100 Subject: [PATCH 10/56] Fix TypeHelper array loading --- .../lang/java/typeresolution/TypeHelper.java | 133 ++++++++++-------- .../java/typeresolution/TypeHelperTest.java | 54 ++++++- 2 files changed, 126 insertions(+), 61 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java index f79499f5dd..9114a2fe60 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java @@ -4,7 +4,7 @@ package net.sourceforge.pmd.lang.java.typeresolution; -import java.util.Collections; +import java.lang.reflect.Array; import java.util.HashMap; import java.util.Map; @@ -23,43 +23,22 @@ import net.sourceforge.pmd.lang.java.typeresolution.internal.NullableClassLoader public final class TypeHelper { - - /** Maps a primitive class name to its corresponding abbreviation used in array class names. */ - private static final Map<String, String> abbreviationMap; /** Maps names of primitives to their corresponding primitive {@code Class}es. */ - private static final Map<String, Class<?>> namePrimitiveMap = new HashMap<>(); + private static final Map<String, Class<?>> PRIMITIVES_BY_NAME = new HashMap<>(); static { - namePrimitiveMap.put("boolean", Boolean.TYPE); - namePrimitiveMap.put("byte", Byte.TYPE); - namePrimitiveMap.put("char", Character.TYPE); - namePrimitiveMap.put("short", Short.TYPE); - namePrimitiveMap.put("int", Integer.TYPE); - namePrimitiveMap.put("long", Long.TYPE); - namePrimitiveMap.put("double", Double.TYPE); - namePrimitiveMap.put("float", Float.TYPE); - namePrimitiveMap.put("void", Void.TYPE); + PRIMITIVES_BY_NAME.put("boolean", Boolean.TYPE); + PRIMITIVES_BY_NAME.put("byte", Byte.TYPE); + PRIMITIVES_BY_NAME.put("char", Character.TYPE); + PRIMITIVES_BY_NAME.put("short", Short.TYPE); + PRIMITIVES_BY_NAME.put("int", Integer.TYPE); + PRIMITIVES_BY_NAME.put("long", Long.TYPE); + PRIMITIVES_BY_NAME.put("double", Double.TYPE); + PRIMITIVES_BY_NAME.put("float", Float.TYPE); + PRIMITIVES_BY_NAME.put("void", Void.TYPE); } - static { - final Map<String, String> m = new HashMap<>(); - m.put("int", "I"); - m.put("boolean", "Z"); - m.put("float", "F"); - m.put("long", "J"); - m.put("short", "S"); - m.put("byte", "B"); - m.put("double", "D"); - m.put("char", "C"); - final Map<String, String> r = new HashMap<>(); - for (final Map.Entry<String, String> e : m.entrySet()) { - r.put(e.getValue(), e.getKey()); - } - abbreviationMap = Collections.unmodifiableMap(m); - } - - private TypeHelper() { // utility class } @@ -79,7 +58,7 @@ public final class TypeHelper { */ public static boolean isA(final TypeNode n, final String clazzName) { if (n.getType() != null && n.getType().isAnnotation()) { - return isAnnotationSubtype(n, clazzName); + return isAnnotationSubtype(n.getType(), clazzName); } final Class<?> clazz = loadClassWithNodeClassloader(n, clazzName); @@ -91,13 +70,18 @@ public final class TypeHelper { return fallbackIsA(n, clazzName); } - private static boolean isAnnotationSubtype(TypeNode n, String clazzName) { + /** + * Returns true if the class n is a subtype of clazzName, given n + * is an annotationt type. + */ + private static boolean isAnnotationSubtype(Class<?> n, String clazzName) { + assert n != null && n.isAnnotation() : "Not an annotation type"; // then, the supertype may only be Object, j.l.Annotation, or the class name // this avoids classloading altogether // this is used e.g. by the typeIs function in XPath - return clazzName.equals("java.lang.annotation.Annotation") - || clazzName.equals("java.lang.Object") - || clazzName.equals(n.getType().getName()); + return "java.lang.annotation.Annotation".equals(clazzName) + || "java.lang.Object".equals(clazzName) + || clazzName.equals(n.getName()); } private static boolean fallbackIsA(TypeNode n, String clazzName) { @@ -171,47 +155,78 @@ public final class TypeHelper { return null; } - private static Class<?> loadClass(NullableClassLoader ctr, String className) { - Class<?> clazz; - if (namePrimitiveMap.containsKey(className)) { - clazz = namePrimitiveMap.get(className); - } else { - clazz = ctr.loadClassOrNull(toCanonicalName(className)); + + /** + * Load a class. Supports loading array types like 'java.lang.String[]' and + * converting a canonical name to a binary name (eg 'java.util.Map.Entry' -> + * 'java.util.Map$Entry'). + */ + // test only + static Class<?> loadClass(NullableClassLoader ctr, String className) { + return loadClassMaybeArray(ctr, StringUtils.deleteWhitespace(className)); + } + + private static Class<?> loadClassFromCanonicalName(NullableClassLoader ctr, String className) { + Class<?> clazz = PRIMITIVES_BY_NAME.get(className); + if (clazz == null) { + clazz = ctr.loadClassOrNull(className); } if (clazz != null) { return clazz; } - // allow path separators (.) as inner class name separators final int lastDotIndex = className.lastIndexOf('.'); - if (lastDotIndex != -1) { + if (lastDotIndex >= 0) { String asInner = className.substring(0, lastDotIndex) + '$' + className.substring(lastDotIndex + 1); - return loadClass(ctr, asInner); + return loadClassFromCanonicalName(ctr, asInner); } return null; } - private static String toCanonicalName(String className) { - className = StringUtils.deleteWhitespace(className); + private static Class<?> loadClassMaybeArray(NullableClassLoader classLoader, + String className) { Validate.notNull(className, "className must not be null."); if (className.endsWith("[]")) { - final StringBuilder classNameBuffer = new StringBuilder(); - while (className.endsWith("[]")) { - className = className.substring(0, className.length() - 2); - classNameBuffer.append("["); + int dimension = 0; + int i = className.length(); + while (i >= 2 && className.startsWith("[]", i - 2)) { + dimension++; + i -= 2; } - final String abbreviation = abbreviationMap.get(className); - if (abbreviation != null) { - classNameBuffer.append(abbreviation); - } else { - classNameBuffer.append("L").append(className).append(";"); + + checkJavaIdent(className, i); + String elementName = className.substring(0, i); + + Class<?> elementType = loadClassFromCanonicalName(classLoader, elementName); + if (elementType == null) { + return null; + } + + return Array.newInstance(elementType, (int[]) Array.newInstance(int.class, dimension)).getClass(); + } else { + checkJavaIdent(className, className.length()); + return loadClassFromCanonicalName(classLoader, className); + } + } + + private static IllegalArgumentException invalidClassName(String className) { + return new IllegalArgumentException("Not a valid class name \"" + className + "\""); + } + + private static void checkJavaIdent(String className, int endOffsetExclusive) { + if (endOffsetExclusive <= 0 || !Character.isJavaIdentifierStart(className.charAt(0))) { + throw invalidClassName(className); + } + + for (int i = 1; i < endOffsetExclusive; i++) { + char c = className.charAt(i); + if (!(Character.isJavaIdentifierPart(c) || c == '.')) { + throw invalidClassName(className); } - className = classNameBuffer.toString(); } - return className; } diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelperTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelperTest.java index 0d4abbbf5b..e0a744135d 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelperTest.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelperTest.java @@ -4,20 +4,30 @@ package net.sourceforge.pmd.lang.java.typeresolution; +import static net.sourceforge.pmd.lang.java.typeresolution.internal.NullableClassLoader.ClassLoaderWrapper.wrapNullable; + import java.io.Serializable; import java.lang.annotation.Annotation; +import java.util.Map; import org.junit.Assert; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import net.sourceforge.pmd.lang.java.ast.ASTAnnotationTypeDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTEnumDeclaration; import net.sourceforge.pmd.lang.java.ast.TypeNode; import net.sourceforge.pmd.lang.java.symboltable.BaseNonParserTest; +import net.sourceforge.pmd.lang.java.typeresolution.internal.NullableClassLoader; public class TypeHelperTest extends BaseNonParserTest { + private static final NullableClassLoader LOADER = wrapNullable(TypeHelperTest.class.getClassLoader()); + + @Rule + public final ExpectedException expect = ExpectedException.none(); @Test public void testIsAFallback() { @@ -69,8 +79,10 @@ public class TypeHelperTest extends BaseNonParserTest { } private void assertIsA(TypeNode node, Class<?> type) { - Assert.assertTrue(TypeHelper.isA(node, type)); - Assert.assertTrue(TypeHelper.isA(node, type.getCanonicalName())); + Assert.assertTrue("TypeHelper::isA with class arg: " + type.getCanonicalName(), + TypeHelper.isA(node, type)); + Assert.assertTrue("TypeHelper::isA with string arg: " + type.getCanonicalName(), + TypeHelper.isA(node, type.getCanonicalName())); } private void assertIsExactlyA(TypeNode node, Class<?> type) { @@ -78,4 +90,42 @@ public class TypeHelperTest extends BaseNonParserTest { assertIsA(node, type); } + + @Test + public void testNestedClass() { + Class<?> c = TypeHelper.loadClass(LOADER, "java.util.Map.Entry"); + Assert.assertEquals(Map.Entry.class, c); + } + + + @Test + public void testPrimitiveArray() { + Class<?> c = TypeHelper.loadClass(LOADER, "int[ ]"); + Assert.assertEquals(int[].class, c); + } + + @Test + public void testNestedClassArray() { + Class<?> c = TypeHelper.loadClass(LOADER, "java.util.Map.Entry[ ]"); + Assert.assertEquals(Map.Entry[].class, c); + } + + @Test + public void testInvalidName() { + expect.expect(IllegalArgumentException.class); + TypeHelper.loadClass(LOADER, "java.util.Map ]"); + } + + @Test + public void testInvalidName2() { + expect.expect(IllegalArgumentException.class); + TypeHelper.loadClass(LOADER, "[]"); + } + + @Test + public void testNullName() { + expect.expect(NullPointerException.class); + TypeHelper.loadClass(LOADER, null); + } + } From 25ef59d5a72dbc6a0c42491131ad891472361e0e Mon Sep 17 00:00:00 2001 From: Andreas Dangel <andreas.dangel@pmd-code.org> Date: Fri, 27 Mar 2020 18:41:05 +0100 Subject: [PATCH 11/56] [java] Performance improve xpath based rules If an unbounded path expression like //PrimaryExpression[//ClassOrInterfaceDeclaration] is used, then this will search the whole document again. It will also avoid using rule chain later on, see #2382 --- .../resources/category/java/bestpractices.xml | 71 +++++++++++-------- .../main/resources/category/java/design.xml | 26 ++++--- .../resources/category/java/errorprone.xml | 66 ++++++++++------- .../category/java/multithreading.xml | 10 +-- 4 files changed, 104 insertions(+), 69 deletions(-) diff --git a/pmd-java/src/main/resources/category/java/bestpractices.xml b/pmd-java/src/main/resources/category/java/bestpractices.xml index 5c089e46d7..d15c60e627 100644 --- a/pmd-java/src/main/resources/category/java/bestpractices.xml +++ b/pmd-java/src/main/resources/category/java/bestpractices.xml @@ -1421,16 +1421,21 @@ This rule detects JUnit assertions in object equality. These assertions should b PrimaryPrefix/Name[@Image = 'assertTrue'] ][ PrimarySuffix/Arguments/ArgumentList/Expression/PrimaryExpression/PrimaryPrefix/Name - [ends-with(@Image, '.equals')] + [ends-with(@Image, '.equals')] ] -[ancestor::ClassOrInterfaceDeclaration[//ClassOrInterfaceType[pmd-java:typeIs('junit.framework.TestCase')] - or //MarkerAnnotation/Name[ +[ancestor::ClassOrInterfaceDeclaration[ + pmd-java:typeIs('junit.framework.TestCase') + or .//MarkerAnnotation/Name[ pmd-java:typeIs('org.junit.Test') - or pmd-java:typeIs('org.junit.jupiter.api.Test') or pmd-java:typeIs('org.junit.jupiter.api.RepeatedTest') - or pmd-java:typeIs('org.junit.jupiter.api.TestFactory') or pmd-java:typeIs('org.junit.jupiter.api.TestTemplate') + or pmd-java:typeIs('org.junit.jupiter.api.Test') + or pmd-java:typeIs('org.junit.jupiter.api.RepeatedTest') + or pmd-java:typeIs('org.junit.jupiter.api.TestFactory') + or pmd-java:typeIs('org.junit.jupiter.api.TestTemplate') or pmd-java:typeIs('org.junit.jupiter.params.ParameterizedTest') + ] ] -]]]]> +] +]]> </value> </property> </properties> @@ -1464,20 +1469,25 @@ more specific methods, like assertNull, assertNotNull. <value> <![CDATA[ //PrimaryExpression[ - PrimaryPrefix/Name[@Image = 'assertTrue' or @Image = 'assertFalse'] + PrimaryPrefix/Name[@Image = 'assertTrue' or @Image = 'assertFalse'] ][ - PrimarySuffix/Arguments/ArgumentList[ - Expression/EqualityExpression/PrimaryExpression/PrimaryPrefix/Literal/NullLiteral - ] -] -[ancestor::ClassOrInterfaceDeclaration[//ClassOrInterfaceType[pmd-java:typeIs('junit.framework.TestCase')] - or //MarkerAnnotation/Name[ - pmd-java:typeIs('org.junit.Test') - or pmd-java:typeIs('org.junit.jupiter.api.Test') or pmd-java:typeIs('org.junit.jupiter.api.RepeatedTest') - or pmd-java:typeIs('org.junit.jupiter.api.TestFactory') or pmd-java:typeIs('org.junit.jupiter.api.TestTemplate') - or pmd-java:typeIs('org.junit.jupiter.params.ParameterizedTest') + PrimarySuffix/Arguments/ArgumentList[ + Expression/EqualityExpression/PrimaryExpression/PrimaryPrefix/Literal/NullLiteral ] -]]]]> +] +[ancestor::ClassOrInterfaceDeclaration[ + pmd-java:typeIs('junit.framework.TestCase') + or .//MarkerAnnotation/Name[ + pmd-java:typeIs('org.junit.Test') + or pmd-java:typeIs('org.junit.jupiter.api.Test') + or pmd-java:typeIs('org.junit.jupiter.api.RepeatedTest') + or pmd-java:typeIs('org.junit.jupiter.api.TestFactory') + or pmd-java:typeIs('org.junit.jupiter.api.TestTemplate') + or pmd-java:typeIs('org.junit.jupiter.params.ParameterizedTest') + ] + ] +] +]]> </value> </property> </properties> @@ -1513,20 +1523,25 @@ by more specific methods, like assertSame, assertNotSame. <value> <![CDATA[ //PrimaryExpression[ - PrimaryPrefix/Name - [@Image = 'assertTrue' or @Image = 'assertFalse'] + PrimaryPrefix/Name[@Image = 'assertTrue' or @Image = 'assertFalse'] ] -[PrimarySuffix/Arguments - /ArgumentList/Expression - /EqualityExpression[count(.//NullLiteral) = 0]] -[ancestor::ClassOrInterfaceDeclaration[//ClassOrInterfaceType[pmd-java:typeIs('junit.framework.TestCase')] - or //MarkerAnnotation/Name[ +[ + PrimarySuffix/Arguments/ArgumentList/Expression/EqualityExpression + [count(.//NullLiteral) = 0] +] +[ancestor::ClassOrInterfaceDeclaration[ + pmd-java:typeIs('junit.framework.TestCase') + or .//MarkerAnnotation/Name[ pmd-java:typeIs('org.junit.Test') - or pmd-java:typeIs('org.junit.jupiter.api.Test') or pmd-java:typeIs('org.junit.jupiter.api.RepeatedTest') - or pmd-java:typeIs('org.junit.jupiter.api.TestFactory') or pmd-java:typeIs('org.junit.jupiter.api.TestTemplate') + or pmd-java:typeIs('org.junit.jupiter.api.Test') + or pmd-java:typeIs('org.junit.jupiter.api.RepeatedTest') + or pmd-java:typeIs('org.junit.jupiter.api.TestFactory') + or pmd-java:typeIs('org.junit.jupiter.api.TestTemplate') or pmd-java:typeIs('org.junit.jupiter.params.ParameterizedTest') + ] ] -]]]]> +] +]]> </value> </property> </properties> diff --git a/pmd-java/src/main/resources/category/java/design.xml b/pmd-java/src/main/resources/category/java/design.xml index 73008d3e30..8727f9745e 100644 --- a/pmd-java/src/main/resources/category/java/design.xml +++ b/pmd-java/src/main/resources/category/java/design.xml @@ -28,7 +28,7 @@ protected constructor in order to prevent instantiation than make the class misl <![CDATA[ //ClassOrInterfaceDeclaration [@Abstract = 'true'] - [count(//MethodDeclaration) + count(//ConstructorDeclaration) = 0] + [count(.//MethodDeclaration) + count(.//ConstructorDeclaration) = 0] [not(../Annotation/MarkerAnnotation/Name[pmd-java:typeIs('com.google.auto.value.AutoValue')])] ]]> </value> @@ -1253,20 +1253,24 @@ as: <![CDATA[ //StatementExpression [ -.//Name[@Image='assertTrue' or @Image='assertFalse'] -and -PrimaryExpression/PrimarySuffix/Arguments/ArgumentList - /Expression/UnaryExpressionNotPlusMinus[@Image='!'] -/PrimaryExpression/PrimaryPrefix + .//Name[@Image='assertTrue' or @Image='assertFalse'] + and + PrimaryExpression/PrimarySuffix/Arguments/ArgumentList/Expression/UnaryExpressionNotPlusMinus[@Image='!'] + /PrimaryExpression/PrimaryPrefix ] -[ancestor::ClassOrInterfaceDeclaration[//ClassOrInterfaceType[pmd-java:typeIs('junit.framework.TestCase')] - or //MarkerAnnotation/Name[ +[ancestor::ClassOrInterfaceDeclaration[ + pmd-java:typeIs('junit.framework.TestCase') + or .//MarkerAnnotation/Name[ pmd-java:typeIs('org.junit.Test') - or pmd-java:typeIs('org.junit.jupiter.api.Test') or pmd-java:typeIs('org.junit.jupiter.api.RepeatedTest') - or pmd-java:typeIs('org.junit.jupiter.api.TestFactory') or pmd-java:typeIs('org.junit.jupiter.api.TestTemplate') + or pmd-java:typeIs('org.junit.jupiter.api.Test') + or pmd-java:typeIs('org.junit.jupiter.api.RepeatedTest') + or pmd-java:typeIs('org.junit.jupiter.api.TestFactory') + or pmd-java:typeIs('org.junit.jupiter.api.TestTemplate') or pmd-java:typeIs('org.junit.jupiter.params.ParameterizedTest') + ] ] -]]]]> +] +]]> </value> </property> </properties> diff --git a/pmd-java/src/main/resources/category/java/errorprone.xml b/pmd-java/src/main/resources/category/java/errorprone.xml index 938ca51d55..7246e14236 100644 --- a/pmd-java/src/main/resources/category/java/errorprone.xml +++ b/pmd-java/src/main/resources/category/java/errorprone.xml @@ -2161,14 +2161,19 @@ Some JUnit framework methods are easy to misspell. or (not(@Name = 'tearDown') and translate(@Name, 'TEARdOWN', 'tearDown') = 'tearDown')] [@Arity = 0] -[ancestor::ClassOrInterfaceDeclaration[//ClassOrInterfaceType[pmd-java:typeIs('junit.framework.TestCase')] - or //MarkerAnnotation/Name[ +[ancestor::ClassOrInterfaceDeclaration[ + pmd-java:typeIs('junit.framework.TestCase') + or .//MarkerAnnotation/Name[ pmd-java:typeIs('org.junit.Test') - or pmd-java:typeIs('org.junit.jupiter.api.Test') or pmd-java:typeIs('org.junit.jupiter.api.RepeatedTest') - or pmd-java:typeIs('org.junit.jupiter.api.TestFactory') or pmd-java:typeIs('org.junit.jupiter.api.TestTemplate') + or pmd-java:typeIs('org.junit.jupiter.api.Test') + or pmd-java:typeIs('org.junit.jupiter.api.RepeatedTest') + or pmd-java:typeIs('org.junit.jupiter.api.TestFactory') + or pmd-java:typeIs('org.junit.jupiter.api.TestTemplate') or pmd-java:typeIs('org.junit.jupiter.params.ParameterizedTest') + ] ] -]]]]> +] +]]> </value> </property> </properties> @@ -2202,14 +2207,19 @@ The suite() method in a JUnit test needs to be both public and static. //MethodDeclaration[not(@Static='true') or not(@Public='true')] [@Name='suite'] [@Arity = 0] -[ancestor::ClassOrInterfaceDeclaration[//ClassOrInterfaceType[pmd-java:typeIs('junit.framework.TestCase')] - or //MarkerAnnotation/Name[ +[ancestor::ClassOrInterfaceDeclaration[ + pmd-java:typeIs('junit.framework.TestCase') + or .//MarkerAnnotation/Name[ pmd-java:typeIs('org.junit.Test') - or pmd-java:typeIs('org.junit.jupiter.api.Test') or pmd-java:typeIs('org.junit.jupiter.api.RepeatedTest') - or pmd-java:typeIs('org.junit.jupiter.api.TestFactory') or pmd-java:typeIs('org.junit.jupiter.api.TestTemplate') + or pmd-java:typeIs('org.junit.jupiter.api.Test') + or pmd-java:typeIs('org.junit.jupiter.api.RepeatedTest') + or pmd-java:typeIs('org.junit.jupiter.api.TestFactory') + or pmd-java:typeIs('org.junit.jupiter.api.TestTemplate') or pmd-java:typeIs('org.junit.jupiter.params.ParameterizedTest') + ] ] -]]]]> +] +]]> </value> </property> </properties> @@ -3223,22 +3233,28 @@ an error, use the fail() method and provide an indication message of why it did. <![CDATA[ //StatementExpression [ -PrimaryExpression/PrimaryPrefix/Name[@Image='assertTrue' or @Image='assertFalse'] -and -PrimaryExpression/PrimarySuffix/Arguments/ArgumentList/Expression -[PrimaryExpression/PrimaryPrefix/Literal/BooleanLiteral -or -UnaryExpressionNotPlusMinus[@Image='!'] -/PrimaryExpression/PrimaryPrefix[Literal/BooleanLiteral or Name[count(../../*)=1]]] -] -[ancestor::ClassOrInterfaceDeclaration[//ClassOrInterfaceType[pmd-java:typeIs('junit.framework.TestCase')] - or //MarkerAnnotation/Name[ - pmd-java:typeIs('org.junit.Test') - or pmd-java:typeIs('org.junit.jupiter.api.Test') or pmd-java:typeIs('org.junit.jupiter.api.RepeatedTest') - or pmd-java:typeIs('org.junit.jupiter.api.TestFactory') or pmd-java:typeIs('org.junit.jupiter.api.TestTemplate') - or pmd-java:typeIs('org.junit.jupiter.params.ParameterizedTest') + PrimaryExpression/PrimaryPrefix/Name[@Image='assertTrue' or @Image='assertFalse'] + and + PrimaryExpression/PrimarySuffix/Arguments/ArgumentList/Expression[ + PrimaryExpression/PrimaryPrefix/Literal/BooleanLiteral + or + UnaryExpressionNotPlusMinus[@Image='!'] + /PrimaryExpression/PrimaryPrefix[Literal/BooleanLiteral or Name[count(../../*)=1]] ] -]]]]> +] +[ancestor::ClassOrInterfaceDeclaration[ + pmd-java:typeIs('junit.framework.TestCase') + or .//MarkerAnnotation/Name[ + pmd-java:typeIs('org.junit.Test') + or pmd-java:typeIs('org.junit.jupiter.api.Test') + or pmd-java:typeIs('org.junit.jupiter.api.RepeatedTest') + or pmd-java:typeIs('org.junit.jupiter.api.TestFactory') + or pmd-java:typeIs('org.junit.jupiter.api.TestTemplate') + or pmd-java:typeIs('org.junit.jupiter.params.ParameterizedTest') + ] + ] +] +]]> </value> </property> </properties> diff --git a/pmd-java/src/main/resources/category/java/multithreading.xml b/pmd-java/src/main/resources/category/java/multithreading.xml index c1ca1d5c2f..d5db6eb8c2 100644 --- a/pmd-java/src/main/resources/category/java/multithreading.xml +++ b/pmd-java/src/main/resources/category/java/multithreading.xml @@ -175,12 +175,12 @@ Explicitly calling Thread.run() method will execute in the caller's thread of co //StatementExpression/PrimaryExpression [ PrimaryPrefix + [pmd-java:typeIs('java.lang.Thread')] [ - ./Name[ends-with(@Image, '.run') or @Image = 'run'] - and substring-before(Name/@Image, '.') =//VariableDeclarator/VariableDeclaratorId/@Image - [../../../Type/ReferenceType/ClassOrInterfaceType[pmd-java:typeIs('java.lang.Thread')]] - or (./AllocationExpression/ClassOrInterfaceType[pmd-java:typeIs('java.lang.Thread')] - and ../PrimarySuffix[@Image = 'run']) + ./Name[ends-with(@Image, '.run') or @Image = 'run'] + or + ./AllocationExpression/ClassOrInterfaceType[pmd-java:typeIs('java.lang.Thread')] + and ../PrimarySuffix[@Image = 'run'] ] ] ]]> From 552931d965e4ec0e8ad8263c274e94a8b20bc1de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= <clement.fournier76@gmail.com> Date: Sat, 28 Mar 2020 11:03:40 +0100 Subject: [PATCH 12/56] Fix AbstractClassWithoutAnyMethods FN with inner classes --- .../main/resources/category/java/design.xml | 2 +- .../xml/AbstractClassWithoutAnyMethod.xml | 33 +++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/pmd-java/src/main/resources/category/java/design.xml b/pmd-java/src/main/resources/category/java/design.xml index 8727f9745e..2fb4fe92d8 100644 --- a/pmd-java/src/main/resources/category/java/design.xml +++ b/pmd-java/src/main/resources/category/java/design.xml @@ -28,7 +28,7 @@ protected constructor in order to prevent instantiation than make the class misl <![CDATA[ //ClassOrInterfaceDeclaration [@Abstract = 'true'] - [count(.//MethodDeclaration) + count(.//ConstructorDeclaration) = 0] + [count(./ClassOrInterfaceBody/*/MethodDeclaration) + count(./ClassOrInterfaceBody/*/ConstructorDeclaration) = 0] [not(../Annotation/MarkerAnnotation/Name[pmd-java:typeIs('com.google.auto.value.AutoValue')])] ]]> </value> diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/AbstractClassWithoutAnyMethod.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/AbstractClassWithoutAnyMethod.xml index 96a38ed646..1078e4946d 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/AbstractClassWithoutAnyMethod.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/AbstractClassWithoutAnyMethod.xml @@ -76,4 +76,37 @@ import com.google.auto.value.AutoValue; } ]]></code> </test-code> + + + <test-code> + <description>FN with nested class</description> + <expected-problems>1</expected-problems> + <expected-linenumbers>1</expected-linenumbers> + <code><![CDATA[ +abstract class Foo { + + class Inner { + void ohio() {} + } + +} + ]]></code> + </test-code> + + <test-code> + <description>FN with nested class</description> + <expected-problems>1</expected-problems> + <expected-linenumbers>3</expected-linenumbers> + <code><![CDATA[ +class Foo { + + abstract class Pos {} + + class Sibling { + void ohio() {} + } + +} + ]]></code> + </test-code> </test-data> From 485fa4ad500c467e258dc5073cd9a4a1ecd4e264 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= <clement.fournier76@gmail.com> Date: Sat, 28 Mar 2020 11:06:10 +0100 Subject: [PATCH 13/56] Fix test case name --- .../lang/java/rule/design/xml/AbstractClassWithoutAnyMethod.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/AbstractClassWithoutAnyMethod.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/AbstractClassWithoutAnyMethod.xml index 1078e4946d..a25b160013 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/AbstractClassWithoutAnyMethod.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/AbstractClassWithoutAnyMethod.xml @@ -94,7 +94,7 @@ abstract class Foo { </test-code> <test-code> - <description>FN with nested class</description> + <description>FN with sibling class</description> <expected-problems>1</expected-problems> <expected-linenumbers>3</expected-linenumbers> <code><![CDATA[ From 9e9c370a4a8959ebfcb9d56f5bc5e918a6addc0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= <clement.fournier76@gmail.com> Date: Tue, 17 Mar 2020 20:27:14 +0100 Subject: [PATCH 14/56] Make yield more conditional Refs #2319 --- pmd-java/etc/grammar/Java.jjt | 50 ++++++++++++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/pmd-java/etc/grammar/Java.jjt b/pmd-java/etc/grammar/Java.jjt index 80186c015e..9b881d591f 100644 --- a/pmd-java/etc/grammar/Java.jjt +++ b/pmd-java/etc/grammar/Java.jjt @@ -502,6 +502,52 @@ public class JavaParser { return getToken(1).kind == IDENTIFIER && getToken(1).image.equals(keyword); } + /** + * True if we're in a switch block, one precondition for parsing a yield + * statement. + */ + private boolean inSwitchBlock = false; + + private boolean isYieldStart() { + return inSwitchBlock && isJava13PreviewOr14() + && isKeyword("yield") + && mayStartExprAfterYield(2); + } + + private boolean mayStartExprAfterYield(final int offset) { + // based off of https://hg.openjdk.java.net/jdk/jdk/file/bc3da0226ffa/src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java#l2580 + // please don't sue me + Token token = getToken(offset); + if (token == null) return false; // eof + switch (token.kind) { + case PLUS: case MINUS: case STRING_LITERAL: case CHARACTER_LITERAL: + case INTEGER_LITERAL: case FLOATING_POINT_LITERAL: case HEX_FLOATING_POINT_LITERAL: + case NULL: case IDENTIFIER: case TRUE: case FALSE: + case NEW: case SWITCH: case THIS: case SUPER: + return true; + case INCR: case DECR: + return getToken(offset + 1).kind != SEMICOLON; // eg yield++; + case LPAREN: + int lookahead = offset + 1; + int balance = 1; + Token t; + while ((t = getToken(lookahead)) != null && balance > 0) { + switch (t.kind) { + case LPAREN: balance++; break; + case RPAREN: balance--; break; + case COMMA: if (balance == 1) return false; // a method call, eg yield(1, 2); + } + lookahead++; + } + // lambda: yield () -> {}; + // method call: yield (); + return lookahead != offset + 2 // ie () + || t.kind == ARROW; + default: + return false; + } + } + private boolean shouldStartStatementInSwitch() { switch (getToken(1).kind) { case _DEFAULT: @@ -1900,8 +1946,9 @@ void SwitchStatement(): } void SwitchBlock() #void : -{} +{boolean prevInSwitchBlock = inSwitchBlock;} { + {inSwitchBlock = true;} "{" ( SwitchLabel() @@ -1916,6 +1963,7 @@ void SwitchBlock() #void : ) )? "}" + {inSwitchBlock = prevInSwitchBlock;} } void SwitchLabeledRule() #void : From b01d4dc0d7bf6a2fa7b85a265635ad5bddbe70b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= <clement.fournier76@gmail.com> Date: Tue, 17 Mar 2020 21:36:15 +0100 Subject: [PATCH 15/56] Test --- .../pmd/lang/ast/AbstractNode.java | 4 +- .../net/sourceforge/pmd/lang/ast/Node.java | 2 +- pmd-java/etc/grammar/Java.jjt | 23 +-- .../pmd/lang/java/ast/Java14Test.java | 141 +++++++++++++----- .../java14/YieldStatements.java | 35 +++++ 5 files changed, 151 insertions(+), 54 deletions(-) create mode 100644 pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/ast/jdkversiontests/java14/YieldStatements.java diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/AbstractNode.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/AbstractNode.java index e7f7b0fdef..c14ede1d12 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/AbstractNode.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/AbstractNode.java @@ -357,7 +357,7 @@ public abstract class AbstractNode implements Node { } @Override - public <T> List<T> findDescendantsOfType(final Class<T> targetType) { + public <T> List<T> findDescendantsOfType(final Class<? extends T> targetType) { final List<T> list = new ArrayList<>(); findDescendantsOfType(this, targetType, list, false); return list; @@ -381,7 +381,7 @@ public abstract class AbstractNode implements Node { findDescendantsOfType(this, targetType, results, crossBoundaries); } - private static <T> void findDescendantsOfType(final Node node, final Class<T> targetType, final List<T> results, + private static <T> void findDescendantsOfType(final Node node, final Class<? extends T> targetType, final List<T> results, final boolean crossFindBoundaries) { for (Node child : node.children()) { diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/Node.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/Node.java index 1b8172ec54..46b9ae08f3 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/Node.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/Node.java @@ -284,7 +284,7 @@ public interface Node { * @return List of all children of type targetType. Returns an empty list if * none found. */ - <T> List<T> findDescendantsOfType(Class<T> targetType); + <T> List<T> findDescendantsOfType(Class<? extends T> targetType); /** * Traverses down the tree to find all the descendant instances of type diff --git a/pmd-java/etc/grammar/Java.jjt b/pmd-java/etc/grammar/Java.jjt index 9b881d591f..ed02e26612 100644 --- a/pmd-java/etc/grammar/Java.jjt +++ b/pmd-java/etc/grammar/Java.jjt @@ -506,10 +506,10 @@ public class JavaParser { * True if we're in a switch block, one precondition for parsing a yield * statement. */ - private boolean inSwitchBlock = false; + private boolean inSwitchExprBlock = false; private boolean isYieldStart() { - return inSwitchBlock && isJava13PreviewOr14() + return inSwitchExprBlock && isJava13PreviewOr14() && isKeyword("yield") && mayStartExprAfterYield(2); } @@ -541,8 +541,9 @@ public class JavaParser { } // lambda: yield () -> {}; // method call: yield (); - return lookahead != offset + 2 // ie () - || t.kind == ARROW; + return t != null + && (lookahead != offset + 2 // ie () + || t.kind == LAMBDA); default: return false; } @@ -1667,10 +1668,13 @@ void CastExpression() : } void SwitchExpression() : -{} +{boolean prevInSwitchBlock = inSwitchExprBlock;} { {checkForSwitchExpression();} - "switch" "(" Expression() ")" SwitchBlock() + "switch" "(" Expression() ")" + {inSwitchExprBlock = true;} + SwitchBlock() + {inSwitchExprBlock = prevInSwitchBlock;} } void PrimaryExpression() : @@ -1838,6 +1842,7 @@ void Statement() : {} { LOOKAHEAD( { isNextTokenAnAssert() } ) AssertStatement() +| LOOKAHEAD( { isYieldStart() } ) YieldStatement() | LOOKAHEAD(2) LabeledStatement() | Block() | EmptyStatement() @@ -1873,7 +1878,7 @@ void BlockStatement(): {} { LOOKAHEAD( { isNextTokenAnAssert() } ) AssertStatement() -| LOOKAHEAD({ jdkVersion >= 13 && isKeyword("yield") }) YieldStatement() +| LOOKAHEAD( { isYieldStart() } ) YieldStatement() | LOOKAHEAD(( "final" | Annotation() )* Type() <IDENTIFIER>) LocalVariableDeclaration() ";" @@ -1946,9 +1951,8 @@ void SwitchStatement(): } void SwitchBlock() #void : -{boolean prevInSwitchBlock = inSwitchBlock;} +{} { - {inSwitchBlock = true;} "{" ( SwitchLabel() @@ -1963,7 +1967,6 @@ void SwitchBlock() #void : ) )? "}" - {inSwitchBlock = prevInSwitchBlock;} } void SwitchLabeledRule() #void : diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/Java14Test.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/Java14Test.java index 334335db65..86cb5e08c7 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/Java14Test.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/Java14Test.java @@ -5,9 +5,14 @@ package net.sourceforge.pmd.lang.java.ast; +import static org.hamcrest.CoreMatchers.instanceOf; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; + import java.util.List; -import org.junit.Assert; import org.junit.Test; import net.sourceforge.pmd.lang.ast.ParseException; @@ -47,36 +52,90 @@ public class Java14Test { private void parseAndCheckSwitchExpression(JavaParsingHelper parser) { ASTCompilationUnit compilationUnit = parser.parseResource("SwitchExpressions.java"); List<ASTSwitchStatement> switchStatements = compilationUnit.findDescendantsOfType(ASTSwitchStatement.class); - Assert.assertEquals(2, switchStatements.size()); + assertEquals(2, switchStatements.size()); - Assert.assertTrue(switchStatements.get(0).getChild(0) instanceof ASTExpression); - Assert.assertTrue(switchStatements.get(0).getChild(1) instanceof ASTSwitchLabeledExpression); - Assert.assertTrue(switchStatements.get(0).getChild(1).getChild(0) instanceof ASTSwitchLabel); - Assert.assertEquals(3, switchStatements.get(0).getChild(1).getChild(0).getNumChildren()); - Assert.assertTrue(switchStatements.get(0).getChild(2).getChild(0) instanceof ASTSwitchLabel); - Assert.assertFalse(((ASTSwitchLabel) switchStatements.get(0).getChild(2).getChild(0)).isDefault()); - Assert.assertEquals(1, switchStatements.get(0).getChild(2).getChild(0).getNumChildren()); + assertTrue(switchStatements.get(0).getChild(0) instanceof ASTExpression); + assertTrue(switchStatements.get(0).getChild(1) instanceof ASTSwitchLabeledExpression); + assertTrue(switchStatements.get(0).getChild(1).getChild(0) instanceof ASTSwitchLabel); + assertEquals(3, switchStatements.get(0).getChild(1).getChild(0).getNumChildren()); + assertTrue(switchStatements.get(0).getChild(2).getChild(0) instanceof ASTSwitchLabel); + assertFalse(((ASTSwitchLabel) switchStatements.get(0).getChild(2).getChild(0)).isDefault()); + assertEquals(1, switchStatements.get(0).getChild(2).getChild(0).getNumChildren()); - Assert.assertTrue(switchStatements.get(1).getChild(3) instanceof ASTSwitchLabeledExpression); - Assert.assertTrue(switchStatements.get(1).getChild(3).getChild(0) instanceof ASTSwitchLabel); - Assert.assertTrue(((ASTSwitchLabel) switchStatements.get(1).getChild(3).getChild(0)).isDefault()); + assertTrue(switchStatements.get(1).getChild(3) instanceof ASTSwitchLabeledExpression); + assertTrue(switchStatements.get(1).getChild(3).getChild(0) instanceof ASTSwitchLabel); + assertTrue(((ASTSwitchLabel) switchStatements.get(1).getChild(3).getChild(0)).isDefault()); List<ASTSwitchExpression> switchExpressions = compilationUnit.findDescendantsOfType(ASTSwitchExpression.class); - Assert.assertEquals(4, switchExpressions.size()); + assertEquals(4, switchExpressions.size()); - Assert.assertEquals(Integer.TYPE, switchExpressions.get(0).getType()); - Assert.assertEquals(4, switchExpressions.get(0).findChildrenOfType(ASTSwitchLabeledExpression.class).size()); - Assert.assertEquals(Integer.TYPE, switchExpressions.get(0).getFirstChildOfType(ASTSwitchLabeledExpression.class) - .getFirstChildOfType(ASTExpression.class).getType()); + assertEquals(Integer.TYPE, switchExpressions.get(0).getType()); + assertEquals(4, switchExpressions.get(0).findChildrenOfType(ASTSwitchLabeledExpression.class).size()); + assertEquals(Integer.TYPE, switchExpressions.get(0).getFirstChildOfType(ASTSwitchLabeledExpression.class) + .getFirstChildOfType(ASTExpression.class).getType()); - Assert.assertTrue(switchExpressions.get(1).getChild(3) instanceof ASTSwitchLabeledBlock); + assertTrue(switchExpressions.get(1).getChild(3) instanceof ASTSwitchLabeledBlock); - Assert.assertEquals(Integer.TYPE, switchExpressions.get(2).getType()); + assertEquals(Integer.TYPE, switchExpressions.get(2).getType()); List<ASTYieldStatement> yields = switchExpressions.get(2).findDescendantsOfType(ASTYieldStatement.class); - Assert.assertEquals(4, yields.size()); - Assert.assertEquals("SwitchExpressions.BAZ", yields.get(2).getImage()); + assertEquals(4, yields.size()); + assertEquals("SwitchExpressions.BAZ", yields.get(2).getImage()); - Assert.assertEquals(String.class, switchExpressions.get(3).getType()); + assertEquals(String.class, switchExpressions.get(3).getType()); + } + + @Test + public void checkYieldConditionalBehaviour() { + checkYieldStatements(java13p); + } + + @Test + public void checkYieldConditionalBehaviourJ14() { + checkYieldStatements(java14); + } + + private void checkYieldStatements(JavaParsingHelper parser) { + ASTCompilationUnit compilationUnit = parser.parseResource("YieldStatements.java"); + List<JavaNode> stmts = compilationUnit.<JavaNode>findDescendantsOfType(ASTBlockStatement.class); + // fetch the interesting node, on the java-grammar branch this is not needed + for (int i = 0; i < stmts.size(); i++) { + JavaNode child = stmts.get(i).getChild(0); + + if (child instanceof ASTStatement) { + stmts.set(i, child.getChild(0)); + } else { + stmts.set(i, child); + } + } + + assertEquals(18, stmts.size()); + + int i = 0; + assertThat(stmts.get(i++), instanceOf(ASTLocalVariableDeclaration.class)); + assertThat(stmts.get(i++), instanceOf(ASTStatementExpression.class)); + assertThat(stmts.get(i++), instanceOf(ASTStatementExpression.class)); + assertThat(stmts.get(i++), instanceOf(ASTStatementExpression.class)); + + assertThat(stmts.get(i++), instanceOf(ASTStatementExpression.class)); + + assertThat(stmts.get(i++), instanceOf(ASTStatementExpression.class)); + assertThat(stmts.get(i++), instanceOf(ASTStatementExpression.class)); + assertThat(stmts.get(i++), instanceOf(ASTYieldStatement.class)); + assertThat(stmts.get(i++), instanceOf(ASTYieldStatement.class)); + assertThat(stmts.get(i++), instanceOf(ASTYieldStatement.class)); + assertThat(stmts.get(i++), instanceOf(ASTStatementExpression.class)); + assertThat(stmts.get(i++), instanceOf(ASTStatementExpression.class)); + + assertThat(stmts.get(i++), instanceOf(ASTIfStatement.class)); + + assertThat(stmts.get(i++), instanceOf(ASTStatementExpression.class)); + assertThat(stmts.get(i++), instanceOf(ASTYieldStatement.class)); + + assertThat(stmts.get(i++), instanceOf(ASTYieldStatement.class)); + assertThat(stmts.get(i++), instanceOf(ASTStatementExpression.class)); + assertThat(stmts.get(i++), instanceOf(ASTYieldStatement.class)); + + assertEquals(i, stmts.size()); } @Test @@ -89,10 +148,10 @@ public class Java14Test { private void multipleCaseLabels(JavaParsingHelper parser) { ASTCompilationUnit compilationUnit = parser.parseResource("MultipleCaseLabels.java"); ASTSwitchStatement switchStatement = compilationUnit.getFirstDescendantOfType(ASTSwitchStatement.class); - Assert.assertTrue(switchStatement.getChild(0) instanceof ASTExpression); - Assert.assertTrue(switchStatement.getChild(1) instanceof ASTSwitchLabel); + assertTrue(switchStatement.getChild(0) instanceof ASTExpression); + assertTrue(switchStatement.getChild(1) instanceof ASTSwitchLabel); ASTSwitchLabel switchLabel = switchStatement.getFirstChildOfType(ASTSwitchLabel.class); - Assert.assertEquals(3, switchLabel.findChildrenOfType(ASTExpression.class).size()); + assertEquals(3, switchLabel.findChildrenOfType(ASTExpression.class).size()); } @Test @@ -105,22 +164,22 @@ public class Java14Test { private void switchRules(JavaParsingHelper parser) { ASTCompilationUnit compilationUnit = parser.parseResource("SwitchRules.java"); ASTSwitchStatement switchStatement = compilationUnit.getFirstDescendantOfType(ASTSwitchStatement.class); - Assert.assertTrue(switchStatement.getChild(0) instanceof ASTExpression); - Assert.assertTrue(switchStatement.getChild(1) instanceof ASTSwitchLabeledExpression); + assertTrue(switchStatement.getChild(0) instanceof ASTExpression); + assertTrue(switchStatement.getChild(1) instanceof ASTSwitchLabeledExpression); ASTSwitchLabeledExpression switchLabeledExpression = (ASTSwitchLabeledExpression) switchStatement.getChild(1); - Assert.assertEquals(2, switchLabeledExpression.getNumChildren()); - Assert.assertTrue(switchLabeledExpression.getChild(0) instanceof ASTSwitchLabel); - Assert.assertTrue(switchLabeledExpression.getChild(1) instanceof ASTExpression); + assertEquals(2, switchLabeledExpression.getNumChildren()); + assertTrue(switchLabeledExpression.getChild(0) instanceof ASTSwitchLabel); + assertTrue(switchLabeledExpression.getChild(1) instanceof ASTExpression); ASTSwitchLabeledBlock switchLabeledBlock = (ASTSwitchLabeledBlock) switchStatement.getChild(4); - Assert.assertEquals(2, switchLabeledBlock.getNumChildren()); - Assert.assertTrue(switchLabeledBlock.getChild(0) instanceof ASTSwitchLabel); - Assert.assertTrue(switchLabeledBlock.getChild(1) instanceof ASTBlock); + assertEquals(2, switchLabeledBlock.getNumChildren()); + assertTrue(switchLabeledBlock.getChild(0) instanceof ASTSwitchLabel); + assertTrue(switchLabeledBlock.getChild(1) instanceof ASTBlock); ASTSwitchLabeledThrowStatement switchLabeledThrowStatement = (ASTSwitchLabeledThrowStatement) switchStatement.getChild(5); - Assert.assertEquals(2, switchLabeledThrowStatement.getNumChildren()); - Assert.assertTrue(switchLabeledThrowStatement.getChild(0) instanceof ASTSwitchLabel); - Assert.assertTrue(switchLabeledThrowStatement.getChild(1) instanceof ASTThrowStatement); + assertEquals(2, switchLabeledThrowStatement.getNumChildren()); + assertTrue(switchLabeledThrowStatement.getChild(0) instanceof ASTSwitchLabel); + assertTrue(switchLabeledThrowStatement.getChild(1) instanceof ASTThrowStatement); } @Test @@ -133,13 +192,13 @@ public class Java14Test { private void simpleSwitchExpressions(JavaParsingHelper parser) { ASTCompilationUnit compilationUnit = parser.parseResource("SimpleSwitchExpressions.java"); ASTSwitchExpression switchExpression = compilationUnit.getFirstDescendantOfType(ASTSwitchExpression.class); - Assert.assertEquals(6, switchExpression.getNumChildren()); - Assert.assertTrue(switchExpression.getChild(0) instanceof ASTExpression); - Assert.assertEquals(5, switchExpression.findChildrenOfType(ASTSwitchLabeledRule.class).size()); + assertEquals(6, switchExpression.getNumChildren()); + assertTrue(switchExpression.getChild(0) instanceof ASTExpression); + assertEquals(5, switchExpression.findChildrenOfType(ASTSwitchLabeledRule.class).size()); ASTLocalVariableDeclaration localVar = compilationUnit.findDescendantsOfType(ASTLocalVariableDeclaration.class).get(1); ASTVariableDeclarator localVarDecl = localVar.getFirstChildOfType(ASTVariableDeclarator.class); - Assert.assertEquals(Integer.TYPE, localVarDecl.getType()); - Assert.assertEquals(Integer.TYPE, switchExpression.getType()); + assertEquals(Integer.TYPE, localVarDecl.getType()); + assertEquals(Integer.TYPE, switchExpression.getType()); } } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/ast/jdkversiontests/java14/YieldStatements.java b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/ast/jdkversiontests/java14/YieldStatements.java new file mode 100644 index 0000000000..f56e25646f --- /dev/null +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/ast/jdkversiontests/java14/YieldStatements.java @@ -0,0 +1,35 @@ +/** + * @see <a href="https://openjdk.java.net/jeps/361">JEP 361: Switch Expressions (Standard)</a> + */ +public class YieldStatements { + { + int yield = 0; + yield = 2; // should be an assignment + yield (2); // should be a method call + yield(a,b); // should be a method call + + + yield = switch (e) { // must be a switch expr + case 1 -> { + yield(a,b); // should be a method call + yield = 2; // should be an assignment + yield (2); // should be a yield statement + yield++bar; // should be a yield statement (++bar is an expression) + yield--bar; // should be a yield statement (--bar is an expression) + yield++; // should be an increment (not an error) + yield--; // should be a decrement (not an error) + + if (true) yield(2); + else yield 4; + + yield = switch (foo) { // putting a switch in the middles checks the reset behavior + case 4 -> {yield(5);} // should be a yield statement + }; + + yield () -> {}; // should be a yield statement + yield (); // should be a method call + yield (2); // should be a yield statement + } + }; + } +} From 7c433f882039cfc2082b1fbef25a4eae25f7f0dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= <clement.fournier76@gmail.com> Date: Sat, 28 Mar 2020 11:35:39 +0100 Subject: [PATCH 16/56] Remove import to reduce diff --- .../pmd/lang/java/ast/Java14Test.java | 88 +++++++++---------- 1 file changed, 43 insertions(+), 45 deletions(-) diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/Java14Test.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/Java14Test.java index 86cb5e08c7..da41bac41e 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/Java14Test.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/Java14Test.java @@ -6,13 +6,11 @@ package net.sourceforge.pmd.lang.java.ast; import static org.hamcrest.CoreMatchers.instanceOf; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertThat; -import static org.junit.Assert.assertTrue; import java.util.List; +import org.junit.Assert; import org.junit.Test; import net.sourceforge.pmd.lang.ast.ParseException; @@ -52,36 +50,36 @@ public class Java14Test { private void parseAndCheckSwitchExpression(JavaParsingHelper parser) { ASTCompilationUnit compilationUnit = parser.parseResource("SwitchExpressions.java"); List<ASTSwitchStatement> switchStatements = compilationUnit.findDescendantsOfType(ASTSwitchStatement.class); - assertEquals(2, switchStatements.size()); + Assert.assertEquals(2, switchStatements.size()); - assertTrue(switchStatements.get(0).getChild(0) instanceof ASTExpression); - assertTrue(switchStatements.get(0).getChild(1) instanceof ASTSwitchLabeledExpression); - assertTrue(switchStatements.get(0).getChild(1).getChild(0) instanceof ASTSwitchLabel); - assertEquals(3, switchStatements.get(0).getChild(1).getChild(0).getNumChildren()); - assertTrue(switchStatements.get(0).getChild(2).getChild(0) instanceof ASTSwitchLabel); - assertFalse(((ASTSwitchLabel) switchStatements.get(0).getChild(2).getChild(0)).isDefault()); - assertEquals(1, switchStatements.get(0).getChild(2).getChild(0).getNumChildren()); + Assert.assertTrue(switchStatements.get(0).getChild(0) instanceof ASTExpression); + Assert.assertTrue(switchStatements.get(0).getChild(1) instanceof ASTSwitchLabeledExpression); + Assert.assertTrue(switchStatements.get(0).getChild(1).getChild(0) instanceof ASTSwitchLabel); + Assert.assertEquals(3, switchStatements.get(0).getChild(1).getChild(0).getNumChildren()); + Assert.assertTrue(switchStatements.get(0).getChild(2).getChild(0) instanceof ASTSwitchLabel); + Assert.assertFalse(((ASTSwitchLabel) switchStatements.get(0).getChild(2).getChild(0)).isDefault()); + Assert.assertEquals(1, switchStatements.get(0).getChild(2).getChild(0).getNumChildren()); - assertTrue(switchStatements.get(1).getChild(3) instanceof ASTSwitchLabeledExpression); - assertTrue(switchStatements.get(1).getChild(3).getChild(0) instanceof ASTSwitchLabel); - assertTrue(((ASTSwitchLabel) switchStatements.get(1).getChild(3).getChild(0)).isDefault()); + Assert.assertTrue(switchStatements.get(1).getChild(3) instanceof ASTSwitchLabeledExpression); + Assert.assertTrue(switchStatements.get(1).getChild(3).getChild(0) instanceof ASTSwitchLabel); + Assert.assertTrue(((ASTSwitchLabel) switchStatements.get(1).getChild(3).getChild(0)).isDefault()); List<ASTSwitchExpression> switchExpressions = compilationUnit.findDescendantsOfType(ASTSwitchExpression.class); - assertEquals(4, switchExpressions.size()); + Assert.assertEquals(4, switchExpressions.size()); - assertEquals(Integer.TYPE, switchExpressions.get(0).getType()); - assertEquals(4, switchExpressions.get(0).findChildrenOfType(ASTSwitchLabeledExpression.class).size()); - assertEquals(Integer.TYPE, switchExpressions.get(0).getFirstChildOfType(ASTSwitchLabeledExpression.class) - .getFirstChildOfType(ASTExpression.class).getType()); + Assert.assertEquals(Integer.TYPE, switchExpressions.get(0).getType()); + Assert.assertEquals(4, switchExpressions.get(0).findChildrenOfType(ASTSwitchLabeledExpression.class).size()); + Assert.assertEquals(Integer.TYPE, switchExpressions.get(0).getFirstChildOfType(ASTSwitchLabeledExpression.class) + .getFirstChildOfType(ASTExpression.class).getType()); - assertTrue(switchExpressions.get(1).getChild(3) instanceof ASTSwitchLabeledBlock); + Assert.assertTrue(switchExpressions.get(1).getChild(3) instanceof ASTSwitchLabeledBlock); - assertEquals(Integer.TYPE, switchExpressions.get(2).getType()); + Assert.assertEquals(Integer.TYPE, switchExpressions.get(2).getType()); List<ASTYieldStatement> yields = switchExpressions.get(2).findDescendantsOfType(ASTYieldStatement.class); - assertEquals(4, yields.size()); - assertEquals("SwitchExpressions.BAZ", yields.get(2).getImage()); + Assert.assertEquals(4, yields.size()); + Assert.assertEquals("SwitchExpressions.BAZ", yields.get(2).getImage()); - assertEquals(String.class, switchExpressions.get(3).getType()); + Assert.assertEquals(String.class, switchExpressions.get(3).getType()); } @Test @@ -108,7 +106,7 @@ public class Java14Test { } } - assertEquals(18, stmts.size()); + Assert.assertEquals(18, stmts.size()); int i = 0; assertThat(stmts.get(i++), instanceOf(ASTLocalVariableDeclaration.class)); @@ -135,7 +133,7 @@ public class Java14Test { assertThat(stmts.get(i++), instanceOf(ASTStatementExpression.class)); assertThat(stmts.get(i++), instanceOf(ASTYieldStatement.class)); - assertEquals(i, stmts.size()); + Assert.assertEquals(i, stmts.size()); } @Test @@ -148,10 +146,10 @@ public class Java14Test { private void multipleCaseLabels(JavaParsingHelper parser) { ASTCompilationUnit compilationUnit = parser.parseResource("MultipleCaseLabels.java"); ASTSwitchStatement switchStatement = compilationUnit.getFirstDescendantOfType(ASTSwitchStatement.class); - assertTrue(switchStatement.getChild(0) instanceof ASTExpression); - assertTrue(switchStatement.getChild(1) instanceof ASTSwitchLabel); + Assert.assertTrue(switchStatement.getChild(0) instanceof ASTExpression); + Assert.assertTrue(switchStatement.getChild(1) instanceof ASTSwitchLabel); ASTSwitchLabel switchLabel = switchStatement.getFirstChildOfType(ASTSwitchLabel.class); - assertEquals(3, switchLabel.findChildrenOfType(ASTExpression.class).size()); + Assert.assertEquals(3, switchLabel.findChildrenOfType(ASTExpression.class).size()); } @Test @@ -164,22 +162,22 @@ public class Java14Test { private void switchRules(JavaParsingHelper parser) { ASTCompilationUnit compilationUnit = parser.parseResource("SwitchRules.java"); ASTSwitchStatement switchStatement = compilationUnit.getFirstDescendantOfType(ASTSwitchStatement.class); - assertTrue(switchStatement.getChild(0) instanceof ASTExpression); - assertTrue(switchStatement.getChild(1) instanceof ASTSwitchLabeledExpression); + Assert.assertTrue(switchStatement.getChild(0) instanceof ASTExpression); + Assert.assertTrue(switchStatement.getChild(1) instanceof ASTSwitchLabeledExpression); ASTSwitchLabeledExpression switchLabeledExpression = (ASTSwitchLabeledExpression) switchStatement.getChild(1); - assertEquals(2, switchLabeledExpression.getNumChildren()); - assertTrue(switchLabeledExpression.getChild(0) instanceof ASTSwitchLabel); - assertTrue(switchLabeledExpression.getChild(1) instanceof ASTExpression); + Assert.assertEquals(2, switchLabeledExpression.getNumChildren()); + Assert.assertTrue(switchLabeledExpression.getChild(0) instanceof ASTSwitchLabel); + Assert.assertTrue(switchLabeledExpression.getChild(1) instanceof ASTExpression); ASTSwitchLabeledBlock switchLabeledBlock = (ASTSwitchLabeledBlock) switchStatement.getChild(4); - assertEquals(2, switchLabeledBlock.getNumChildren()); - assertTrue(switchLabeledBlock.getChild(0) instanceof ASTSwitchLabel); - assertTrue(switchLabeledBlock.getChild(1) instanceof ASTBlock); + Assert.assertEquals(2, switchLabeledBlock.getNumChildren()); + Assert.assertTrue(switchLabeledBlock.getChild(0) instanceof ASTSwitchLabel); + Assert.assertTrue(switchLabeledBlock.getChild(1) instanceof ASTBlock); ASTSwitchLabeledThrowStatement switchLabeledThrowStatement = (ASTSwitchLabeledThrowStatement) switchStatement.getChild(5); - assertEquals(2, switchLabeledThrowStatement.getNumChildren()); - assertTrue(switchLabeledThrowStatement.getChild(0) instanceof ASTSwitchLabel); - assertTrue(switchLabeledThrowStatement.getChild(1) instanceof ASTThrowStatement); + Assert.assertEquals(2, switchLabeledThrowStatement.getNumChildren()); + Assert.assertTrue(switchLabeledThrowStatement.getChild(0) instanceof ASTSwitchLabel); + Assert.assertTrue(switchLabeledThrowStatement.getChild(1) instanceof ASTThrowStatement); } @Test @@ -192,13 +190,13 @@ public class Java14Test { private void simpleSwitchExpressions(JavaParsingHelper parser) { ASTCompilationUnit compilationUnit = parser.parseResource("SimpleSwitchExpressions.java"); ASTSwitchExpression switchExpression = compilationUnit.getFirstDescendantOfType(ASTSwitchExpression.class); - assertEquals(6, switchExpression.getNumChildren()); - assertTrue(switchExpression.getChild(0) instanceof ASTExpression); - assertEquals(5, switchExpression.findChildrenOfType(ASTSwitchLabeledRule.class).size()); + Assert.assertEquals(6, switchExpression.getNumChildren()); + Assert.assertTrue(switchExpression.getChild(0) instanceof ASTExpression); + Assert.assertEquals(5, switchExpression.findChildrenOfType(ASTSwitchLabeledRule.class).size()); ASTLocalVariableDeclaration localVar = compilationUnit.findDescendantsOfType(ASTLocalVariableDeclaration.class).get(1); ASTVariableDeclarator localVarDecl = localVar.getFirstChildOfType(ASTVariableDeclarator.class); - assertEquals(Integer.TYPE, localVarDecl.getType()); - assertEquals(Integer.TYPE, switchExpression.getType()); + Assert.assertEquals(Integer.TYPE, localVarDecl.getType()); + Assert.assertEquals(Integer.TYPE, switchExpression.getType()); } } From 961e2b224470e23638b00bc81f4340a3dee0757a Mon Sep 17 00:00:00 2001 From: Andreas Dangel <andreas.dangel@pmd-code.org> Date: Sun, 29 Mar 2020 17:25:11 +0200 Subject: [PATCH 17/56] [doc] Update release notes, fixes #2390 --- 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..6f8708d728 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -36,6 +36,8 @@ 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 +* java-design + * [#2390](https://github.com/pmd/pmd/issues/2390): \[java] AbstractClassWithoutAnyMethod: missing violation for nested classes ### API Changes From e58eb0f017a3c89b2f3807046ac7422fe1287a9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= <clement.fournier76@gmail.com> Date: Mon, 30 Mar 2020 09:55:54 +0200 Subject: [PATCH 18/56] Fix UseDiamondOperator Fixes #1723 --- .../resources/category/java/codestyle.xml | 15 ++- .../rule/codestyle/xml/UseDiamondOperator.xml | 99 +++++++++++++------ 2 files changed, 75 insertions(+), 39 deletions(-) diff --git a/pmd-java/src/main/resources/category/java/codestyle.xml b/pmd-java/src/main/resources/category/java/codestyle.xml index 6f8e3e2ae4..7dca69eaa0 100644 --- a/pmd-java/src/main/resources/category/java/codestyle.xml +++ b/pmd-java/src/main/resources/category/java/codestyle.xml @@ -1967,18 +1967,17 @@ which makes the code also more readable. </description> <priority>3</priority> <properties> + <property name="version" value="2.0" /> <property name="xpath"> <value> <![CDATA[ -//VariableInitializer[preceding-sibling::VariableDeclaratorId[1]/@TypeInferred="false"] -//PrimaryExpression[not(PrimarySuffix)] -[not(ancestor::ArgumentList)] -/PrimaryPrefix/AllocationExpression[ClassOrInterfaceType[@AnonymousClass='false']/TypeArguments//ReferenceType[not(.//TypeArguments)]] +( +//VariableInitializer[preceding-sibling::VariableDeclaratorId[1]/@TypeInferred=false()] | -//StatementExpression[AssignmentOperator][PrimaryExpression/PrimaryPrefix[not(Expression)]] -//PrimaryExpression[not(PrimarySuffix)] -[not(ancestor::ArgumentList)] -/PrimaryPrefix/AllocationExpression[ClassOrInterfaceType[@AnonymousClass='false']/TypeArguments//ReferenceType[not(.//TypeArguments)]] +//StatementExpression[AssignmentOperator and PrimaryExpression/PrimaryPrefix[not(Expression)]] +) +/Expression/PrimaryExpression[not(PrimarySuffix) and not(ancestor::ArgumentList)] +/PrimaryPrefix/AllocationExpression[@AnonymousClass=false() and ClassOrInterfaceType/TypeArguments[@Diamond=false()]] ]]> </value> </property> diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UseDiamondOperator.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UseDiamondOperator.xml index 7d61c4a29a..03b7df7862 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UseDiamondOperator.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UseDiamondOperator.xml @@ -1,13 +1,12 @@ <?xml version="1.0" encoding="UTF-8"?> -<test-data - xmlns="http://pmd.sourceforge.net/rule-tests" - xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" - xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests http://pmd.sourceforge.net/rule-tests_1_0_0.xsd"> - <test-code> - <description>Use Diamond</description> - <expected-problems>2</expected-problems> - <expected-linenumbers>6,8</expected-linenumbers> - <code><![CDATA[ +<test-data xmlns="http://pmd.sourceforge.net/rule-tests" + xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" + xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests http://pmd.sourceforge.net/rule-tests_1_0_0.xsd"> + <test-code> + <description>Use Diamond</description> + <expected-problems>3</expected-problems> + <expected-linenumbers>6,9,10</expected-linenumbers> + <code><![CDATA[ import java.util.ArrayList; import java.util.List; public class Foo { @@ -15,15 +14,19 @@ public class Foo { public void foo() { List<String> strings = new ArrayList<String>(); List<String> strings2 = new ArrayList<>(); + List<List<String>> strings3 = new ArrayList<>(); + List<List<String>> strings4 = new ArrayList<List<List<String>>>(); this.field = new ArrayList<String>(); } } - ]]></code> - </test-code> - <test-code> - <description>False positive cases: anonymous classes, methods calls</description> - <expected-problems>0</expected-problems> - <code><![CDATA[ + ]]></code> + </test-code> + + <test-code> + <description>False positive cases: anonymous classes, methods calls</description> + <expected-problems>1</expected-problems> + <expected-linenumbers>27</expected-linenumbers> + <code><![CDATA[ public class Foo { private WeakReference<Class<?>> typeReference; public void foo() { @@ -58,12 +61,13 @@ public class Foo { } } ]]></code> - </test-code> - <test-code> - <description>#1624[java] UseDiamondOperator doesn't work with var</description> - <expected-problems>1</expected-problems> - <expected-linenumbers>6</expected-linenumbers> - <code><![CDATA[ + </test-code> + + <test-code> + <description>#1624[java] UseDiamondOperator doesn't work with var</description> + <expected-problems>1</expected-problems> + <expected-linenumbers>6</expected-linenumbers> + <code><![CDATA[ import java.util.ArrayList; public class Buzz { public void buzz() { @@ -72,13 +76,14 @@ public class Buzz { f = new ArrayList<String>(); // flagged by rule } } - ]]></code> - </test-code> - <test-code> - <description>Multiple initializations in a single declaration</description> - <expected-problems>1</expected-problems> - <expected-linenumbers>6</expected-linenumbers> - <code><![CDATA[ + ]]></code> + </test-code> + + <test-code> + <description>Multiple initializations in a single declaration</description> + <expected-problems>1</expected-problems> + <expected-linenumbers>6</expected-linenumbers> + <code><![CDATA[ import java.util.ArrayList; import java.util.List; public class Buzz { @@ -88,6 +93,38 @@ public class Buzz { baz = new ArrayList<>(); // ok } } - ]]></code> - </test-code> -</test-data> + ]]></code> + </test-code> + + <test-code> + <description>#1723 FP with var inside lambda (declaration)</description> + <expected-problems>0</expected-problems> + <code><![CDATA[ +class Foo { + { + Runnable someAction = () -> { + var foo = new ArrayList<String>(5); // ok + System.err.println(foo); + }; + } +} + ]]></code> + </test-code> + + <test-code> + <description>#1723 FP with var inside lambda (assignment)</description> + <expected-problems>0</expected-problems> + <code><![CDATA[ +class Foo { + { + Runnable someAction; + someAction = () -> { + var foo = new ArrayList<String>(5); // ok + System.err.println(foo); + }; + } +} + ]]></code> + </test-code> + +</test-data> \ No newline at end of file From eb7949045ffac0d7f6b2ca530ce937f1e02706d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= <clement.fournier76@gmail.com> Date: Mon, 30 Mar 2020 10:12:37 +0200 Subject: [PATCH 19/56] Isolate version-sensitive tests --- .../resources/category/java/codestyle.xml | 2 +- .../rule/codestyle/xml/UseDiamondOperator.xml | 54 ++++++++++++++++--- 2 files changed, 48 insertions(+), 8 deletions(-) diff --git a/pmd-java/src/main/resources/category/java/codestyle.xml b/pmd-java/src/main/resources/category/java/codestyle.xml index 7dca69eaa0..b2495ee404 100644 --- a/pmd-java/src/main/resources/category/java/codestyle.xml +++ b/pmd-java/src/main/resources/category/java/codestyle.xml @@ -1977,7 +1977,7 @@ which makes the code also more readable. //StatementExpression[AssignmentOperator and PrimaryExpression/PrimaryPrefix[not(Expression)]] ) /Expression/PrimaryExpression[not(PrimarySuffix) and not(ancestor::ArgumentList)] -/PrimaryPrefix/AllocationExpression[@AnonymousClass=false() and ClassOrInterfaceType/TypeArguments[@Diamond=false()]] +/PrimaryPrefix/AllocationExpression[@AnonymousClass=false() and ClassOrInterfaceType/TypeArguments[@Diamond=false() and not(TypeArgument//TypeArguments)]] ]]> </value> </property> diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UseDiamondOperator.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UseDiamondOperator.xml index 03b7df7862..563e479afd 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UseDiamondOperator.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UseDiamondOperator.xml @@ -4,8 +4,8 @@ xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests http://pmd.sourceforge.net/rule-tests_1_0_0.xsd"> <test-code> <description>Use Diamond</description> - <expected-problems>3</expected-problems> - <expected-linenumbers>6,9,10</expected-linenumbers> + <expected-problems>2</expected-problems> + <expected-linenumbers>6,11</expected-linenumbers> <code><![CDATA[ import java.util.ArrayList; import java.util.List; @@ -15,6 +15,7 @@ public class Foo { List<String> strings = new ArrayList<String>(); List<String> strings2 = new ArrayList<>(); List<List<String>> strings3 = new ArrayList<>(); + // this is a known false negative, see at the bottom List<List<String>> strings4 = new ArrayList<List<List<String>>>(); this.field = new ArrayList<String>(); } @@ -24,11 +25,9 @@ public class Foo { <test-code> <description>False positive cases: anonymous classes, methods calls</description> - <expected-problems>1</expected-problems> - <expected-linenumbers>27</expected-linenumbers> + <expected-problems>0</expected-problems> <code><![CDATA[ public class Foo { - private WeakReference<Class<?>> typeReference; public void foo() { Collections.sort(files, new Comparator<DataSource>() { @Override @@ -52,8 +51,6 @@ public class Foo { } }; Iterator<Node> EMPTY_ITERATOR = new ArrayList<Node>().iterator(); - Class<?> type = null; - typeReference = new WeakReference<Class<?>>(type); ((ListNode<E>) rev).reverseCache = new SoftReference<ImmutableList<E>>(this); } public Map<PropertyDescriptor<?>, Object> getOverriddenPropertiesByPropertyDescriptor() { @@ -127,4 +124,47 @@ class Foo { ]]></code> </test-code> + <!-- These tests depend on the Java version used --> + <!-- For now we keep the old behaviour of ignoring type + arguments that have type arguments themselves, ie we have + false negatives. We can improve that with better type resolution + in PMD 7. --> + + <test-code regressionTest="false"> + <description>(J7) Version sensitive tests</description> + <expected-problems>1</expected-problems> + <expected-linenumbers>6</expected-linenumbers> + <code><![CDATA[ +public class Foo { + private WeakReference<Class<?>> typeReference; + public void foo() { + // this should be positive in Java 8, negative in Java 7 + // this is because in java 7, new WeakReference<>(String.class) types as WeakReference<Class<String>> + // which is incompatible with WeakReference<Class<?>>, whereas Java 8's type inference is better. + typeReference = new WeakReference<Class<?>>(String.class); + Class<?> type = null; + typeReference = new WeakReference<Class<?>>(type); // this should be positive on all versions + } +} + ]]></code> + <source-type>java 1.7</source-type> + </test-code> + + <test-code regressionTest="false"> + <description>(J8) Version sensitive tests</description> + <expected-problems>2</expected-problems> + <expected-linenumbers>4,6</expected-linenumbers> + <code><![CDATA[ +public class Foo { + private WeakReference<Class<?>> typeReference; + public void foo() { + typeReference = new WeakReference<Class<?>>(String.class); // pos + Class<?> type = null; + typeReference = new WeakReference<Class<?>>(type); // pos + } +} + ]]></code> + <source-type>java 1.8</source-type> + </test-code> + </test-data> \ No newline at end of file From d95763c5042e59bc2c473928dfb7a271daec4708 Mon Sep 17 00:00:00 2001 From: Gwilym Kuiper <gwilym@gearset.com> Date: Mon, 30 Mar 2020 10:54:58 +0100 Subject: [PATCH 20/56] Basic implementation of detecting apex unused local variables --- .../UnusedLocalVariableRule.java | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableRule.java diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableRule.java new file mode 100644 index 0000000000..0afc2ca2f6 --- /dev/null +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableRule.java @@ -0,0 +1,27 @@ +package net.sourceforge.pmd.lang.apex.rule.bestpractices; + +import net.sourceforge.pmd.lang.apex.ast.ASTMethod; +import net.sourceforge.pmd.lang.apex.ast.ASTVariableDeclaration; +import net.sourceforge.pmd.lang.apex.ast.ASTVariableExpression; +import net.sourceforge.pmd.lang.apex.rule.AbstractApexRule; + +import java.util.List; + +public class UnusedLocalVariableRule extends AbstractApexRule { + @Override + public Object visit(ASTVariableDeclaration node, Object data) { + String variableName = node.getImage(); + + ASTMethod containerMethod = node.getFirstParentOfType(ASTMethod.class); + List<ASTVariableExpression> potentialUsages = containerMethod.findChildrenOfType(ASTVariableExpression.class); + + for (ASTVariableExpression usage : potentialUsages) { + if (usage.getImage().equals(variableName)) { + return data; + } + } + + addViolation(data, node); + return data; + } +} From 4d1735874680155a5c3ad5a9544d75518e01d268 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= <clement.fournier76@gmail.com> Date: Mon, 30 Mar 2020 16:48:42 +0200 Subject: [PATCH 21/56] Fix FP with array creation --- .../main/resources/category/java/codestyle.xml | 6 +++++- .../rule/codestyle/xml/UseDiamondOperator.xml | 16 ++++++++++++++-- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/pmd-java/src/main/resources/category/java/codestyle.xml b/pmd-java/src/main/resources/category/java/codestyle.xml index b2495ee404..c8e273d530 100644 --- a/pmd-java/src/main/resources/category/java/codestyle.xml +++ b/pmd-java/src/main/resources/category/java/codestyle.xml @@ -1977,7 +1977,11 @@ which makes the code also more readable. //StatementExpression[AssignmentOperator and PrimaryExpression/PrimaryPrefix[not(Expression)]] ) /Expression/PrimaryExpression[not(PrimarySuffix) and not(ancestor::ArgumentList)] -/PrimaryPrefix/AllocationExpression[@AnonymousClass=false() and ClassOrInterfaceType/TypeArguments[@Diamond=false() and not(TypeArgument//TypeArguments)]] +/PrimaryPrefix +/AllocationExpression + [@AnonymousClass=false()] + [ClassOrInterfaceType/TypeArguments[@Diamond=false() and not(TypeArgument//TypeArguments)]] + [not(ArrayDimsAndInits)] ]]> </value> </property> diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UseDiamondOperator.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UseDiamondOperator.xml index 563e479afd..b65723cb19 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UseDiamondOperator.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UseDiamondOperator.xml @@ -124,6 +124,18 @@ class Foo { ]]></code> </test-code> + <test-code> + <description>FP with array creation</description> + <expected-problems>0</expected-problems> + <code><![CDATA[ +class Foo { + { + Class<?> c = new Class<?>[0]; + } +} + ]]></code> + </test-code> + <!-- These tests depend on the Java version used --> <!-- For now we keep the old behaviour of ignoring type arguments that have type arguments themselves, ie we have @@ -147,7 +159,7 @@ public class Foo { } } ]]></code> - <source-type>java 1.7</source-type> + <source-type>java 1.7</source-type> </test-code> <test-code regressionTest="false"> @@ -164,7 +176,7 @@ public class Foo { } } ]]></code> - <source-type>java 1.8</source-type> + <source-type>java 1.8</source-type> </test-code> </test-data> \ No newline at end of file From 29cef7f23dc3deb8346b7210c2acb9f948edd917 Mon Sep 17 00:00:00 2001 From: Gwilym Kuiper <gwilym@gearset.com> Date: Mon, 30 Mar 2020 10:59:06 +0100 Subject: [PATCH 22/56] Add rule to bestpractices.xml --- .../resources/category/apex/bestpractices.xml | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/pmd-apex/src/main/resources/category/apex/bestpractices.xml b/pmd-apex/src/main/resources/category/apex/bestpractices.xml index 9d2913a771..ecef669384 100644 --- a/pmd-apex/src/main/resources/category/apex/bestpractices.xml +++ b/pmd-apex/src/main/resources/category/apex/bestpractices.xml @@ -208,4 +208,25 @@ public class Foo { </example> </rule> + <rule name="UnusedLocalVariable" + since="6.23.0" + language="apex" + message="Variable defined but not used" + class="net.sourceforge.pmd.lang.apex.rule.bestpractices.UnusedLocalVariableRule" + externalInfoUrl="${pmd.website.baseurl}/pmd_rules_apex_bestpractices.html#unusedlocalvariable"> + <description> +Detects when a local variable is declared and/or assigned but not used. + </description> + <example> +<![CDATA[ + public Boolean bar(String z) { + String x = 'some string'; // not used + + String y = 'some other string'; // used in the next line + return z.equals(y); + } +]]> + </example> + </rule> + </ruleset> From 80c26bf537717b96844067d4dd9e2f10ccb67c75 Mon Sep 17 00:00:00 2001 From: Gwilym Kuiper <gwilym@gearset.com> Date: Mon, 30 Mar 2020 11:44:09 +0100 Subject: [PATCH 23/56] Add tests and fix up issues found while writing them --- .../UnusedLocalVariableRule.java | 9 ++-- .../UnusedLocalVariableTest.java | 7 +++ .../bestpractices/xml/UnusedLocalVariable.xml | 44 +++++++++++++++++++ 3 files changed, 57 insertions(+), 3 deletions(-) create mode 100644 pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableTest.java create mode 100644 pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/UnusedLocalVariable.xml diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableRule.java index 0afc2ca2f6..e4c31520fc 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableRule.java @@ -1,6 +1,6 @@ package net.sourceforge.pmd.lang.apex.rule.bestpractices; -import net.sourceforge.pmd.lang.apex.ast.ASTMethod; +import net.sourceforge.pmd.lang.apex.ast.ASTBlockStatement; import net.sourceforge.pmd.lang.apex.ast.ASTVariableDeclaration; import net.sourceforge.pmd.lang.apex.ast.ASTVariableExpression; import net.sourceforge.pmd.lang.apex.rule.AbstractApexRule; @@ -12,10 +12,13 @@ public class UnusedLocalVariableRule extends AbstractApexRule { public Object visit(ASTVariableDeclaration node, Object data) { String variableName = node.getImage(); - ASTMethod containerMethod = node.getFirstParentOfType(ASTMethod.class); - List<ASTVariableExpression> potentialUsages = containerMethod.findChildrenOfType(ASTVariableExpression.class); + ASTBlockStatement variableContext = node.getFirstParentOfType(ASTBlockStatement.class); + List<ASTVariableExpression> potentialUsages = variableContext.findDescendantsOfType(ASTVariableExpression.class); for (ASTVariableExpression usage : potentialUsages) { + if (usage.getParent() == node) { + continue; + } if (usage.getImage().equals(variableName)) { return data; } diff --git a/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableTest.java b/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableTest.java new file mode 100644 index 0000000000..a66f9095b1 --- /dev/null +++ b/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableTest.java @@ -0,0 +1,7 @@ +package net.sourceforge.pmd.lang.apex.rule.bestpractices; + +import net.sourceforge.pmd.testframework.PmdRuleTst; + +public class UnusedLocalVariableTest extends PmdRuleTst { + // no additional unit tests +} diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/UnusedLocalVariable.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/UnusedLocalVariable.xml new file mode 100644 index 0000000000..0377753927 --- /dev/null +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/UnusedLocalVariable.xml @@ -0,0 +1,44 @@ +<?xml version="1.0" encoding="utf-8" ?> +<test-data + xmlns="http://pmd.sourceforge.net/rule-tests" + xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" + xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests http://pmd.sourceforge.net/rule-tests_1_0_0.xsd"> + <test-code> + <description>Unused variables should result in errors</description> + <expected-problems>2</expected-problems> + <expected-linenumbers>3,7</expected-linenumbers> + <code> +<![CDATA[ +public class Foo { + public void assignedVariable() { + String foo = 'unused string'; + } + + public void justADeclaration() { + String foo; + } +} +]]> + </code> + </test-code> + + <test-code> + <description>Used variables should not result in errors</description> + <expected-problems>0</expected-problems> + <code> +<![CDATA[ +public class Foo { + public String basicUsage() { + String x = 'used variable'; + return x; + } + + public Account moreComplexUsage() { + String x = 'blah'; + return [SELECT Id FROM Account WHERE Name = :x]; + } +} +]]> + </code> + </test-code> +</test-data> \ No newline at end of file From 705fa2563adc473b7b8bbac34804054fe7a8b405 Mon Sep 17 00:00:00 2001 From: Gwilym Kuiper <gwilym@gearset.com> Date: Wed, 1 Apr 2020 09:22:03 +0100 Subject: [PATCH 24/56] Fixup checkstyle issues --- .../apex/rule/bestpractices/UnusedLocalVariableRule.java | 8 ++++++-- .../apex/rule/bestpractices/UnusedLocalVariableTest.java | 4 ++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableRule.java index e4c31520fc..ee3b7579e2 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableRule.java @@ -1,12 +1,16 @@ +/** + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + package net.sourceforge.pmd.lang.apex.rule.bestpractices; +import java.util.List; + import net.sourceforge.pmd.lang.apex.ast.ASTBlockStatement; import net.sourceforge.pmd.lang.apex.ast.ASTVariableDeclaration; import net.sourceforge.pmd.lang.apex.ast.ASTVariableExpression; import net.sourceforge.pmd.lang.apex.rule.AbstractApexRule; -import java.util.List; - public class UnusedLocalVariableRule extends AbstractApexRule { @Override public Object visit(ASTVariableDeclaration node, Object data) { diff --git a/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableTest.java b/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableTest.java index a66f9095b1..cef1960afc 100644 --- a/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableTest.java +++ b/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableTest.java @@ -1,3 +1,7 @@ +/* + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + package net.sourceforge.pmd.lang.apex.rule.bestpractices; import net.sourceforge.pmd.testframework.PmdRuleTst; From abbe46c7ba3c15d8dcbc1b603466e5c25754cbbd Mon Sep 17 00:00:00 2001 From: Gwilym Kuiper <gwilym@gearset.com> Date: Wed, 1 Apr 2020 09:25:09 +0100 Subject: [PATCH 25/56] Add another test for variable usage in blocks --- .../rule/bestpractices/xml/UnusedLocalVariable.xml | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/UnusedLocalVariable.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/UnusedLocalVariable.xml index 0377753927..4e21cf9b26 100644 --- a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/UnusedLocalVariable.xml +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/UnusedLocalVariable.xml @@ -37,6 +37,16 @@ public class Foo { String x = 'blah'; return [SELECT Id FROM Account WHERE Name = :x]; } + + public String usageInBlocks(Boolean y) { + String x = 'used variable'; + + if (y) { + return x; + } else { + return 'some other string'; + } + } } ]]> </code> From 8acb63870cc6143130192ee7102959bde26da94b Mon Sep 17 00:00:00 2001 From: Gwilym Kuiper <gwilym@gearset.com> Date: Wed, 1 Apr 2020 11:24:34 +0100 Subject: [PATCH 26/56] Add basic implmentation of FieldDeclarationsShouldBeAtStart for apex --- .../FieldDeclarationsShouldBeAtStartRule.java | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java new file mode 100644 index 0000000000..d389a8f8a8 --- /dev/null +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java @@ -0,0 +1,67 @@ +/** + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.apex.rule.codestyle; + +import java.util.Comparator; +import java.util.List; +import java.util.Optional; + +import net.sourceforge.pmd.lang.apex.ast.ASTFieldDeclaration; +import net.sourceforge.pmd.lang.apex.ast.ASTMethod; +import net.sourceforge.pmd.lang.apex.ast.ASTUserClass; +import net.sourceforge.pmd.lang.apex.ast.ApexNode; +import net.sourceforge.pmd.lang.apex.rule.AbstractApexRule; + +public class FieldDeclarationsShouldBeAtStartRule extends AbstractApexRule { + @Override + public Object visit(ASTUserClass node, Object data) { + // Unfortunately the parser re-orders the AST to put field declarations before method declarations + // so we have to rely on line numbers / positions to work out where the first method starts so we + // can check if the fields are in acceptable places. + List<ASTFieldDeclaration> fields = node.findDescendantsOfType(ASTFieldDeclaration.class); + List<ASTMethod> methods = node.findDescendantsOfType(ASTMethod.class); + + Optional<NodeAndLocation> firstMethod = + methods.stream().map(NodeAndLocation::new) + .min(Comparator.naturalOrder()); + + if (!firstMethod.isPresent()) { + // there are no methods so the field declaration has to come first + return data; + } + + for (ASTFieldDeclaration field : fields) { + NodeAndLocation fieldPosition = new NodeAndLocation(field); + if (fieldPosition.compareTo(firstMethod.get()) > 0) { + addViolation(data, field); + } + } + + return data; + } + + private static class NodeAndLocation implements Comparable<NodeAndLocation> { + public int line; + public int column; + public ApexNode<?> node; + + public NodeAndLocation(ApexNode<?> node) { + this.node = node; + + line = node.getBeginLine(); + column = node.getBeginColumn(); + } + + @Override + public int compareTo(NodeAndLocation other) { + int lineCompare = Integer.compare(line, other.line); + if (lineCompare != 0) { + return lineCompare; + } + + return Integer.compare(column, other.column); + } + } +} From 1d62a1f984094621c5450e727f671ec5d523c6ec Mon Sep 17 00:00:00 2001 From: Gwilym Kuiper <gwilym@gearset.com> Date: Wed, 1 Apr 2020 11:28:14 +0100 Subject: [PATCH 27/56] Add rule definition in codestyle.xml --- .../resources/category/apex/codestyle.xml | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/pmd-apex/src/main/resources/category/apex/codestyle.xml b/pmd-apex/src/main/resources/category/apex/codestyle.xml index 262b6f9757..d5df6de277 100644 --- a/pmd-apex/src/main/resources/category/apex/codestyle.xml +++ b/pmd-apex/src/main/resources/category/apex/codestyle.xml @@ -365,4 +365,27 @@ while (true) { // preferred approach </example> </rule> + <rule name="FieldDeclarationsShouldBeAtStart" + language="apex" + since="5.23.0" + message="Field declarations should be before method declarations in a class" + class="net.sourceforge.pmd.lang.apex.rule.codestyle.FieldDeclarationsShouldBeAtStartRule" + externalInfoUrl="${pmd.website.baseurl}/pmd_rules_apex_codestyle.html#fielddeclarationsshouldbeatstart"> + <description> + Field declarations should appear before method declarations within a class. + </description> + <priority>3</priority> + <example> +<![CDATA[ +class Foo { + public Integer someField; // good + + public void someMethod() { + } + + public Integer anotherField; // bad +} +]]> + </example> + </rule> </ruleset> From 3f0dc27a4a77514ae73665823761cb9950f235fb Mon Sep 17 00:00:00 2001 From: Gwilym Kuiper <gwilym@gearset.com> Date: Wed, 1 Apr 2020 11:47:32 +0100 Subject: [PATCH 28/56] Add tests for apex field declarations should be at start --- .../FieldDeclarationsShouldBeAtStartRule.java | 6 +- .../FieldDeclarationsShouldBeAtStartTest.java | 11 +++ .../xml/FieldDeclarationsShouldBeAtStart.xml | 94 +++++++++++++++++++ 3 files changed, 109 insertions(+), 2 deletions(-) create mode 100644 pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartTest.java create mode 100644 pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java index d389a8f8a8..eff6412f9c 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java @@ -24,7 +24,9 @@ public class FieldDeclarationsShouldBeAtStartRule extends AbstractApexRule { List<ASTMethod> methods = node.findDescendantsOfType(ASTMethod.class); Optional<NodeAndLocation> firstMethod = - methods.stream().map(NodeAndLocation::new) + methods.stream() + .filter(method -> method.hasRealLoc()) + .map(method -> new NodeAndLocation(method)) .min(Comparator.naturalOrder()); if (!firstMethod.isPresent()) { @@ -47,7 +49,7 @@ public class FieldDeclarationsShouldBeAtStartRule extends AbstractApexRule { public int column; public ApexNode<?> node; - public NodeAndLocation(ApexNode<?> node) { + NodeAndLocation(ApexNode<?> node) { this.node = node; line = node.getBeginLine(); diff --git a/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartTest.java b/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartTest.java new file mode 100644 index 0000000000..c22140d52d --- /dev/null +++ b/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartTest.java @@ -0,0 +1,11 @@ +/** + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.apex.rule.codestyle; + +import net.sourceforge.pmd.testframework.PmdRuleTst; + +public class FieldDeclarationsShouldBeAtStartTest extends PmdRuleTst { + // no additional unit tests +} diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml new file mode 100644 index 0000000000..b7b9549086 --- /dev/null +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml @@ -0,0 +1,94 @@ +<?xml version="1.0" encoding="UTF-8"?> + +<test-data + xmlns="http://pmd.sourceforge.net/rule-tests" + xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" + xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests http://pmd.sourceforge.net/rule-tests_1_0_0.xsd"> + <test-code> + <description>Does not warn if there are no methods</description> + <expected-problems>0</expected-problems> + <code> +<![CDATA[ +class Foo { + public Integer thisIsOkay; +} +]]> + </code> + </test-code> + + <test-code> + <description>Does warn if a field is after a method</description> + <expected-problems>1</expected-problems> + <expected-linenumbers>4</expected-linenumbers> + <code> +<![CDATA[ +class Foo { + public void someMethod() {} + + public Integer thisIsNotOkay; +} +]]> + </code> + </test-code> + + <test-code> + <description>Warns if field is after constructor</description> + <expected-problems>1</expected-problems> + <expected-linenumbers>6</expected-linenumbers> + <code> +<![CDATA[ +class Foo { + public Foo(Integer someValue) { + someField = someValue; + } + + private Integer someField; +} +]]> + </code> + </test-code> + + <test-code> + <description>Warns only for fields after the first method declaration</description> + <expected-problems>1</expected-problems> + <expected-linenumbers>8</expected-linenumbers> + <code> +<![CDATA[ +class Foo { + private Integer thisFieldIsOkay; + + public Foo(Integer someValue) { + someField = someValue; + } + + private Integer thisFieldIsNotOkay; +} +]]> + </code> + </test-code> + + <test-code> + <description>Warns for fields defined on the same line after a method</description> + <expected-problems>1</expected-problems> + <expected-linenumbers>2</expected-linenumbers> + <code> +<![CDATA[ +class Foo { + public Foo(Integer someValue) { someField = someValue; } private Integer thisFieldIsNotOkay; +} +]]> + </code> + </test-code> + + <test-code> + <description>Does not warn for fields defined on the same line before a method</description> + <expected-problems>0</expected-problems> + <code> + <![CDATA[ +class Foo { + private Integer thisFieldIsOkay; public Foo(Integer someValue) { someField = someValue; } +} +]]> + </code> + </test-code> +</test-data> \ No newline at end of file From 504cf440a32023d95e85488debcdde4a33f1df9d Mon Sep 17 00:00:00 2001 From: Gwilym Kuiper <gwilym@gearset.com> Date: Wed, 1 Apr 2020 11:55:50 +0100 Subject: [PATCH 29/56] Inculde field name in violation message --- .../FieldDeclarationsShouldBeAtStartRule.java | 2 +- .../src/main/resources/category/apex/codestyle.xml | 2 +- .../xml/FieldDeclarationsShouldBeAtStart.xml | 12 ++++++++++++ 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java index eff6412f9c..95473f2e0e 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java @@ -37,7 +37,7 @@ public class FieldDeclarationsShouldBeAtStartRule extends AbstractApexRule { for (ASTFieldDeclaration field : fields) { NodeAndLocation fieldPosition = new NodeAndLocation(field); if (fieldPosition.compareTo(firstMethod.get()) > 0) { - addViolation(data, field); + addViolation(data, field, field.getName()); } } diff --git a/pmd-apex/src/main/resources/category/apex/codestyle.xml b/pmd-apex/src/main/resources/category/apex/codestyle.xml index d5df6de277..a28d42ff9f 100644 --- a/pmd-apex/src/main/resources/category/apex/codestyle.xml +++ b/pmd-apex/src/main/resources/category/apex/codestyle.xml @@ -368,7 +368,7 @@ while (true) { // preferred approach <rule name="FieldDeclarationsShouldBeAtStart" language="apex" since="5.23.0" - message="Field declarations should be before method declarations in a class" + message="Field declaration for ''{0}'' should be before method declarations in its class" class="net.sourceforge.pmd.lang.apex.rule.codestyle.FieldDeclarationsShouldBeAtStartRule" externalInfoUrl="${pmd.website.baseurl}/pmd_rules_apex_codestyle.html#fielddeclarationsshouldbeatstart"> <description> diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml index b7b9549086..de3957117e 100644 --- a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml @@ -20,6 +20,9 @@ class Foo { <description>Does warn if a field is after a method</description> <expected-problems>1</expected-problems> <expected-linenumbers>4</expected-linenumbers> + <expected-messages> + <message>Field declaration for 'thisIsNotOkay' should be before method declarations in its class</message> + </expected-messages> <code> <![CDATA[ class Foo { @@ -35,6 +38,9 @@ class Foo { <description>Warns if field is after constructor</description> <expected-problems>1</expected-problems> <expected-linenumbers>6</expected-linenumbers> + <expected-messages> + <message>Field declaration for 'someField' should be before method declarations in its class</message> + </expected-messages> <code> <![CDATA[ class Foo { @@ -52,6 +58,9 @@ class Foo { <description>Warns only for fields after the first method declaration</description> <expected-problems>1</expected-problems> <expected-linenumbers>8</expected-linenumbers> + <expected-messages> + <message>Field declaration for 'thisFieldIsNotOkay' should be before method declarations in its class</message> + </expected-messages> <code> <![CDATA[ class Foo { @@ -71,6 +80,9 @@ class Foo { <description>Warns for fields defined on the same line after a method</description> <expected-problems>1</expected-problems> <expected-linenumbers>2</expected-linenumbers> + <expected-messages> + <message>Field declaration for 'thisFieldIsNotOkay' should be before method declarations in its class</message> + </expected-messages> <code> <![CDATA[ class Foo { From 41f6a595a9ad9d116447fa63d9a69a84a343dd37 Mon Sep 17 00:00:00 2001 From: Gwilym Kuiper <gwilym@gearset.com> Date: Wed, 1 Apr 2020 11:58:32 +0100 Subject: [PATCH 30/56] Include variable name in violation message --- .../lang/apex/rule/bestpractices/UnusedLocalVariableRule.java | 2 +- pmd-apex/src/main/resources/category/apex/bestpractices.xml | 2 +- .../lang/apex/rule/bestpractices/xml/UnusedLocalVariable.xml | 4 ++++ 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableRule.java index ee3b7579e2..661befda13 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableRule.java @@ -28,7 +28,7 @@ public class UnusedLocalVariableRule extends AbstractApexRule { } } - addViolation(data, node); + addViolation(data, node, variableName); return data; } } diff --git a/pmd-apex/src/main/resources/category/apex/bestpractices.xml b/pmd-apex/src/main/resources/category/apex/bestpractices.xml index ecef669384..f91fa5095a 100644 --- a/pmd-apex/src/main/resources/category/apex/bestpractices.xml +++ b/pmd-apex/src/main/resources/category/apex/bestpractices.xml @@ -211,7 +211,7 @@ public class Foo { <rule name="UnusedLocalVariable" since="6.23.0" language="apex" - message="Variable defined but not used" + message="Variable ''{0}'' defined but not used" class="net.sourceforge.pmd.lang.apex.rule.bestpractices.UnusedLocalVariableRule" externalInfoUrl="${pmd.website.baseurl}/pmd_rules_apex_bestpractices.html#unusedlocalvariable"> <description> diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/UnusedLocalVariable.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/UnusedLocalVariable.xml index 4e21cf9b26..a708821b1a 100644 --- a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/UnusedLocalVariable.xml +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/UnusedLocalVariable.xml @@ -7,6 +7,10 @@ <description>Unused variables should result in errors</description> <expected-problems>2</expected-problems> <expected-linenumbers>3,7</expected-linenumbers> + <expected-messages> + <message>Variable 'foo' defined but not used</message> + <message>Variable 'foo' defined but not used</message> + </expected-messages> <code> <![CDATA[ public class Foo { From 9c13702e3971eead42526d420f8e8489749ff4e7 Mon Sep 17 00:00:00 2001 From: Gwilym Kuiper <gwilym@gearset.com> Date: Fri, 3 Apr 2020 13:38:40 +0100 Subject: [PATCH 31/56] Massively simplify rule with custom comparator --- .../FieldDeclarationsShouldBeAtStartRule.java | 39 +++++-------------- 1 file changed, 9 insertions(+), 30 deletions(-) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java index 95473f2e0e..d97046182b 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java @@ -15,6 +15,11 @@ import net.sourceforge.pmd.lang.apex.ast.ApexNode; import net.sourceforge.pmd.lang.apex.rule.AbstractApexRule; public class FieldDeclarationsShouldBeAtStartRule extends AbstractApexRule { + private static final Comparator<ApexNode<?>> nodeBySourceLocationComparator = + Comparator + .<ApexNode<?>>comparingInt(ApexNode::getBeginLine) + .thenComparing(ApexNode::getBeginColumn); + @Override public Object visit(ASTUserClass node, Object data) { // Unfortunately the parser re-orders the AST to put field declarations before method declarations @@ -23,11 +28,9 @@ public class FieldDeclarationsShouldBeAtStartRule extends AbstractApexRule { List<ASTFieldDeclaration> fields = node.findDescendantsOfType(ASTFieldDeclaration.class); List<ASTMethod> methods = node.findDescendantsOfType(ASTMethod.class); - Optional<NodeAndLocation> firstMethod = - methods.stream() - .filter(method -> method.hasRealLoc()) - .map(method -> new NodeAndLocation(method)) - .min(Comparator.naturalOrder()); + Optional<ASTMethod> firstMethod = methods.stream() + .filter(ApexNode::hasRealLoc) + .min(nodeBySourceLocationComparator); if (!firstMethod.isPresent()) { // there are no methods so the field declaration has to come first @@ -35,35 +38,11 @@ public class FieldDeclarationsShouldBeAtStartRule extends AbstractApexRule { } for (ASTFieldDeclaration field : fields) { - NodeAndLocation fieldPosition = new NodeAndLocation(field); - if (fieldPosition.compareTo(firstMethod.get()) > 0) { + if (nodeBySourceLocationComparator.compare(field, firstMethod.get()) > 0) { addViolation(data, field, field.getName()); } } return data; } - - private static class NodeAndLocation implements Comparable<NodeAndLocation> { - public int line; - public int column; - public ApexNode<?> node; - - NodeAndLocation(ApexNode<?> node) { - this.node = node; - - line = node.getBeginLine(); - column = node.getBeginColumn(); - } - - @Override - public int compareTo(NodeAndLocation other) { - int lineCompare = Integer.compare(line, other.line); - if (lineCompare != 0) { - return lineCompare; - } - - return Integer.compare(column, other.column); - } - } } From eb8e7eabd8a08d95bfe5ded004f7d3d7d73ff20b Mon Sep 17 00:00:00 2001 From: Gwilym Kuiper <gwilym@gearset.com> Date: Fri, 3 Apr 2020 13:45:34 +0100 Subject: [PATCH 32/56] Fix field declaration location rule around nested classes --- .../FieldDeclarationsShouldBeAtStartRule.java | 8 ++++---- .../xml/FieldDeclarationsShouldBeAtStart.xml | 16 ++++++++++++++++ 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java index d97046182b..f8e4c0ac7f 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java @@ -8,7 +8,7 @@ import java.util.Comparator; import java.util.List; import java.util.Optional; -import net.sourceforge.pmd.lang.apex.ast.ASTFieldDeclaration; +import net.sourceforge.pmd.lang.apex.ast.ASTField; import net.sourceforge.pmd.lang.apex.ast.ASTMethod; import net.sourceforge.pmd.lang.apex.ast.ASTUserClass; import net.sourceforge.pmd.lang.apex.ast.ApexNode; @@ -25,8 +25,8 @@ public class FieldDeclarationsShouldBeAtStartRule extends AbstractApexRule { // Unfortunately the parser re-orders the AST to put field declarations before method declarations // so we have to rely on line numbers / positions to work out where the first method starts so we // can check if the fields are in acceptable places. - List<ASTFieldDeclaration> fields = node.findDescendantsOfType(ASTFieldDeclaration.class); - List<ASTMethod> methods = node.findDescendantsOfType(ASTMethod.class); + List<ASTField> fields = node.findChildrenOfType(ASTField.class); + List<ASTMethod> methods = node.findChildrenOfType(ASTMethod.class); Optional<ASTMethod> firstMethod = methods.stream() .filter(ApexNode::hasRealLoc) @@ -37,7 +37,7 @@ public class FieldDeclarationsShouldBeAtStartRule extends AbstractApexRule { return data; } - for (ASTFieldDeclaration field : fields) { + for (ASTField field : fields) { if (nodeBySourceLocationComparator.compare(field, firstMethod.get()) > 0) { addViolation(data, field, field.getName()); } diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml index de3957117e..66a29cb58a 100644 --- a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml @@ -100,6 +100,22 @@ class Foo { class Foo { private Integer thisFieldIsOkay; public Foo(Integer someValue) { someField = someValue; } } +]]> + </code> + </test-code> + + <test-code> + <description>Allows nested classes to have fields</description> + <expected-problems>0</expected-problems> + <code> + <![CDATA[ +class Foo { + void bar() { } + + private class InnerFoo { + public Integer thisIsOkay; + } +} ]]> </code> </test-code> From 1359c88e90314e7aad49f5bc5335275e7371e8ab Mon Sep 17 00:00:00 2001 From: Gwilym Kuiper <gwilym@gearset.com> Date: Fri, 3 Apr 2020 13:48:31 +0100 Subject: [PATCH 33/56] Also produce errors in inner classes --- .../FieldDeclarationsShouldBeAtStartRule.java | 4 ++-- .../xml/FieldDeclarationsShouldBeAtStart.xml | 21 +++++++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java index f8e4c0ac7f..dc1f7f5c1a 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java @@ -34,7 +34,7 @@ public class FieldDeclarationsShouldBeAtStartRule extends AbstractApexRule { if (!firstMethod.isPresent()) { // there are no methods so the field declaration has to come first - return data; + return super.visit(node, data); } for (ASTField field : fields) { @@ -43,6 +43,6 @@ public class FieldDeclarationsShouldBeAtStartRule extends AbstractApexRule { } } - return data; + return super.visit(node, data); } } diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml index 66a29cb58a..55bba1805a 100644 --- a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml @@ -116,6 +116,27 @@ class Foo { public Integer thisIsOkay; } } +]]> + </code> + </test-code> + + <test-code> + <description>Allows nested classes to have fields</description> + <expected-problems>1</expected-problems> + <expected-linenumbers>9</expected-linenumbers> + <code> + <![CDATA[ +class Foo { + void bar() { } + + private class InnerFoo { + public Integer thisIsOkay; + + public void bar() {} + + public Integer thisIsNotOkay; + } +} ]]> </code> </test-code> From 5d1ffa5a55e17cd8a03ce992d588a2653577a6ac Mon Sep 17 00:00:00 2001 From: Gwilym Kuiper <gwilym@gearset.com> Date: Fri, 3 Apr 2020 13:57:34 +0100 Subject: [PATCH 34/56] Correctly detect fields after inner classes --- .../FieldDeclarationsShouldBeAtStartRule.java | 19 ++++++++++++------- .../xml/FieldDeclarationsShouldBeAtStart.xml | 15 +++++++++++++++ 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java index dc1f7f5c1a..f9a1a9dc87 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java @@ -4,6 +4,7 @@ package net.sourceforge.pmd.lang.apex.rule.codestyle; +import java.util.ArrayList; import java.util.Comparator; import java.util.List; import java.util.Optional; @@ -23,22 +24,26 @@ public class FieldDeclarationsShouldBeAtStartRule extends AbstractApexRule { @Override public Object visit(ASTUserClass node, Object data) { // Unfortunately the parser re-orders the AST to put field declarations before method declarations - // so we have to rely on line numbers / positions to work out where the first method starts so we - // can check if the fields are in acceptable places. + // so we have to rely on line numbers / positions to work out where the first non-field declaration starts + // so we can check if the fields are in acceptable places. List<ASTField> fields = node.findChildrenOfType(ASTField.class); - List<ASTMethod> methods = node.findChildrenOfType(ASTMethod.class); - Optional<ASTMethod> firstMethod = methods.stream() + List<ApexNode<?>> nonFieldDeclarations = new ArrayList<>(); + + nonFieldDeclarations.addAll(node.findChildrenOfType(ASTMethod.class)); + nonFieldDeclarations.addAll(node.findChildrenOfType(ASTUserClass.class)); + + Optional<ApexNode<?>> firstNonFieldDeclaration = nonFieldDeclarations.stream() .filter(ApexNode::hasRealLoc) .min(nodeBySourceLocationComparator); - if (!firstMethod.isPresent()) { - // there are no methods so the field declaration has to come first + if (!firstNonFieldDeclaration.isPresent()) { + // there is nothing except field declaration, so that has to come first return super.visit(node, data); } for (ASTField field : fields) { - if (nodeBySourceLocationComparator.compare(field, firstMethod.get()) > 0) { + if (nodeBySourceLocationComparator.compare(field, firstNonFieldDeclaration.get()) > 0) { addViolation(data, field, field.getName()); } } diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml index 55bba1805a..36c37a7d92 100644 --- a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml @@ -137,6 +137,21 @@ class Foo { public Integer thisIsNotOkay; } } +]]> + </code> + </test-code> + + <test-code> + <description>Fields should go before inner classes too</description> + <expected-problems>1</expected-problems> + <expected-linenumbers>4</expected-linenumbers> + <code> +<![CDATA[ +class Foo { + private class InnerFoo {} + + public Integer thisIsNotOkay; +} ]]> </code> </test-code> From eeac5796614a83d2ebc170be33bb631ae4893ccf Mon Sep 17 00:00:00 2001 From: Gwilym Kuiper <gwilym@gearset.com> Date: Fri, 3 Apr 2020 13:58:49 +0100 Subject: [PATCH 35/56] Correctly detect properties before fields --- .../FieldDeclarationsShouldBeAtStartRule.java | 2 ++ .../xml/FieldDeclarationsShouldBeAtStart.xml | 15 +++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java index f9a1a9dc87..736f1a17ad 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java @@ -11,6 +11,7 @@ import java.util.Optional; import net.sourceforge.pmd.lang.apex.ast.ASTField; import net.sourceforge.pmd.lang.apex.ast.ASTMethod; +import net.sourceforge.pmd.lang.apex.ast.ASTProperty; import net.sourceforge.pmd.lang.apex.ast.ASTUserClass; import net.sourceforge.pmd.lang.apex.ast.ApexNode; import net.sourceforge.pmd.lang.apex.rule.AbstractApexRule; @@ -32,6 +33,7 @@ public class FieldDeclarationsShouldBeAtStartRule extends AbstractApexRule { nonFieldDeclarations.addAll(node.findChildrenOfType(ASTMethod.class)); nonFieldDeclarations.addAll(node.findChildrenOfType(ASTUserClass.class)); + nonFieldDeclarations.addAll(node.findChildrenOfType(ASTProperty.class)); Optional<ApexNode<?>> firstNonFieldDeclaration = nonFieldDeclarations.stream() .filter(ApexNode::hasRealLoc) diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml index 36c37a7d92..183bda51af 100644 --- a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml @@ -152,6 +152,21 @@ class Foo { public Integer thisIsNotOkay; } +]]> + </code> + </test-code> + + <test-code> + <description>Fields should go before properties too</description> + <expected-problems>1</expected-problems> + <expected-linenumbers>4</expected-linenumbers> + <code> + <![CDATA[ +class Foo { + public Integer someProperty { get; } + + public Integer thisIsNotOkay; +} ]]> </code> </test-code> From 9385feb0e7ead14c97366563527d7a171cea5b6c Mon Sep 17 00:00:00 2001 From: Andreas Dangel <andreas.dangel@pmd-code.org> Date: Fri, 3 Apr 2020 15:19:43 +0200 Subject: [PATCH 36/56] [doc] Update release notes, refs #2372 --- docs/pages/release_notes.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index bb755f4f26..bc0971faf0 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -95,6 +95,12 @@ implementations, and their corresponding Parser if it exists (in the same packag * {% jdoc matlab::lang.matlab.MatlabTokenManager %} * {% jdoc objectivec::lang.objectivec.ObjectiveCTokenManager %} +##### For removal + +* {% jdoc !!core::lang.Parser#getTokenManager(java.lang.String,java.io.Reader) %} +* {% jdoc !!core::lang.TokenManager#setFileName(java.lang.String) %} +* {% jdoc !!core::lang.ast.AbstractTokenManager#setFileName(java.lang.String) %} +* {% jdoc !!core::lang.ast.AbstractTokenManager#getFileName(java.lang.String) %} ### External Contributions From e30e229cf5e4c99926758e51f8e4c1926a4ac04d Mon Sep 17 00:00:00 2001 From: Andreas Dangel <andreas.dangel@pmd-code.org> Date: Fri, 3 Apr 2020 15:40:10 +0200 Subject: [PATCH 37/56] [doc] Update release notes, fixes #2378 --- 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..2d317bd652 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -36,6 +36,8 @@ 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 +* java + * [#2378](https://github.com/pmd/pmd/issues/2378): \[java] AbstractJUnitRule has bad performance on large code bases ### API Changes From 292bcbbf04eb395589ef1186ea1b2a1f6737bdf9 Mon Sep 17 00:00:00 2001 From: Gwilym Kuiper <gwilym@gearset.com> Date: Fri, 3 Apr 2020 15:01:56 +0100 Subject: [PATCH 38/56] Correctly detect that fields should go before block statements --- .../FieldDeclarationsShouldBeAtStartRule.java | 2 ++ .../xml/FieldDeclarationsShouldBeAtStart.xml | 17 +++++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java index 736f1a17ad..8144dabbbb 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java @@ -9,6 +9,7 @@ import java.util.Comparator; import java.util.List; import java.util.Optional; +import net.sourceforge.pmd.lang.apex.ast.ASTBlockStatement; import net.sourceforge.pmd.lang.apex.ast.ASTField; import net.sourceforge.pmd.lang.apex.ast.ASTMethod; import net.sourceforge.pmd.lang.apex.ast.ASTProperty; @@ -34,6 +35,7 @@ public class FieldDeclarationsShouldBeAtStartRule extends AbstractApexRule { nonFieldDeclarations.addAll(node.findChildrenOfType(ASTMethod.class)); nonFieldDeclarations.addAll(node.findChildrenOfType(ASTUserClass.class)); nonFieldDeclarations.addAll(node.findChildrenOfType(ASTProperty.class)); + nonFieldDeclarations.addAll(node.findChildrenOfType(ASTBlockStatement.class)); Optional<ApexNode<?>> firstNonFieldDeclaration = nonFieldDeclarations.stream() .filter(ApexNode::hasRealLoc) diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml index 183bda51af..2abc99d575 100644 --- a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml @@ -167,6 +167,23 @@ class Foo { public Integer thisIsNotOkay; } +]]> + </code> + </test-code> + + <test-code> + <description>Fields should go before block statements</description> + <expected-problems>1</expected-problems> + <expected-linenumbers>6</expected-linenumbers> + <code> + <![CDATA[ +class Foo { + { + System.debug('Hello'); + } + + public Integer thisIsNotOkay; +} ]]> </code> </test-code> From 9e078366590b5efe2fc336c48fd863ded3e4b521 Mon Sep 17 00:00:00 2001 From: Gwilym Kuiper <gwilym@gearset.com> Date: Fri, 3 Apr 2020 15:10:30 +0100 Subject: [PATCH 39/56] Correctly detect fields that appear after static initialization blocks --- .../FieldDeclarationsShouldBeAtStartRule.java | 14 +++++++++++++- .../xml/FieldDeclarationsShouldBeAtStart.xml | 17 +++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java index 8144dabbbb..9386accdad 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java @@ -8,6 +8,8 @@ import java.util.ArrayList; import java.util.Comparator; import java.util.List; import java.util.Optional; +import java.util.stream.Collectors; +import java.util.stream.Stream; import net.sourceforge.pmd.lang.apex.ast.ASTBlockStatement; import net.sourceforge.pmd.lang.apex.ast.ASTField; @@ -32,7 +34,7 @@ public class FieldDeclarationsShouldBeAtStartRule extends AbstractApexRule { List<ApexNode<?>> nonFieldDeclarations = new ArrayList<>(); - nonFieldDeclarations.addAll(node.findChildrenOfType(ASTMethod.class)); + nonFieldDeclarations.addAll(getMethodNodes(node)); nonFieldDeclarations.addAll(node.findChildrenOfType(ASTUserClass.class)); nonFieldDeclarations.addAll(node.findChildrenOfType(ASTProperty.class)); nonFieldDeclarations.addAll(node.findChildrenOfType(ASTBlockStatement.class)); @@ -54,4 +56,14 @@ public class FieldDeclarationsShouldBeAtStartRule extends AbstractApexRule { return super.visit(node, data); } + + private List<ApexNode<?>> getMethodNodes(ASTUserClass node) { + // The method <clinit> represents static initializer blocks, of which there can be many. Given that the + // <clinit> method doesn't contain location information, only the containing ASTBlockStatements, we fetch + // them for that method only. + return node.findChildrenOfType(ASTMethod.class).stream() + .flatMap(method -> method.getImage().equals("<clinit>") ? + method.findChildrenOfType(ASTBlockStatement.class).stream() : Stream.of(method)) + .collect(Collectors.toList()); + } } diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml index 2abc99d575..f8f1b95503 100644 --- a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/codestyle/xml/FieldDeclarationsShouldBeAtStart.xml @@ -184,6 +184,23 @@ class Foo { public Integer thisIsNotOkay; } +]]> + </code> + </test-code> + + <test-code> + <description>Fields should go before static block statements</description> + <expected-problems>1</expected-problems> + <expected-linenumbers>6</expected-linenumbers> + <code> + <![CDATA[ +class Foo { + static { + System.debug('Hello'); + } + + public Integer thisIsNotOkay; +} ]]> </code> </test-code> From 4aae0e2ebf6dc4466b3b74a1d986fe61361779f4 Mon Sep 17 00:00:00 2001 From: Gwilym Kuiper <gwilym@gearset.com> Date: Fri, 3 Apr 2020 15:16:52 +0100 Subject: [PATCH 40/56] Fix checkstyle violations --- .../FieldDeclarationsShouldBeAtStartRule.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java index 9386accdad..58f3bbb217 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java @@ -20,7 +20,7 @@ import net.sourceforge.pmd.lang.apex.ast.ApexNode; import net.sourceforge.pmd.lang.apex.rule.AbstractApexRule; public class FieldDeclarationsShouldBeAtStartRule extends AbstractApexRule { - private static final Comparator<ApexNode<?>> nodeBySourceLocationComparator = + private static final Comparator<ApexNode<?>> NODE_BY_SOURCE_LOCATION_COMPARATOR = Comparator .<ApexNode<?>>comparingInt(ApexNode::getBeginLine) .thenComparing(ApexNode::getBeginColumn); @@ -41,7 +41,7 @@ public class FieldDeclarationsShouldBeAtStartRule extends AbstractApexRule { Optional<ApexNode<?>> firstNonFieldDeclaration = nonFieldDeclarations.stream() .filter(ApexNode::hasRealLoc) - .min(nodeBySourceLocationComparator); + .min(NODE_BY_SOURCE_LOCATION_COMPARATOR); if (!firstNonFieldDeclaration.isPresent()) { // there is nothing except field declaration, so that has to come first @@ -49,7 +49,7 @@ public class FieldDeclarationsShouldBeAtStartRule extends AbstractApexRule { } for (ASTField field : fields) { - if (nodeBySourceLocationComparator.compare(field, firstNonFieldDeclaration.get()) > 0) { + if (NODE_BY_SOURCE_LOCATION_COMPARATOR.compare(field, firstNonFieldDeclaration.get()) > 0) { addViolation(data, field, field.getName()); } } @@ -62,8 +62,8 @@ public class FieldDeclarationsShouldBeAtStartRule extends AbstractApexRule { // <clinit> method doesn't contain location information, only the containing ASTBlockStatements, we fetch // them for that method only. return node.findChildrenOfType(ASTMethod.class).stream() - .flatMap(method -> method.getImage().equals("<clinit>") ? - method.findChildrenOfType(ASTBlockStatement.class).stream() : Stream.of(method)) + .flatMap(method -> method.getImage().equals("<clinit>") + ? method.findChildrenOfType(ASTBlockStatement.class).stream() : Stream.of(method)) .collect(Collectors.toList()); } } From 1331eec2084d349b45ca4968603948131576af74 Mon Sep 17 00:00:00 2001 From: Gwilym Kuiper <gwilym@gearset.com> Date: Fri, 3 Apr 2020 15:17:01 +0100 Subject: [PATCH 41/56] Make comment clearer --- .../codestyle/FieldDeclarationsShouldBeAtStartRule.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java index 58f3bbb217..6dc4c45b05 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java @@ -58,9 +58,9 @@ public class FieldDeclarationsShouldBeAtStartRule extends AbstractApexRule { } private List<ApexNode<?>> getMethodNodes(ASTUserClass node) { - // The method <clinit> represents static initializer blocks, of which there can be many. Given that the - // <clinit> method doesn't contain location information, only the containing ASTBlockStatements, we fetch - // them for that method only. + // The method <clinit> represents static initializer blocks, of which there can be many. The + // <clinit> method doesn't contain location information, however the containing ASTBlockStatements do, + // so we fetch them for that method only. return node.findChildrenOfType(ASTMethod.class).stream() .flatMap(method -> method.getImage().equals("<clinit>") ? method.findChildrenOfType(ASTBlockStatement.class).stream() : Stream.of(method)) From 58185fce2f70c8efa5efa93ef26beeca1dfec0bd Mon Sep 17 00:00:00 2001 From: Gwilym Kuiper <gwilym@gearset.com> Date: Fri, 3 Apr 2020 15:17:29 +0100 Subject: [PATCH 42/56] Extract constant --- .../rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java index 6dc4c45b05..4286da8b51 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java @@ -24,6 +24,7 @@ public class FieldDeclarationsShouldBeAtStartRule extends AbstractApexRule { Comparator .<ApexNode<?>>comparingInt(ApexNode::getBeginLine) .thenComparing(ApexNode::getBeginColumn); + public static final String STATIC_INITIALIZER_METHOD_NAME = "<clinit>"; @Override public Object visit(ASTUserClass node, Object data) { @@ -62,7 +63,7 @@ public class FieldDeclarationsShouldBeAtStartRule extends AbstractApexRule { // <clinit> method doesn't contain location information, however the containing ASTBlockStatements do, // so we fetch them for that method only. return node.findChildrenOfType(ASTMethod.class).stream() - .flatMap(method -> method.getImage().equals("<clinit>") + .flatMap(method -> method.getImage().equals(STATIC_INITIALIZER_METHOD_NAME) ? method.findChildrenOfType(ASTBlockStatement.class).stream() : Stream.of(method)) .collect(Collectors.toList()); } From e94db8bf2254271114b1cb85b84fc9890e2a412c Mon Sep 17 00:00:00 2001 From: Gwilym Kuiper <gwilym@gearset.com> Date: Fri, 3 Apr 2020 15:18:22 +0100 Subject: [PATCH 43/56] Make rule a RuleChain --- .../codestyle/FieldDeclarationsShouldBeAtStartRule.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java index 4286da8b51..0c3c056306 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/codestyle/FieldDeclarationsShouldBeAtStartRule.java @@ -26,6 +26,10 @@ public class FieldDeclarationsShouldBeAtStartRule extends AbstractApexRule { .thenComparing(ApexNode::getBeginColumn); public static final String STATIC_INITIALIZER_METHOD_NAME = "<clinit>"; + public FieldDeclarationsShouldBeAtStartRule() { + addRuleChainVisit(ASTUserClass.class); + } + @Override public Object visit(ASTUserClass node, Object data) { // Unfortunately the parser re-orders the AST to put field declarations before method declarations @@ -46,7 +50,7 @@ public class FieldDeclarationsShouldBeAtStartRule extends AbstractApexRule { if (!firstNonFieldDeclaration.isPresent()) { // there is nothing except field declaration, so that has to come first - return super.visit(node, data); + return data; } for (ASTField field : fields) { @@ -55,7 +59,7 @@ public class FieldDeclarationsShouldBeAtStartRule extends AbstractApexRule { } } - return super.visit(node, data); + return data; } private List<ApexNode<?>> getMethodNodes(ASTUserClass node) { From 6a70bda5e6abff63e753457d5a4016792fd35229 Mon Sep 17 00:00:00 2001 From: Gwilym Kuiper <gwilym@gearset.com> Date: Fri, 3 Apr 2020 15:19:12 +0100 Subject: [PATCH 44/56] Should be 6.23.0 for release --- pmd-apex/src/main/resources/category/apex/codestyle.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pmd-apex/src/main/resources/category/apex/codestyle.xml b/pmd-apex/src/main/resources/category/apex/codestyle.xml index a28d42ff9f..62b88d5a65 100644 --- a/pmd-apex/src/main/resources/category/apex/codestyle.xml +++ b/pmd-apex/src/main/resources/category/apex/codestyle.xml @@ -367,7 +367,7 @@ while (true) { // preferred approach <rule name="FieldDeclarationsShouldBeAtStart" language="apex" - since="5.23.0" + since="6.23.0" message="Field declaration for ''{0}'' should be before method declarations in its class" class="net.sourceforge.pmd.lang.apex.rule.codestyle.FieldDeclarationsShouldBeAtStartRule" externalInfoUrl="${pmd.website.baseurl}/pmd_rules_apex_codestyle.html#fielddeclarationsshouldbeatstart"> From 7b06d3001faca93a1f163d2fe9956e576351e59c Mon Sep 17 00:00:00 2001 From: Gwilym Kuiper <gwilym@gearset.com> Date: Fri, 3 Apr 2020 15:27:37 +0100 Subject: [PATCH 45/56] Make UnusedLocalVariableRule a rule chain rule --- .../lang/apex/rule/bestpractices/UnusedLocalVariableRule.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableRule.java index 661befda13..b898dd6999 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/UnusedLocalVariableRule.java @@ -12,6 +12,10 @@ import net.sourceforge.pmd.lang.apex.ast.ASTVariableExpression; import net.sourceforge.pmd.lang.apex.rule.AbstractApexRule; public class UnusedLocalVariableRule extends AbstractApexRule { + public UnusedLocalVariableRule() { + addRuleChainVisit(ASTVariableDeclaration.class); + } + @Override public Object visit(ASTVariableDeclaration node, Object data) { String variableName = node.getImage(); From d09d4de3081a35957a20ecb85e3d6a6b661cdf46 Mon Sep 17 00:00:00 2001 From: Kieran Black <kieran@gatech.edu> Date: Fri, 3 Apr 2020 12:25:22 -0400 Subject: [PATCH 46/56] fixed WITH SECURITY_ENFORCED regex to recognise line break characters --- .../pmd/lang/apex/rule/security/ApexCRUDViolationRule.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java index f1571cd01c..7616100ee3 100644 --- a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java +++ b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/security/ApexCRUDViolationRule.java @@ -85,7 +85,7 @@ public class ApexCRUDViolationRule extends AbstractApexRule { private static final String[] RESERVED_KEYS_FLS = new String[] { "Schema", S_OBJECT_TYPE, }; - private static final Pattern WITH_SECURITY_ENFORCED = Pattern.compile("(?i).*[^']\\s*WITH\\s+SECURITY_ENFORCED\\s*[^']*"); + private static final Pattern WITH_SECURITY_ENFORCED = Pattern.compile("(?is).*[^']\\s*WITH\\s+SECURITY_ENFORCED\\s*[^']*"); private final Map<String, String> varToTypeMapping = new HashMap<>(); private final ListMultimap<String, String> typeToDMLOperationMapping = ArrayListMultimap.create(); From 746797e805a82bb25c3d7f2e9d5dc62d6a1d2280 Mon Sep 17 00:00:00 2001 From: Kieran Black <kieran@gatech.edu> Date: Fri, 3 Apr 2020 15:43:56 -0400 Subject: [PATCH 47/56] added tests for line breaks --- .../rule/security/xml/ApexCRUDViolation.xml | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexCRUDViolation.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexCRUDViolation.xml index 6df26b5834..3d0293ebcb 100644 --- a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexCRUDViolation.xml +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexCRUDViolation.xml @@ -275,6 +275,19 @@ public class Foo { } ]]></code> </test-code> + <test-code> + <description>Accepts Closure SECURITY ENFORCED Line Break </description> + <expected-problems>0</expected-problems> + <code><![CDATA[ +public class Foo { + public Contact foo(String tempID) { + Contact c = [SELECT Name FROM Contact WHERE Id=: tempID + WITH SECURITY_ENFORCED]; + return c; + } +} ]]></code> + </test-code> + <test-code> <description>Accepts Closure SECURITY ENFORCED in a List </description> <expected-problems>0</expected-problems> @@ -287,6 +300,19 @@ public class Foo { } ]]></code> </test-code> + <test-code> + <description>Accepts Closure SECURITY ENFORCED in a List Line Break</description> + <expected-problems>0</expected-problems> + <code><![CDATA[ +public class Foo { + public List<Contact> m() { + List<Contact> c = [SELECT Name FROM Contact + WITH SECURITY_ENFORCED LIMIT 1]; + return c; + } +} ]]></code> + </test-code> + <test-code> <description>Accepts Closure SECURITY ENFORCED with Case Insensitivity </description> <expected-problems>0</expected-problems> @@ -299,6 +325,19 @@ public class Foo { } ]]></code> </test-code> + <test-code> + <description>Accepts Closure SECURITY ENFORCED with Case Insensitivity Line Break </description> + <expected-problems>0</expected-problems> + <code><![CDATA[ +public class Foo { + public Contact foo(String tempID) { + Contact c = [SELECT Name FROM Contact WHERE Id=: tempID + WItH SECURITY_ENFORCED]; + return c; + } +} ]]></code> + </test-code> + <test-code> <description>Accepts Closure SECURITY ENFORCED Not Secured </description> <expected-problems>1</expected-problems> @@ -323,6 +362,19 @@ public class Foo { } ]]></code> </test-code> + <test-code> + <description>Accepts Closure SECURITY ENFORCED Secured Line Break </description> + <expected-problems>0</expected-problems> + <code><![CDATA[ +public class Foo { + public Contact foo() { + Contact c = [SELECT Name FROM Contact WHERE Name = 'WITH SECURITY_ENFORCED' + WITH SECURITY_ENFORCED]; + return c; + } +} ]]></code> + </test-code> + <test-code> <description>Proper accessibility CRUD,FLS </description> <expected-problems>0</expected-problems> From e47a6b0007f3a4eee09cc686cc97bb54780673cf Mon Sep 17 00:00:00 2001 From: Andreas Dangel <andreas.dangel@pmd-code.org> Date: Sat, 4 Apr 2020 10:21:18 +0200 Subject: [PATCH 48/56] [doc] Update release notes, refs #2397, fixes #2399 --- docs/pages/release_notes.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index fe9fbfe609..30b5b87174 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -34,9 +34,11 @@ Note that XPath 1.0 support, the default XPath version, is deprecated since PMD ### Fixed Issues -* apex - * [#2210](https://github.com/pmd/pmd/issues/2210): \[apex] ApexCRUDViolation: Support WITH SECURITY_ENFORCED +* apex-design * [#2358](https://github.com/pmd/pmd/issues/2358): \[apex] Invalid Apex in Cognitive Complexity tests +* apex-security + * [#2210](https://github.com/pmd/pmd/issues/2210): \[apex] ApexCRUDViolation: Support WITH SECURITY_ENFORCED + * [#2399](https://github.com/pmd/pmd/issues/2399): \[apex] ApexCRUDViolation: false positive with security enforced with line break * java * [#2378](https://github.com/pmd/pmd/issues/2378): \[java] AbstractJUnitRule has bad performance on large code bases * java-design @@ -113,6 +115,7 @@ implementations, and their corresponding Parser if it exists (in the same packag * [#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) +* [#2397](https://github.com/pmd/pmd/pull/2397): \[apex] fixed WITH SECURITY_ENFORCED regex to recognise line break characters - [Kieran Black](https://github.com/kieranlblack) {% endtocmaker %} From e064a5f5cab679e882b50eee9b1ff2bcfe21b7a6 Mon Sep 17 00:00:00 2001 From: Andreas Dangel <andreas.dangel@pmd-code.org> Date: Sat, 4 Apr 2020 10:43:52 +0200 Subject: [PATCH 49/56] [doc] Update release notes, fixes #1723 --- 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 9a265e1482..3650caaeee 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -37,6 +37,8 @@ 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 +* java-codestyle + * [#1723](https://github.com/pmd/pmd/issues/1723): \[java] UseDiamondOperator false-positive inside lambda ### API Changes From f1759c68c6fdd9694de7cde8e1d8003948db27f0 Mon Sep 17 00:00:00 2001 From: Andreas Dangel <andreas.dangel@pmd-code.org> Date: Sat, 4 Apr 2020 17:38:13 +0200 Subject: [PATCH 50/56] [apex] Move new rule def up in codestyle.xml --- .../resources/category/apex/codestyle.xml | 48 +++++++++---------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/pmd-apex/src/main/resources/category/apex/codestyle.xml b/pmd-apex/src/main/resources/category/apex/codestyle.xml index 62b88d5a65..f04a7580f8 100644 --- a/pmd-apex/src/main/resources/category/apex/codestyle.xml +++ b/pmd-apex/src/main/resources/category/apex/codestyle.xml @@ -102,6 +102,30 @@ if (foo) { // preferred approach </example> </rule> + <rule name="FieldDeclarationsShouldBeAtStart" + language="apex" + since="6.23.0" + message="Field declaration for ''{0}'' should be before method declarations in its class" + class="net.sourceforge.pmd.lang.apex.rule.codestyle.FieldDeclarationsShouldBeAtStartRule" + externalInfoUrl="${pmd.website.baseurl}/pmd_rules_apex_codestyle.html#fielddeclarationsshouldbeatstart"> + <description> + Field declarations should appear before method declarations within a class. + </description> + <priority>3</priority> + <example> +<![CDATA[ +class Foo { + public Integer someField; // good + + public void someMethod() { + } + + public Integer anotherField; // bad +} +]]> + </example> + </rule> + <rule name="FieldNamingConventions" since="6.15.0" message="The {0} name ''{1}'' doesn''t match ''{2}''" @@ -361,30 +385,6 @@ while (true) // not recommended while (true) { // preferred approach x++; } -]]> - </example> - </rule> - - <rule name="FieldDeclarationsShouldBeAtStart" - language="apex" - since="6.23.0" - message="Field declaration for ''{0}'' should be before method declarations in its class" - class="net.sourceforge.pmd.lang.apex.rule.codestyle.FieldDeclarationsShouldBeAtStartRule" - externalInfoUrl="${pmd.website.baseurl}/pmd_rules_apex_codestyle.html#fielddeclarationsshouldbeatstart"> - <description> - Field declarations should appear before method declarations within a class. - </description> - <priority>3</priority> - <example> -<![CDATA[ -class Foo { - public Integer someField; // good - - public void someMethod() { - } - - public Integer anotherField; // bad -} ]]> </example> </rule> From 92a518c99114fd09249464c022acc49314db7c6a Mon Sep 17 00:00:00 2001 From: Andreas Dangel <andreas.dangel@pmd-code.org> Date: Sat, 4 Apr 2020 17:43:14 +0200 Subject: [PATCH 51/56] [doc] Update release notes, refs #2396 --- docs/pages/release_notes.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 678fd22650..35f00a38c5 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -30,7 +30,10 @@ not change the result of your rules*, if it does, please report a bug at https:/ Note that XPath 1.0 support, the default XPath version, is deprecated since PMD 6.22.0. **We highly recommend that you upgrade your rules to XPath 2.0**. Please refer to the [migration guide](https://pmd.github.io/latest/pmd_userdocs_extending_writing_xpath_rules.html#migrating-from-10-to-20). +#### New Rules +* The new Apex rule {% rule "apex/codestyle/FieldDeclarationsShouldBeAtStart" %} (`apex-codestyle`) + helps to ensure that field declarations are always at the beginning of a class. ### Fixed Issues @@ -105,6 +108,7 @@ implementations, and their corresponding Parser if it exists (in the same packag * [#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) +* [#2396](https://github.com/pmd/pmd/pull/2396): \[apex] New rule: field declarations should be at start - [Gwilym Kuiper](https://github.com/gwilymatgearset) {% endtocmaker %} From 2f33951f604b7f0c30135b6744a80784f88ef257 Mon Sep 17 00:00:00 2001 From: Andreas Dangel <andreas.dangel@pmd-code.org> Date: Sat, 4 Apr 2020 17:49:09 +0200 Subject: [PATCH 52/56] Add new rules ruleset for release 6.23.0 --- .../src/main/resources/rulesets/releases/6230.xml | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 pmd-core/src/main/resources/rulesets/releases/6230.xml diff --git a/pmd-core/src/main/resources/rulesets/releases/6230.xml b/pmd-core/src/main/resources/rulesets/releases/6230.xml new file mode 100644 index 0000000000..51bbf60505 --- /dev/null +++ b/pmd-core/src/main/resources/rulesets/releases/6230.xml @@ -0,0 +1,13 @@ +<?xml version="1.0"?> + +<ruleset name="6230" + xmlns="http://pmd.sourceforge.net/ruleset/2.0.0" + xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" + xsi:schemaLocation="http://pmd.sourceforge.net/ruleset/2.0.0 https://pmd.sourceforge.io/ruleset_2_0_0.xsd"> + <description> +This ruleset contains links to rules that are new in PMD v6.23.0 + </description> + + <rule ref="category/apex/codestyle.xml/FieldDeclarationsShouldBeAtStart"/> + +</ruleset> From 9db695eee2aca65a9163a84fffe4415d2f87e2b3 Mon Sep 17 00:00:00 2001 From: Andreas Dangel <andreas.dangel@pmd-code.org> Date: Sat, 4 Apr 2020 18:04:41 +0200 Subject: [PATCH 53/56] [apex] Add additional test case for unused local var shadowing a field --- .../bestpractices/xml/UnusedLocalVariable.xml | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/UnusedLocalVariable.xml b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/UnusedLocalVariable.xml index a708821b1a..5e5e7ed803 100644 --- a/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/UnusedLocalVariable.xml +++ b/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/UnusedLocalVariable.xml @@ -55,4 +55,28 @@ public class Foo { ]]> </code> </test-code> + + <test-code> + <description>Shadowing a field</description> + <expected-problems>1</expected-problems> + <expected-linenumbers>5</expected-linenumbers> + <code><![CDATA[ +public class Foo { + private String myfield; + + public void unused() { + String myfield = 'unused string'; + } + + public String usedDifferentMethod() { + String myfield = 'used'; + return myfield; + } + + public String fieldUsage() { + return myfield; + } +} + ]]></code> + </test-code> </test-data> \ No newline at end of file From cdbb4570b44512df3f9bd0bdd775b3283619b07a Mon Sep 17 00:00:00 2001 From: Andreas Dangel <andreas.dangel@pmd-code.org> Date: Sat, 4 Apr 2020 18:04:53 +0200 Subject: [PATCH 54/56] [doc] Update release notes, refs #2395 --- 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 678fd22650..cedd5618d6 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -30,6 +30,10 @@ not change the result of your rules*, if it does, please report a bug at https:/ Note that XPath 1.0 support, the default XPath version, is deprecated since PMD 6.22.0. **We highly recommend that you upgrade your rules to XPath 2.0**. Please refer to the [migration guide](https://pmd.github.io/latest/pmd_userdocs_extending_writing_xpath_rules.html#migrating-from-10-to-20). +#### New Rules + +* The apex rule {% rule "apex/bestpractices/UnusedLocalVariable" %} (`apex-bestpractices`) detects unused + local variables. ### Fixed Issues @@ -105,6 +109,7 @@ implementations, and their corresponding Parser if it exists (in the same packag * [#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) +* [#2395](https://github.com/pmd/pmd/pull/2395): \[apex] New Rule: Unused local variables - [Gwilym Kuiper](https://github.com/gwilymatgearset) {% endtocmaker %} From 5ed857070373f222ad6d8aa69b797574ba9b258e Mon Sep 17 00:00:00 2001 From: Andreas Dangel <andreas.dangel@pmd-code.org> Date: Sat, 4 Apr 2020 18:09:31 +0200 Subject: [PATCH 55/56] Add new rule: apex/bestpractices.xml/UnusedLocalVariable --- pmd-core/src/main/resources/rulesets/releases/6230.xml | 1 + 1 file changed, 1 insertion(+) diff --git a/pmd-core/src/main/resources/rulesets/releases/6230.xml b/pmd-core/src/main/resources/rulesets/releases/6230.xml index 51bbf60505..e2d8620ff2 100644 --- a/pmd-core/src/main/resources/rulesets/releases/6230.xml +++ b/pmd-core/src/main/resources/rulesets/releases/6230.xml @@ -8,6 +8,7 @@ This ruleset contains links to rules that are new in PMD v6.23.0 </description> + <rule ref="category/apex/bestpractices.xml/UnusedLocalVariable"/> <rule ref="category/apex/codestyle.xml/FieldDeclarationsShouldBeAtStart"/> </ruleset> From 0356895cc2e6afc6379afa104e55def2b6f0310f Mon Sep 17 00:00:00 2001 From: Andreas Dangel <andreas.dangel@pmd-code.org> Date: Sat, 4 Apr 2020 18:29:50 +0200 Subject: [PATCH 56/56] [doc] Fix github issue templates The labels were wrong, maybe now they are assigned automatically... --- .github/ISSUE_TEMPLATE/bug_report.md | 2 +- .github/ISSUE_TEMPLATE/feature_request.md | 2 +- .github/ISSUE_TEMPLATE/new_rule.md | 2 +- .github/ISSUE_TEMPLATE/question.md | 2 +- .github/ISSUE_TEMPLATE/rule_violation.md | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/ISSUE_TEMPLATE/bug_report.md b/.github/ISSUE_TEMPLATE/bug_report.md index 0bd4d2f99f..5c752659ea 100644 --- a/.github/ISSUE_TEMPLATE/bug_report.md +++ b/.github/ISSUE_TEMPLATE/bug_report.md @@ -2,7 +2,7 @@ name: Bug report about: Create a report to help us improve title: '' -labels: bug +labels: 'a:bug' assignees: '' --- diff --git a/.github/ISSUE_TEMPLATE/feature_request.md b/.github/ISSUE_TEMPLATE/feature_request.md index 5efb987e38..ecbd0ebc84 100644 --- a/.github/ISSUE_TEMPLATE/feature_request.md +++ b/.github/ISSUE_TEMPLATE/feature_request.md @@ -2,7 +2,7 @@ name: Feature request about: Suggest an idea for this project title: '' -labels: enhancement +labels: 'an:enhancement' assignees: '' --- diff --git a/.github/ISSUE_TEMPLATE/new_rule.md b/.github/ISSUE_TEMPLATE/new_rule.md index ec48bd982c..4e79d37f11 100644 --- a/.github/ISSUE_TEMPLATE/new_rule.md +++ b/.github/ISSUE_TEMPLATE/new_rule.md @@ -2,7 +2,7 @@ name: New Rule about: You have an idea for a new rule? Great! title: '' -labels: new-rule +labels: 'a:new-rule' assignees: '' --- diff --git a/.github/ISSUE_TEMPLATE/question.md b/.github/ISSUE_TEMPLATE/question.md index 9389f8c3ba..5af12acfa1 100644 --- a/.github/ISSUE_TEMPLATE/question.md +++ b/.github/ISSUE_TEMPLATE/question.md @@ -2,7 +2,7 @@ name: Question about: Feel free to ask any question about PMD and its usage title: '' -labels: question +labels: 'a:question' assignees: '' --- diff --git a/.github/ISSUE_TEMPLATE/rule_violation.md b/.github/ISSUE_TEMPLATE/rule_violation.md index 92404bd43b..dd2c61ab0d 100644 --- a/.github/ISSUE_TEMPLATE/rule_violation.md +++ b/.github/ISSUE_TEMPLATE/rule_violation.md @@ -2,7 +2,7 @@ name: Rule violation about: Let us know about a false positive/false negative title: '' -labels: bug +labels: 'a:bug' assignees: '' ---