Merge pull request #3958 from oowekyala:issue2352-ruleset-deprecations

[core] Deprecate some syntax for ruleset references #3958

docs/pages/release_notes.md

@@ -84,6 +84,7 @@ ruleset. Use the new rule {% rule java/codestyle/UnnecessarySemicolon %} instead
 * cli
     * [#1445](https://github.com/pmd/pmd/issues/1445): \[core] Allow CLI to take globs as parameters
 * core
+    * [#2352](https://github.com/pmd/pmd/issues/2352): \[core] Deprecate \<lang\>-\<ruleset\> hyphen notation for ruleset references
     * [#3942](https://github.com/pmd/pmd/issues/3942): \[core] common-io path traversal vulnerability (CVE-2021-29425)
 * cs (c#)
     * [#3974](https://github.com/pmd/pmd/pull/3974): \[cs] Add option to ignore C# attributes (annotations)
@@ -110,8 +111,20 @@ ruleset. Use the new rule {% rule java/codestyle/UnnecessarySemicolon %} instead
 
 ### API Changes
 
+#### Deprecated ruleset references
+
+Ruleset references with the following formats are now deprecated and will produce a warning
+when used on the CLI or in a ruleset XML file:
+- `<lang-name>-<ruleset-name>`, eg `java-basic`, which resolves to `rulesets/java/basic.xml`
+- the internal release number, eg `600`, which resolves to `rulesets/releases/600.xml`
+
+Use the explicit forms of these references to be compatible with PMD 7.
+
 #### Deprecated API
 
+- {% jdoc core::RuleSetReferenceId#toString() %} is now deprecated. The format of this
+ method will remain the same until PMD 7. The deprecation is intended to steer users
+ away from relying on this format, as it may be changed in PMD 7.
 - {% jdoc core::PMDConfiguration#getInputPaths() %} and
 {% jdoc core::PMDConfiguration#setInputPaths(java.lang.String) %} are now deprecated.
 A new set of methods have been added, which use lists and do not rely on comma splitting.

pmd-core/src/main/java/net/sourceforge/pmd/PMD.java

@@ -242,6 +242,7 @@ public SourceCodeProcessor getSourceCodeProcessor() {
     @Deprecated
     @InternalApi
     public static int doPMD(PMDConfiguration configuration) {
+        LOG.fine("Current classpath:\n" + System.getProperty("java.class.path"));
         try (PmdAnalysis pmd = PmdAnalysis.create(configuration)) {
             if (pmd.getRulesets().isEmpty()) {
                 return pmd.getReporter().numErrors() > 0 ? -1 : 0;

pmd-core/src/main/java/net/sourceforge/pmd/RuleSetFactory.java

@@ -492,17 +492,22 @@ private DocumentBuilder createDocumentBuilder() throws ParserConfigurationExcept
      * @param rulesetReferences keeps track of already processed complete ruleset references in order to log a warning
      */
     private void parseRuleNode(RuleSetReferenceId ruleSetReferenceId, RuleSetBuilder ruleSetBuilder, Node ruleNode,
-            boolean withDeprecatedRuleReferences, Set<String> rulesetReferences)
-            throws RuleSetNotFoundException {
+                               boolean withDeprecatedRuleReferences, Set<String> rulesetReferences)
+        throws RuleSetNotFoundException {
         Element ruleElement = (Element) ruleNode;
-        String ref = ruleElement.getAttribute("ref");
-        if (ref.endsWith("xml")) {
-            parseRuleSetReferenceNode(ruleSetBuilder, ruleElement, ref, rulesetReferences);
-        } else if (StringUtils.isBlank(ref)) {
-            parseSingleRuleNode(ruleSetReferenceId, ruleSetBuilder, ruleNode);
-        } else {
-            parseRuleReferenceNode(ruleSetReferenceId, ruleSetBuilder, ruleNode, ref, withDeprecatedRuleReferences);
+        if (ruleElement.hasAttribute("ref")) {
+            String ref = ruleElement.getAttribute("ref");
+            RuleSetReferenceId refId = parseReferenceAndWarn(ruleSetBuilder, ref);
+            if (refId != null) {
+                if (refId.isAllRules()) {
+                    parseRuleSetReferenceNode(ruleSetBuilder, ruleElement, ref, refId, rulesetReferences);
+                } else {
+                    parseRuleReferenceNode(ruleSetReferenceId, ruleSetBuilder, ruleNode, ref, refId, withDeprecatedRuleReferences);
+                }
+                return;
+            }
         }
+        parseSingleRuleNode(ruleSetReferenceId, ruleSetBuilder, ruleNode);
     }
 
     /**
@@ -519,8 +524,10 @@ private void parseRuleNode(RuleSetReferenceId ruleSetReferenceId, RuleSetBuilder
      *            The RuleSet reference.
      * @param rulesetReferences keeps track of already processed complete ruleset references in order to log a warning
      */
-    private void parseRuleSetReferenceNode(RuleSetBuilder ruleSetBuilder, Element ruleElement, String ref, Set<String> rulesetReferences)
-            throws RuleSetNotFoundException {
+    private void parseRuleSetReferenceNode(RuleSetBuilder ruleSetBuilder, Element ruleElement,
+                                           String ref,
+                                           RuleSetReferenceId ruleSetReferenceId, Set<String> rulesetReferences)
+        throws RuleSetNotFoundException {
         String priority = null;
         NodeList childNodes = ruleElement.getChildNodes();
         Set<String> excludedRulesCheck = new HashSet<>();
@@ -539,7 +546,7 @@ private void parseRuleSetReferenceNode(RuleSetBuilder ruleSetBuilder, Element ru
         // load the ruleset with minimum priority low, so that we get all rules, to be able to exclude any rule
         // minimum priority will be applied again, before constructing the final ruleset
         RuleSetFactory ruleSetFactory = toLoader().filterAbovePriority(RulePriority.LOW).warnDeprecated(false).toFactory();
-        RuleSet otherRuleSet = ruleSetFactory.createRuleSet(RuleSetReferenceId.parse(ref).get(0));
+        RuleSet otherRuleSet = ruleSetFactory.createRuleSet(ruleSetReferenceId);
         List<RuleReference> potentialRules = new ArrayList<>();
         int countDeprecated = 0;
         for (Rule rule : otherRuleSet.getRules()) {
@@ -584,11 +591,23 @@ private void parseRuleSetReferenceNode(RuleSetBuilder ruleSetBuilder, Element ru
 
         if (rulesetReferences.contains(ref)) {
             LOG.warning("The ruleset " + ref + " is referenced multiple times in \""
-                    + ruleSetBuilder.getName() + "\".");
+                        + ruleSetBuilder.getName() + "\".");
         }
         rulesetReferences.add(ref);
     }
 
+    private RuleSetReferenceId parseReferenceAndWarn(RuleSetBuilder ruleSetBuilder, String ref) {
+        List<RuleSetReferenceId> references = RuleSetReferenceId.parse(ref, warnDeprecated);
+        if (references.size() > 1 && warnDeprecated) {
+            LOG.warning("Using a comma separated list as a ref attribute is deprecated. "
+                        + "All references but the first are ignored. Reference: '" + ref + "'");
+        } else if (references.isEmpty()) {
+            LOG.warning("Empty ref attribute in ruleset '" + ruleSetBuilder.getName() + "'");
+            return null;
+        }
+        return references.get(0);
+    }
+
     /**
      * Parse a rule node as a single Rule. The Rule has been fully defined
      * within the context of the current RuleSet.
@@ -641,7 +660,9 @@ private void parseSingleRuleNode(RuleSetReferenceId ruleSetReferenceId, RuleSetB
      *            or not
      */
     private void parseRuleReferenceNode(RuleSetReferenceId ruleSetReferenceId, RuleSetBuilder ruleSetBuilder,
-            Node ruleNode, String ref, boolean withDeprecatedRuleReferences) throws RuleSetNotFoundException {
+                                        Node ruleNode, String ref,
+                                        RuleSetReferenceId otherRuleSetReferenceId,
+                                        boolean withDeprecatedRuleReferences) throws RuleSetNotFoundException {
         Element ruleElement = (Element) ruleNode;
 
         // Stop if we're looking for a particular Rule, and this element is not
@@ -656,7 +677,6 @@ private void parseRuleReferenceNode(RuleSetReferenceId ruleSetReferenceId, RuleS
         RuleSetFactory ruleSetFactory = toLoader().filterAbovePriority(RulePriority.LOW).warnDeprecated(false).toFactory();
 
         boolean isSameRuleSet = false;
-        RuleSetReferenceId otherRuleSetReferenceId = RuleSetReferenceId.parse(ref).get(0);
         if (!otherRuleSetReferenceId.isExternal()
                 && containsRule(ruleSetReferenceId, otherRuleSetReferenceId.getRuleName())) {
             otherRuleSetReferenceId = new RuleSetReferenceId(ref, ruleSetReferenceId);
@@ -743,6 +763,7 @@ && containsRule(ruleSetReferenceId, otherRuleSetReferenceId.getRuleName())) {
      * @return {@code true} if the ruleName exists
      */
     private boolean containsRule(RuleSetReferenceId ruleSetReferenceId, String ruleName) {
+        // TODO: avoid reloading the ruleset once again
         boolean found = false;
         try (InputStream ruleSet = ruleSetReferenceId.getInputStream(resourceLoader)) {
             DocumentBuilder builder = createDocumentBuilder();

pmd-core/src/main/java/net/sourceforge/pmd/RuleSetLoader.java

@@ -150,7 +150,7 @@ public RuleSetFactory toFactory() {
      * @throws RuleSetLoadException If any error occurs (eg, invalid syntax, or resource not found)
      */
     public RuleSet loadFromResource(String rulesetPath) {
-        return loadFromResource(new RuleSetReferenceId(rulesetPath));
+        return loadFromResource(new RuleSetReferenceId(rulesetPath, null, warnDeprecated));
     }
 
     /**
@@ -162,7 +162,7 @@ public RuleSet loadFromResource(String rulesetPath) {
      * @throws RuleSetLoadException If any error occurs (eg, invalid syntax)
      */
     public RuleSet loadFromString(String filename, final String rulesetXmlContent) {
-        return loadFromResource(new RuleSetReferenceId(filename) {
+        return loadFromResource(new RuleSetReferenceId(filename, null, warnDeprecated) {
             @Override
             public InputStream getInputStream(ResourceLoader rl) {
                 return new ByteArrayInputStream(rulesetXmlContent.getBytes(StandardCharsets.UTF_8));

pmd-core/src/main/java/net/sourceforge/pmd/RuleSetReferenceId.java

@@ -11,6 +11,7 @@
 import java.net.URL;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.logging.Logger;
 
 import org.apache.commons.lang3.StringUtils;
 
@@ -58,16 +59,6 @@
  * <td>all</td>
  * </tr>
  * <tr>
- * <td>java-basic</td>
- * <td>rulesets/java/basic.xml</td>
- * <td>all</td>
- * </tr>
- * <tr>
- * <td>50</td>
- * <td>rulesets/releases/50.xml</td>
- * <td>all</td>
- * </tr>
- * <tr>
  * <td>rulesets/java/basic.xml/EmptyCatchBlock</td>
  * <td>rulesets/java/basic.xml</td>
  * <td>EmptyCatchBlock</td>
@@ -85,12 +76,21 @@
 @Deprecated
 @InternalApi
 public class RuleSetReferenceId {
+
+    // todo this class has issues... What is even an "external" ruleset?
+    //  terminology and API should be clarified.
+
+    // use the logger of RuleSetFactory, because the warnings conceptually come from there.
+    private static final Logger LOG = Logger.getLogger(RuleSetFactory.class.getName());
+
     private final boolean external;
     private final String ruleSetFileName;
     private final boolean allRules;
     private final String ruleName;
     private final RuleSetReferenceId externalRuleSetReferenceId;
 
+    private final String originalRef;
+
     /**
      * Construct a RuleSetReferenceId for the given single ID string.
      *
@@ -110,19 +110,34 @@ public RuleSetReferenceId(final String id) {
      * Rule. The external RuleSetReferenceId will be responsible for producing
      * the InputStream containing the Rule.
      *
-     * @param id
-     *            The id string.
-     * @param externalRuleSetReferenceId
-     *            A RuleSetReferenceId to associate with this new instance.
-     * @throws IllegalArgumentException
-     *             If the ID contains a comma character.
-     * @throws IllegalArgumentException
-     *             If external RuleSetReferenceId is not external.
-     * @throws IllegalArgumentException
-     *             If the ID is not Rule reference when there is an external
-     *             RuleSetReferenceId.
+     * @param id                         The id string.
+     * @param externalRuleSetReferenceId A RuleSetReferenceId to associate with this new instance.
+     *
+     * @throws IllegalArgumentException If the ID contains a comma character.
+     * @throws IllegalArgumentException If external RuleSetReferenceId is not external.
+     * @throws IllegalArgumentException If the ID is not Rule reference when there is an external
+     *                                  RuleSetReferenceId.
      */
     public RuleSetReferenceId(final String id, final RuleSetReferenceId externalRuleSetReferenceId) {
+        this(id, externalRuleSetReferenceId, false);
+    }
+
+    /**
+     * Construct a RuleSetReferenceId for the given single ID string. If an
+     * external RuleSetReferenceId is given, the ID must refer to a non-external
+     * Rule. The external RuleSetReferenceId will be responsible for producing
+     * the InputStream containing the Rule.
+     *
+     * @param id                         The id string.
+     * @param externalRuleSetReferenceId A RuleSetReferenceId to associate with this new instance.
+     *
+     * @throws IllegalArgumentException If the ID contains a comma character.
+     * @throws IllegalArgumentException If external RuleSetReferenceId is not external.
+     * @throws IllegalArgumentException If the ID is not Rule reference when there is an external
+     *                                  RuleSetReferenceId.
+     */
+    RuleSetReferenceId(final String id, final RuleSetReferenceId externalRuleSetReferenceId, boolean warnDeprecated) {
+        this.originalRef = id;
 
         if (externalRuleSetReferenceId != null && !externalRuleSetReferenceId.isExternal()) {
             throw new IllegalArgumentException("Cannot pair with non-external <" + externalRuleSetReferenceId + ">.");
@@ -177,8 +192,16 @@ public RuleSetReferenceId(final String id, final RuleSetReferenceId externalRule
                 allRules = tempRuleName == null;
             } else {
                 // resolve the ruleset name - it's maybe a built in ruleset
-                String builtinRuleSet = resolveBuiltInRuleset(tempRuleSetFileName);
+                String expandedRuleset = resolveDeprecatedBuiltInRulesetShorthand(tempRuleSetFileName);
+                String builtinRuleSet = expandedRuleset == null ? tempRuleSetFileName : expandedRuleset;
                 if (checkRulesetExists(builtinRuleSet)) {
+                    if (expandedRuleset != null && warnDeprecated) {
+                        LOG.warning(
+                            "Ruleset reference '" + tempRuleSetFileName + "' uses a deprecated form, use '"
+                            + builtinRuleSet + "' instead"
+                        );
+                    }
+
                     external = true;
                     ruleSetFileName = builtinRuleSet;
                     ruleName = tempRuleName;
@@ -249,25 +272,22 @@ private boolean checkRulesetExists(final String name) {
      *            the ruleset name
      * @return the full classpath to the ruleset
      */
-    private String resolveBuiltInRuleset(final String name) {
-        String result = null;
-        if (name != null) {
-            // Likely a simple RuleSet name
-            int index = name.indexOf('-');
-            if (index >= 0) {
-                // Standard short name
-                result = "rulesets/" + name.substring(0, index) + '/' + name.substring(index + 1) + ".xml";
-            } else {
-                // A release RuleSet?
-                if (name.matches("[0-9]+.*")) {
-                    result = "rulesets/releases/" + name + ".xml";
-                } else {
-                    // Appears to be a non-standard RuleSet name
-                    result = name;
-                }
-            }
+    private String resolveDeprecatedBuiltInRulesetShorthand(final String name) {
+        if (name == null) {
+            return null;
         }
-        return result;
+        // Likely a simple RuleSet name
+        int index = name.indexOf('-');
+        if (index > 0) {
+            // Standard short name
+            return "rulesets/" + name.substring(0, index) + '/' + name.substring(index + 1) + ".xml";
+        }
+        // A release RuleSet?
+        if (name.matches("[0-9]+.*")) {
+            return "rulesets/releases/" + name + ".xml";
+        }
+        // Appears to be a non-standard RuleSet name
+        return null;
     }
 
     /**
@@ -330,19 +350,31 @@ private static boolean isFullRuleSetName(String name) {
      * Parse a String comma separated list of RuleSet reference IDs into a List
      * of RuleReferenceId instances.
      *
-     * @param referenceString
-     *            A comma separated list of RuleSet reference IDs.
+     * @param referenceString A comma separated list of RuleSet reference IDs.
+     *
      * @return The corresponding List of RuleSetReferenceId instances.
      */
     public static List<RuleSetReferenceId> parse(String referenceString) {
+        return parse(referenceString, false);
+    }
+
+    /**
+     * Parse a String comma separated list of RuleSet reference IDs into a List
+     * of RuleReferenceId instances.
+     *
+     * @param referenceString A comma separated list of RuleSet reference IDs.
+     *
+     * @return The corresponding List of RuleSetReferenceId instances.
+     */
+    public static List<RuleSetReferenceId> parse(String referenceString, boolean warnDeprecated) {
         List<RuleSetReferenceId> references = new ArrayList<>();
         if (referenceString != null && referenceString.trim().length() > 0) {
 
             if (referenceString.indexOf(',') == -1) {
-                references.add(new RuleSetReferenceId(referenceString));
+                references.add(new RuleSetReferenceId(referenceString, null, warnDeprecated));
             } else {
                 for (String name : referenceString.split(",")) {
-                    references.add(new RuleSetReferenceId(name.trim()));
+                    references.add(new RuleSetReferenceId(name.trim(), null, warnDeprecated));
                 }
             }
         }
@@ -405,9 +437,9 @@ public InputStream getInputStream(final ResourceLoader rl) throws RuleSetNotFoun
             InputStream in = StringUtils.isBlank(ruleSetFileName) ? null
                     : rl.loadResourceAsStream(ruleSetFileName);
             if (in == null) {
-                throw new RuleSetNotFoundException("Can't find resource '" + ruleSetFileName + "' for rule '" + ruleName
+                throw new RuleSetNotFoundException("Cannot resolve rule/ruleset reference '" + originalRef
                         + "'" + ".  Make sure the resource is a valid file or URL and is on the CLASSPATH. "
-                        + "Here's the current classpath: " + System.getProperty("java.class.path"));
+                        + "Use --debug (or a fine log level) to see the current classpath.");
             }
             return in;
         } else {
@@ -422,8 +454,11 @@ public InputStream getInputStream(final ResourceLoader rl) throws RuleSetNotFoun
      *         <i>ruleSetFileName</i> for all Rule external references,
      *         <i>ruleSetFileName/ruleName</i>, for a single Rule external
      *         references, or <i>ruleName</i> otherwise.
+     *
+     * @deprecated Do not rely on the format of this method, it may be changed in PMD 7.
      */
     @Override
+    @Deprecated
     public String toString() {
         if (ruleSetFileName != null) {
             if (allRules) {