Better integrate pmd#3922 and pmd#3958

They were merged on separate branches

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

@@ -9,9 +9,11 @@
 import static net.sourceforge.pmd.util.internal.xml.SchemaConstants.EXCLUDE;
 import static net.sourceforge.pmd.util.internal.xml.SchemaConstants.EXCLUDE_PATTERN;
 import static net.sourceforge.pmd.util.internal.xml.SchemaConstants.INCLUDE_PATTERN;
+import static net.sourceforge.pmd.util.internal.xml.SchemaConstants.NAME;
 import static net.sourceforge.pmd.util.internal.xml.SchemaConstants.PRIORITY;
 import static net.sourceforge.pmd.util.internal.xml.SchemaConstants.REF;
 import static net.sourceforge.pmd.util.internal.xml.SchemaConstants.RULE;
+import static net.sourceforge.pmd.util.internal.xml.SchemaConstants.RULESET;
 
 import java.io.IOException;
 import java.io.InputStream;
@@ -38,6 +40,7 @@
 import org.slf4j.event.Level;
 import org.w3c.dom.Document;
 import org.w3c.dom.Element;
+import org.w3c.dom.Node;
 import org.w3c.dom.NodeList;
 import org.xml.sax.InputSource;
 
@@ -47,10 +50,10 @@
 import net.sourceforge.pmd.util.ResourceLoader;
 import net.sourceforge.pmd.util.StringUtil;
 import net.sourceforge.pmd.util.internal.xml.PmdXmlReporter;
-import net.sourceforge.pmd.util.internal.xml.SchemaConstants;
 import net.sourceforge.pmd.util.internal.xml.XmlErrorMessages;
 import net.sourceforge.pmd.util.internal.xml.XmlUtil;
 import net.sourceforge.pmd.util.log.MessageReporter;
+import net.sourceforge.pmd.util.log.internal.NoopReporter;
 
 import com.github.oowekyala.ooxml.DomUtils;
 import com.github.oowekyala.ooxml.messages.NiceXmlMessageSpec;
@@ -242,7 +245,7 @@ private RuleSet parseRulesetNode(RuleSetReferenceId ruleSetReferenceId,
             } else {
                 err.at(node).error(XmlErrorMessages.ERR__UNEXPECTED_ELEMENT_IN,
                                    node.getTagName(),
-                                   SchemaConstants.RULESET);
+                                   RULESET);
             }
         }
 
@@ -322,9 +325,9 @@ private void parseRuleNode(RuleSetReferenceId ruleSetReferenceId,
                                boolean withDeprecatedRuleReferences,
                                Set<String> rulesetReferences,
                                PmdXmlReporter err) {
-        if (ruleNode.hasAttribute("ref")) {
-            String ref = ruleNode.getAttribute("ref");
-            RuleSetReferenceId refId = parseReferenceAndWarn(ruleSetBuilder, ref);
+        if (REF.hasAttribute(ruleNode)) {
+            String ref = REF.getAttributeOrThrow(ruleNode, err);
+            RuleSetReferenceId refId = parseReferenceAndWarn(ref, REF.getAttributeNode(ruleNode), err);
             if (refId != null) {
                 if (refId.isAllRules()) {
                     parseRuleSetReferenceNode(ruleSetBuilder, ruleNode, ref, refId, rulesetReferences, err);
@@ -362,7 +365,7 @@ private void parseRuleSetReferenceNode(RuleSetBuilder ruleSetBuilder,
             if (EXCLUDE.matchesElt(child)) {
                 String excludedRuleName;
                 try {
-                    excludedRuleName = SchemaConstants.NAME.getAttributeOrThrow(child, err);
+                    excludedRuleName = NAME.getAttributeOrThrow(child, err);
                 } catch (XmlException ignored) {
                     // has been reported
                     continue;
@@ -431,19 +434,23 @@ private void parseRuleSetReferenceNode(RuleSetBuilder ruleSetBuilder,
         rulesetReferences.add(ref);
     }
 
-    private RuleSetReferenceId parseReferenceAndWarn(RuleSetBuilder ruleSetBuilder, String ref) {
+    private RuleSetReferenceId parseReferenceAndWarn(String ref,
+                                                     Node xmlPlace,
+                                                     PmdXmlReporter err) {
         ref = compatibilityFilter.applyRef(ref, this.warnDeprecated);
         if (ref == null) {
-            LOG.debug("Rule ref {} references a deleted rule, ignoring", ref);
+            err.at(xmlPlace).warn("Rule reference references a deleted rule, ignoring");
             return null; // deleted rule
         }
+        // only emit a warning if we check for deprecated syntax
+        MessageReporter subReporter = warnDeprecated ? err.at(xmlPlace) : new NoopReporter();
 
-        List<RuleSetReferenceId> references = RuleSetReferenceId.parse(ref, warnDeprecated);
+        List<RuleSetReferenceId> references = RuleSetReferenceId.parse(ref, subReporter);
         if (references.size() > 1 && warnDeprecated) {
-            LOG.warn("Using a comma separated list as a ref attribute is deprecated. "
-                        + "All references but the first are ignored. Reference: '{}'", ref);
+            err.at(xmlPlace).warn("Using a comma separated list as a ref attribute is deprecated. "
+                                      + "All references but the first are ignored.");
         } else if (references.isEmpty()) {
-            LOG.warn("Empty ref attribute in ruleset '{}'", ruleSetBuilder.getName());
+            err.at(xmlPlace).warn("Empty ref attribute");
             return null;
         }
         return references.get(0);
@@ -518,11 +525,11 @@ private void parseRuleReferenceNode(RuleSetReferenceId ruleSetReferenceId,
         boolean isSameRuleSet = false;
         if (!otherRuleSetReferenceId.isExternal()
             && containsRule(ruleSetReferenceId, otherRuleSetReferenceId.getRuleName())) {
-            otherRuleSetReferenceId = new RuleSetReferenceId(ref, ruleSetReferenceId);
+            otherRuleSetReferenceId = new RuleSetReferenceId(ref, ruleSetReferenceId, err.at(REF.getAttributeNode(ruleNode)));
             isSameRuleSet = true;
         } else if (otherRuleSetReferenceId.isExternal()
             && otherRuleSetReferenceId.getRuleSetFileName().equals(ruleSetReferenceId.getRuleSetFileName())) {
-            otherRuleSetReferenceId = new RuleSetReferenceId(otherRuleSetReferenceId.getRuleName(), ruleSetReferenceId);
+            otherRuleSetReferenceId = new RuleSetReferenceId(otherRuleSetReferenceId.getRuleName(), ruleSetReferenceId, err.at(REF.getAttributeNode(ruleNode)));
             isSameRuleSet = true;
         }
         // do not ignore deprecated rule references

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

@@ -16,6 +16,7 @@
 
 import org.apache.commons.lang3.StringUtils;
 import org.checkerframework.checker.nullness.qual.NonNull;
+import org.checkerframework.checker.nullness.qual.Nullable;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -144,6 +145,9 @@ public RuleSetFactory toFactory() {
         );
     }
 
+    private @Nullable MessageReporter filteredReporter() {
+        return warnDeprecated ? reporter : null;
+    }
 
     /**
      * Parses and returns a ruleset from its location. The location may
@@ -154,7 +158,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, null, warnDeprecated));
+        return loadFromResource(new RuleSetReferenceId(rulesetPath, null, filteredReporter()));
     }
 
     /**
@@ -166,7 +170,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, null, warnDeprecated) {
+        return loadFromResource(new RuleSetReferenceId(filename, null, filteredReporter()) {
             @Override
             public InputStream getInputStream(ResourceLoader rl) {
                 return new ByteArrayInputStream(rulesetXmlContent.getBytes(StandardCharsets.UTF_8));

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

@@ -21,6 +21,7 @@
 
 import net.sourceforge.pmd.annotation.InternalApi;
 import net.sourceforge.pmd.util.ResourceLoader;
+import net.sourceforge.pmd.util.log.MessageReporter;
 
 /**
  * This class is used to parse a RuleSet reference value. Most commonly used for
@@ -105,7 +106,7 @@ public class RuleSetReferenceId {
      */
     public RuleSetReferenceId(final String id) {
 
-        this(id, null);
+        this(id, null, null);
     }
 
     private RuleSetReferenceId(final String ruleSetFileName, boolean external, String ruleName, RuleSetReferenceId externalRuleSetReferenceId) {
@@ -132,7 +133,7 @@ private RuleSetReferenceId(final String ruleSetFileName, boolean external, Strin
      *                                  RuleSetReferenceId.
      */
     public RuleSetReferenceId(final String id, final RuleSetReferenceId externalRuleSetReferenceId) {
-        this(id, externalRuleSetReferenceId, false);
+        this(id, externalRuleSetReferenceId, null);
     }
 
     /**
@@ -149,7 +150,9 @@ public RuleSetReferenceId(final String id, final RuleSetReferenceId externalRule
      * @throws IllegalArgumentException If the ID is not Rule reference when there is an external
      *                                  RuleSetReferenceId.
      */
-    RuleSetReferenceId(final String id, final RuleSetReferenceId externalRuleSetReferenceId, boolean warnDeprecated) {
+    RuleSetReferenceId(final String id,
+                       final RuleSetReferenceId externalRuleSetReferenceId,
+                       final @Nullable MessageReporter err) {
         this.originalRef = id;
 
         if (externalRuleSetReferenceId != null && !externalRuleSetReferenceId.isExternal()) {
@@ -179,7 +182,7 @@ public RuleSetReferenceId(final String id, final RuleSetReferenceId externalRule
         } else {
             String tempRuleName = getRuleName(id);
             String tempRuleSetFileName = tempRuleName != null && id != null
-                    ? id.substring(0, id.length() - tempRuleName.length() - 1) : id;
+                                         ? id.substring(0, id.length() - tempRuleName.length() - 1) : id;
 
             if (isValidUrl(tempRuleSetFileName)) {
                 // remaining part is a xml ruleset file, so the tempRuleName is
@@ -208,9 +211,9 @@ public RuleSetReferenceId(final String id, final RuleSetReferenceId externalRule
                 String expandedRuleset = resolveDeprecatedBuiltInRulesetShorthand(tempRuleSetFileName);
                 String builtinRuleSet = expandedRuleset == null ? tempRuleSetFileName : expandedRuleset;
                 if (checkRulesetExists(builtinRuleSet)) {
-                    if (expandedRuleset != null && warnDeprecated) {
-                        LOG.warn(
-                            "Ruleset reference '{}' uses a deprecated form, use '{}' instead",
+                    if (expandedRuleset != null && err != null) {
+                        err.warn(
+                            "Ruleset reference ''{0}'' uses a deprecated form, use ''{1}'' instead",
                             tempRuleSetFileName, builtinRuleSet
                         );
                     }
@@ -376,27 +379,21 @@ private static boolean isFullRuleSetName(String name) {
      *
      * @return The corresponding List of RuleSetReferenceId instances.
      */
+    // TODO deprecate and remove
     public static List<RuleSetReferenceId> parse(String referenceString) {
-        return parse(referenceString, false);
+        return parse(referenceString, null);
     }
 
-    /**
-     * 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) {
+    static List<RuleSetReferenceId> parse(String referenceString,
+                                          MessageReporter err) {
         List<RuleSetReferenceId> references = new ArrayList<>();
         if (referenceString != null && referenceString.trim().length() > 0) {
 
             if (referenceString.indexOf(',') == -1) {
-                references.add(new RuleSetReferenceId(referenceString, null, warnDeprecated));
+                references.add(new RuleSetReferenceId(referenceString, null, err));
             } else {
                 for (String name : referenceString.split(",")) {
-                    references.add(new RuleSetReferenceId(name.trim(), null, warnDeprecated));
+                    references.add(new RuleSetReferenceId(name.trim(), null, err));
                 }
             }
         }
@@ -407,7 +404,7 @@ public static List<RuleSetReferenceId> parse(String referenceString, boolean war
      * Is this an external RuleSet reference?
      *
      * @return <code>true</code> if this is an external reference,
-     *         <code>false</code> otherwise.
+     *     <code>false</code> otherwise.
      */
     public boolean isExternal() {
         return external;

pmd-core/src/test/java/net/sourceforge/pmd/RuleSetFactoryTest.java

@@ -135,7 +135,7 @@ void testSingleRuleEmptyRef() throws Exception {
             assertEquals("net.sourceforge.pmd.lang.rule.MockRule", r.getRuleClass());
             assertEquals("avoid the mock rule", r.getMessage());
         });
-        assertThat(log, containsString("Empty ref attribute in ruleset 'test'"));
+        assertThat(log, containsString("Empty ref attribute"));
     }
 
     @Test
@@ -910,50 +910,48 @@ void testWrongRuleNameExcluded() {
      */
     @Test
     void testExcludeAndImportTwice() {
-        RuleSet ruleset = loadRuleSet("<?xml version=\"1.0\"?>\n" + "<ruleset name=\"Custom ruleset for tests\"\n"
-                                          + "    xmlns=\"http://pmd.sourceforge.net/ruleset/2.0.0\"\n"
-                                          + "    xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\"\n"
-                                          + "    xsi:schemaLocation=\"http://pmd.sourceforge.net/ruleset/2.0.0 https://pmd.sourceforge.io/ruleset_2_0_0.xsd\">\n"
-                                          + "  <description>Custom ruleset for tests</description>\n"
-                                          + "  <rule ref=\"rulesets/dummy/basic.xml\">\n"
-                                          + "    <exclude name=\"DummyBasicMockRule\"/>\n"
-                                          + "  </rule>\n" + "</ruleset>\n");
+        RuleSet ruleset = loadRuleSet(
+            rulesetXml(
+                rulesetRef("rulesets/dummy/basic.xml",
+                           excludeRule("DummyBasicMockRule")
+                )
+            )
+        );
+
         assertNull(ruleset.getRuleByName("DummyBasicMockRule"));
 
-        RuleSet ruleset2 = loadRuleSet("<?xml version=\"1.0\"?>\n" + "<ruleset name=\"Custom ruleset for tests\"\n"
-                                           + "    xmlns=\"http://pmd.sourceforge.net/ruleset/2.0.0\"\n"
-                                           + "    xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\"\n"
-                                           + "    xsi:schemaLocation=\"http://pmd.sourceforge.net/ruleset/2.0.0 https://pmd.sourceforge.io/ruleset_2_0_0.xsd\">\n"
-                                           + "  <description>Custom ruleset for tests</description>\n"
-                                           + "  <rule ref=\"rulesets/dummy/basic.xml\">\n"
-                                           + "    <exclude name=\"DummyBasicMockRule\"/>\n"
-                                           + "  </rule>\n" + "  <rule ref=\"rulesets/dummy/basic.xml\"/>\n"
-                                           + "</ruleset>\n");
+        RuleSet ruleset2 = loadRuleSet(
+            rulesetXml(
+                rulesetRef("rulesets/dummy/basic.xml",
+                           excludeRule("DummyBasicMockRule")
+                ),
+                rulesetRef("rulesets/dummy/basic.xml")
+            )
+        );
         assertNotNull(ruleset2.getRuleByName("DummyBasicMockRule"));
 
-        RuleSet ruleset3 = loadRuleSet("<?xml version=\"1.0\"?>\n" + "<ruleset name=\"Custom ruleset for tests\"\n"
-                                           + "    xmlns=\"http://pmd.sourceforge.net/ruleset/2.0.0\"\n"
-                                           + "    xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\"\n"
-                                           + "    xsi:schemaLocation=\"http://pmd.sourceforge.net/ruleset/2.0.0 https://pmd.sourceforge.io/ruleset_2_0_0.xsd\">\n"
-                                           + "  <description>Custom ruleset for tests</description>\n"
-                                           + "  <rule ref=\"rulesets/dummy/basic.xml\"/>\n"
-                                           + "  <rule ref=\"rulesets/dummy/basic.xml\">\n"
-                                           + "    <exclude name=\"DummyBasicMockRule\"/>\n" + "  </rule>\n"
-                                           + "</ruleset>\n");
+        RuleSet ruleset3 = loadRuleSet(
+            rulesetXml(
+                rulesetRef("rulesets/dummy/basic.xml"),
+                rulesetRef("rulesets/dummy/basic.xml",
+                           excludeRule("DummyBasicMockRule")
+                )
+            )
+        );
         assertNotNull(ruleset3.getRuleByName("DummyBasicMockRule"));
     }
 
     @Test
     void testMissingRuleSetNameIsWarning() throws Exception {
-        String log = SystemLambda.tapSystemErr(() -> {
+        SystemLambda.tapSystemErr(() -> {
             loadRuleSetWithDeprecationWarnings(
-                    "<?xml version=\"1.0\"?>\n" + "<ruleset \n"
-                            + "    xmlns=\"http://pmd.sourceforge.net/ruleset/2.0.0\"\n"
-                            + "    xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\"\n"
-                            + "    xsi:schemaLocation=\"http://pmd.sourceforge.net/ruleset/2.0.0 https://pmd.sourceforge.io/ruleset_2_0_0.xsd\">\n"
-                            + "  <description>Custom ruleset for tests</description>\n"
-                            + "  <rule ref=\"rulesets/dummy/basic.xml\"/>\n"
-                            + "  </ruleset>\n"
+                "<?xml version=\"1.0\"?>\n" + "<ruleset \n"
+                    + "    xmlns=\"http://pmd.sourceforge.net/ruleset/2.0.0\"\n"
+                    + "    xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\"\n"
+                    + "    xsi:schemaLocation=\"http://pmd.sourceforge.net/ruleset/2.0.0 https://pmd.sourceforge.io/ruleset_2_0_0.xsd\">\n"
+                    + "  <description>Custom ruleset for tests</description>\n"
+                    + "  <rule ref=\"rulesets/dummy/basic.xml\"/>\n"
+                    + "  </ruleset>\n"
             );
         });
 
@@ -975,11 +973,12 @@ public void testMissingRuleSetDescriptionIsWarning() {
 
     @Test
     void testDeprecatedRulesetReferenceProducesWarning() throws Exception {
-        SystemLambda.tapSystemErr(
+        String log = SystemLambda.tapSystemErr(
             () -> loadRuleSetWithDeprecationWarnings(
                 rulesetXml(
                     ruleRef("dummy-basic")
                 )));
+        System.out.println(log);
 
         verifyFoundAWarningWithMessage(containing(
             "Ruleset reference 'dummy-basic' uses a deprecated form, use 'rulesets/dummy/basic.xml' instead"
@@ -1109,7 +1108,8 @@ void testDeprecatedRulesetReferenceProducesWarning() throws Exception {
             )
         );
 
-    private static final String SINGLE_RULE_EMPTY_REF = "<?xml version=\"1.0\"?>\n"
+    private static final String SINGLE_RULE_EMPTY_REF =
+        "<?xml version=\"1.0\"?>\n"
         + "<ruleset name=\"test\">\n"
         + "<description>testdesc</description>\n"
         + "<rule \n"

pmd-core/src/test/java/net/sourceforge/pmd/cli/CoreCliTest.java

@@ -161,7 +161,7 @@ void testRelativeReportFile() throws Exception {
         String reportFile = "reportFile.txt";
         Path absoluteReportFile = FileSystems.getDefault().getPath(reportFile).toAbsolutePath();
         // verify the file doesn't exist yet - we will delete the file at the end!
-        assertFalse(Files.exists(absoluteReportFile), "Report file must not exist yet!");
+        assertFalse(Files.exists(absoluteReportFile), "Report file must not exist yet! " + absoluteReportFile);
 
         try {
             runPmdSuccessfully("--no-cache", "-d", srcDir, "-R", DUMMY_RULESET, "-r", reportFile);