diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/Rule.java b/pmd-core/src/main/java/net/sourceforge/pmd/Rule.java index c3e91db4da..f9d8f249a2 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/Rule.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/Rule.java @@ -4,8 +4,8 @@ package net.sourceforge.pmd; +import java.util.Collection; import java.util.List; -import java.util.Set; import net.sourceforge.pmd.annotation.Experimental; import net.sourceforge.pmd.lang.Language; @@ -14,6 +14,8 @@ import net.sourceforge.pmd.lang.ParserOptions; import net.sourceforge.pmd.lang.ast.AstProcessingStage; import net.sourceforge.pmd.lang.ast.Node; import net.sourceforge.pmd.lang.rule.internal.TargetSelectionStrategy; +import net.sourceforge.pmd.lang.rule.internal.TargetSelectionStrategy.ClassRulechainVisits; +import net.sourceforge.pmd.lang.rule.internal.TargetSelectionStrategy.StringRulechainVisits; import net.sourceforge.pmd.properties.PropertySource; import net.sourceforge.pmd.properties.StringProperty; @@ -289,57 +291,20 @@ public interface Rule extends PropertySource { /** - * Gets whether this Rule uses the RuleChain. - * - * @return true if RuleChain is used. + * Returns the object that selects the nodes to which this rule applies + * in a tree. */ - default boolean isRuleChain() { - return !getRuleChainVisits().isEmpty() || !getClassRuleChainVisits().isEmpty(); - } - - - /** - * Gets the collection of AST node names visited by the Rule on the - * RuleChain. - * - * @return the list of AST node names - */ - Set getRuleChainVisits(); - - - Set> getClassRuleChainVisits(); - - - /** - * Adds an AST node by class to be visited by the Rule on the RuleChain. - * - * @param nodeClass - * the AST node to add to the RuleChain visit list - */ - void addRuleChainVisit(Class nodeClass); - - /** - * Adds an AST node by XPath name to be visited by the Rule on the RuleChain. - * - * @param astNodeName - * the XPath name of the node to add to the RuleChain visit list - * - * @see Node#getXPathNodeName() - */ - void addRuleChainVisit(String astNodeName); - - TargetSelectionStrategy getTargetingStrategy(); /** * Start processing. Called once, before apply() is first called. * - * @param ctx - * the rule context + * @param ctx the rule context */ void start(RuleContext ctx); + /** * Apply this rule to the given collection of nodes, using the given * context. @@ -365,4 +330,25 @@ public interface Rule extends PropertySource { * @return A new exact copy of this rule */ Rule deepCopy(); + + + static TargetSelectionStrategy visitNodesNamed(Collection names) { + if (names.isEmpty()) { + throw new IllegalArgumentException("Cannot visit zero nodes"); + } + return new StringRulechainVisits(names); + } + + + static TargetSelectionStrategy visitNodesWithType(Collection> types) { + if (types.isEmpty()) { + throw new IllegalArgumentException("Cannot visit zero types"); + } + return new ClassRulechainVisits(types); + } + + + static TargetSelectionStrategy visitRootOnly() { + return ClassRulechainVisits.ROOT_ONLY; + } } diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/RuleSets.java b/pmd-core/src/main/java/net/sourceforge/pmd/RuleSets.java index 8178803085..995569bcd0 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/RuleSets.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/RuleSets.java @@ -13,7 +13,6 @@ import java.util.List; import java.util.Set; import java.util.stream.Collectors; -import net.sourceforge.pmd.lang.Language; import net.sourceforge.pmd.lang.ast.Node; import net.sourceforge.pmd.lang.rule.internal.RuleApplicator; diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/AbstractDelegateRule.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/AbstractDelegateRule.java index 5fa2ad503e..7ccb154704 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/AbstractDelegateRule.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/AbstractDelegateRule.java @@ -235,31 +235,6 @@ public abstract class AbstractDelegateRule implements Rule { return rule.getPropertiesByPropertyDescriptor(); } - @Override - public boolean isRuleChain() { - return rule.isRuleChain(); - } - - @Override - public Set getRuleChainVisits() { - return rule.getRuleChainVisits(); - } - - @Override - public Set> getClassRuleChainVisits() { - return rule.getClassRuleChainVisits(); - } - - @Override - public void addRuleChainVisit(Class nodeClass) { - rule.addRuleChainVisit(nodeClass); - } - - @Override - public void addRuleChainVisit(String astNodeName) { - rule.addRuleChainVisit(astNodeName); - } - @Override public TargetSelectionStrategy getTargetingStrategy() { return rule.getTargetingStrategy(); diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/AbstractRule.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/AbstractRule.java index f11b0cae4f..058689f701 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/AbstractRule.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/AbstractRule.java @@ -22,7 +22,6 @@ import net.sourceforge.pmd.lang.ast.Node; import net.sourceforge.pmd.lang.ast.RootNode; import net.sourceforge.pmd.lang.rule.internal.TargetSelectionStrategy; import net.sourceforge.pmd.lang.rule.internal.TargetSelectionStrategy.ClassRulechainVisits; -import net.sourceforge.pmd.lang.rule.internal.TargetSelectionStrategy.StringRulechainVisits; import net.sourceforge.pmd.properties.AbstractPropertySource; import net.sourceforge.pmd.properties.PropertyDescriptor; @@ -235,53 +234,37 @@ public abstract class AbstractRule extends AbstractPropertySource implements Rul return new ParserOptions(); } - @Override - public boolean isRuleChain() { - return !getRuleChainVisits().isEmpty(); - } - @Override - public Set getRuleChainVisits() { - return ruleChainVisits; - } - - @Override - public Set> getClassRuleChainVisits() { + protected Set> getClassRuleChainVisits() { if (classRuleChainVisits.isEmpty() && ruleChainVisits.isEmpty()) { return Collections.singleton(RootNode.class); } return classRuleChainVisits; } - @Override - public void addRuleChainVisit(Class nodeClass) { + + /** + * @deprecated Override {@link #buildTargetingStrategy()}, this is + * provided for legacy compatibility + */ + @Deprecated + protected void addRuleChainVisit(Class nodeClass) { classRuleChainVisits.add(nodeClass); } @Override - public void addRuleChainVisit(String astNodeName) { - ruleChainVisits.add(astNodeName); - } - - @Override - public TargetSelectionStrategy getTargetingStrategy() { + public final TargetSelectionStrategy getTargetingStrategy() { if (myStrategy == null) { - myStrategy = buildSelectionStrat(); + myStrategy = buildTargetingStrategy(); } return myStrategy; } @NonNull - private TargetSelectionStrategy buildSelectionStrat() { - Set rvs = getRuleChainVisits(); + protected TargetSelectionStrategy buildTargetingStrategy() { Set> crvs = getClassRuleChainVisits(); - if (rvs.isEmpty() && crvs.isEmpty()) { - return ClassRulechainVisits.ROOT_ONLY; - } else if (rvs.isEmpty()) { - return new ClassRulechainVisits(crvs); - } else { - return new StringRulechainVisits(rvs); - } + return crvs.isEmpty() ? ClassRulechainVisits.ROOT_ONLY + : Rule.visitNodesWithType(crvs); } @Override @@ -426,4 +409,6 @@ public abstract class AbstractRule extends AbstractPropertySource implements Rul } return rule; } + + } diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/XPathRule.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/XPathRule.java index f790c0874f..a2fb25a23d 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/XPathRule.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/XPathRule.java @@ -13,16 +13,17 @@ import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; -import java.util.Set; import java.util.logging.Level; import java.util.logging.Logger; import org.apache.commons.lang3.StringUtils; +import org.checkerframework.checker.nullness.qual.NonNull; import net.sourceforge.pmd.Rule; import net.sourceforge.pmd.RuleContext; import net.sourceforge.pmd.lang.ast.AstProcessingStage; import net.sourceforge.pmd.lang.ast.Node; +import net.sourceforge.pmd.lang.rule.internal.TargetSelectionStrategy; import net.sourceforge.pmd.lang.ast.xpath.internal.DeprecatedAttrLogger; import net.sourceforge.pmd.lang.rule.xpath.JaxenXPathRuleQuery; import net.sourceforge.pmd.lang.rule.xpath.SaxonXPathRuleQuery; @@ -72,7 +73,7 @@ public class XPathRule extends AbstractRule { .build(); /** - * This is initialized only once when calling {@link #evaluate(Node, RuleContext)} or {@link #getRuleChainVisits()}. + * This is initialized only once when calling {@link #evaluate(Node, RuleContext)} {@link #getTargetingStrategy()}. */ private XPathRuleQuery xpathRuleQuery; @@ -216,16 +217,17 @@ public class XPathRule extends AbstractRule { } @Override - public Set getRuleChainVisits() { + protected @NonNull TargetSelectionStrategy buildTargetingStrategy() { if (xPathRuleQueryNeedsInitialization()) { initXPathRuleQuery(); - - for (String nodeName : xpathRuleQuery.getRuleChainVisits()) { - super.addRuleChainVisit(nodeName); - } - logXPathRuleChainUsage(!xpathRuleQuery.getRuleChainVisits().isEmpty()); } - return super.getRuleChainVisits(); + + List visits = xpathRuleQuery.getRuleChainVisits(); + + logXPathRuleChainUsage(!visits.isEmpty()); + + return visits.isEmpty() ? Rule.visitRootOnly() + : Rule.visitNodesNamed(visits); } diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/internal/TargetSelectionStrategy.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/internal/TargetSelectionStrategy.java index e9c1097032..e3f9f84426 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/internal/TargetSelectionStrategy.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/internal/TargetSelectionStrategy.java @@ -5,7 +5,9 @@ package net.sourceforge.pmd.lang.rule.internal; +import java.util.Collection; import java.util.Collections; +import java.util.HashSet; import java.util.Iterator; import java.util.Set; @@ -28,8 +30,8 @@ public abstract class TargetSelectionStrategy { private final Set visits; - public StringRulechainVisits(Set visits) { - this.visits = visits; + public StringRulechainVisits(Collection visits) { + this.visits = new HashSet<>(visits); } @@ -43,13 +45,12 @@ public abstract class TargetSelectionStrategy { public static final TargetSelectionStrategy ROOT_ONLY = new ClassRulechainVisits(Collections.singleton(RootNode.class)); - private final Set> visits; + private final Set> visits; - public ClassRulechainVisits(Set> visits) { - this.visits = visits; + public ClassRulechainVisits(Collection> visits) { + this.visits = new HashSet<>(visits); } - @Override Iterator getVisitedNodes(NodeIdx index) { return index.getByClass(visits).iterator(); diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/util/CollectionUtil.java b/pmd-core/src/main/java/net/sourceforge/pmd/util/CollectionUtil.java index 642b6a3098..f3e32550af 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/util/CollectionUtil.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/util/CollectionUtil.java @@ -183,15 +183,6 @@ public final class CollectionUtil { return map; } - - @SafeVarargs - public static Set setOf(T first, T... ts) { - LinkedHashSet set = new LinkedHashSet<>(ts.length + 1); - set.add(first); - Collections.addAll(set, ts); - return set; - } - /** * Consumes all the elements of the iterator and * returns a list containing them. The iterator is diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/AbstractRuleTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/AbstractRuleTest.java index 276c8d306c..df36b33a60 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/AbstractRuleTest.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/AbstractRuleTest.java @@ -221,7 +221,6 @@ public class AbstractRuleTest { assertEquals(r1.getName(), r2.getName()); assertEquals(r1.getPriority(), r2.getPriority()); assertEquals(r1.getPropertyDescriptors(), r2.getPropertyDescriptors()); - assertEquals(r1.getRuleChainVisits(), r2.getRuleChainVisits()); assertEquals(r1.getRuleClass(), r2.getRuleClass()); assertEquals(r1.getRuleSetName(), r2.getRuleSetName()); assertEquals(r1.getSince(), r2.getSince()); diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/FooRule.java b/pmd-core/src/test/java/net/sourceforge/pmd/FooRule.java index 6d37897942..cfdd90b706 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/FooRule.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/FooRule.java @@ -4,22 +4,33 @@ package net.sourceforge.pmd; +import static net.sourceforge.pmd.util.CollectionUtil.setOf; + import java.util.List; +import org.checkerframework.checker.nullness.qual.NonNull; + import net.sourceforge.pmd.lang.DummyLanguageModule; import net.sourceforge.pmd.lang.LanguageRegistry; import net.sourceforge.pmd.lang.ast.Node; import net.sourceforge.pmd.lang.rule.AbstractRule; +import net.sourceforge.pmd.lang.rule.internal.TargetSelectionStrategy; /** * Sample rule that detect any node with an image of "Foo". Used for testing. */ public class FooRule extends AbstractRule { + public FooRule() { setLanguage(LanguageRegistry.getLanguage(DummyLanguageModule.NAME)); setName("Foo"); } + @Override + protected @NonNull TargetSelectionStrategy buildTargetingStrategy() { + return Rule.visitNodesNamed(setOf("dummyNode")); + } + @Override public String getMessage() { return "blah"; diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/RuleSetTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/RuleSetTest.java index fffff19bd1..1b4b0fdddb 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/RuleSetTest.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/RuleSetTest.java @@ -4,6 +4,7 @@ package net.sourceforge.pmd; +import static net.sourceforge.pmd.util.CollectionUtil.setOf; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; @@ -22,6 +23,7 @@ import java.util.Random; import java.util.Set; import java.util.regex.Pattern; +import org.checkerframework.checker.nullness.qual.NonNull; import org.junit.Test; import net.sourceforge.pmd.Report.ProcessingError; @@ -35,6 +37,7 @@ import net.sourceforge.pmd.lang.ast.DummyRoot; import net.sourceforge.pmd.lang.ast.Node; import net.sourceforge.pmd.lang.rule.MockRule; import net.sourceforge.pmd.lang.rule.RuleReference; +import net.sourceforge.pmd.lang.rule.internal.TargetSelectionStrategy; public class RuleSetTest { @@ -402,8 +405,6 @@ public class RuleSetTest { Rule rule = new FooRule(); rule.setName("FooRule1"); rule.setLanguage(LanguageRegistry.getLanguage(DummyLanguageModule.NAME)); - rule.addRuleChainVisit("dummyRootNode"); - assertTrue("RuleChain rule", rule.isRuleChain()); RuleSet ruleSet1 = createRuleSetBuilder("RuleSet1") .addRule(rule) .build(); @@ -446,7 +447,6 @@ public class RuleSetTest { Rule rule = new FooRule(); rule.setName("FooRule1"); rule.setLanguage(LanguageRegistry.getLanguage(DummyLanguageModule.NAME)); - rule.addRuleChainVisit("dummyNode"); RuleSet ruleSet1 = createRuleSetBuilder("RuleSet1") .addRule(rule) .build(); @@ -568,8 +568,10 @@ public class RuleSetTest { @Test public void ruleExceptionShouldNotStopProcessingFileWithRuleChain() { RuleSet ruleset = createRuleSetBuilder("ruleExceptionShouldBeReported").addRule(new MockRule() { - { - addRuleChainVisit("dummyRootNode"); + + @Override + protected @NonNull TargetSelectionStrategy buildTargetingStrategy() { + return Rule.visitNodesNamed(setOf("dummyRootNode")); } @Override @@ -577,8 +579,10 @@ public class RuleSetTest { throw new RuntimeException("Test exception while applying rule"); } }).addRule(new MockRule() { - { - addRuleChainVisit("dummyRootNode"); + + @Override + protected @NonNull TargetSelectionStrategy buildTargetingStrategy() { + return Rule.visitNodesNamed(setOf("dummyRootNode")); } @Override diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/FormalParameterNamingConventionsRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/FormalParameterNamingConventionsRule.java index 4a78682992..65d297ee30 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/FormalParameterNamingConventionsRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/FormalParameterNamingConventionsRule.java @@ -5,9 +5,14 @@ package net.sourceforge.pmd.lang.java.rule.codestyle; +import static net.sourceforge.pmd.util.CollectionUtil.setOf; + import java.util.regex.Pattern; +import org.checkerframework.checker.nullness.qual.NonNull; + import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId; +import net.sourceforge.pmd.lang.rule.internal.TargetSelectionStrategy; import net.sourceforge.pmd.properties.PropertyDescriptor; @@ -34,10 +39,12 @@ public final class FormalParameterNamingConventionsRule extends AbstractNamingCo definePropertyDescriptor(finalFormalParamRegex); definePropertyDescriptor(lambdaParamRegex); definePropertyDescriptor(explicitLambdaParamRegex); - - addRuleChainVisit(ASTVariableDeclaratorId.class); } + @Override + protected @NonNull TargetSelectionStrategy buildTargetingStrategy() { + return visitNodesWithType(setOf(ASTVariableDeclaratorId.class)); + } @Override public Object visit(ASTVariableDeclaratorId node, Object data) { diff --git a/pmd-test/src/main/java/net/sourceforge/pmd/AbstractLanguageVersionTest.java b/pmd-test/src/main/java/net/sourceforge/pmd/AbstractLanguageVersionTest.java index 0827d2aa03..f1a4417601 100644 --- a/pmd-test/src/main/java/net/sourceforge/pmd/AbstractLanguageVersionTest.java +++ b/pmd-test/src/main/java/net/sourceforge/pmd/AbstractLanguageVersionTest.java @@ -119,7 +119,7 @@ public class AbstractLanguageVersionTest { @Test public void testOldRegisteredRulesets() throws Exception { // only check for languages, that support rules - if (expected == null || expected.getLanguage().getRuleChainVisitorClass() == null) { + if (expected == null) { return; } diff --git a/pmd-test/src/test/java/net/sourceforge/pmd/testframework/RuleTstTest.java b/pmd-test/src/test/java/net/sourceforge/pmd/testframework/RuleTstTest.java index 815d3562ce..be273637a5 100644 --- a/pmd-test/src/test/java/net/sourceforge/pmd/testframework/RuleTstTest.java +++ b/pmd-test/src/test/java/net/sourceforge/pmd/testframework/RuleTstTest.java @@ -43,13 +43,14 @@ public class RuleTstTest { Report report = new Report(); when(rule.getLanguage()).thenReturn(dummyLanguage.getLanguage()); when(rule.getName()).thenReturn("test rule"); + when(rule.getTargetingStrategy()).thenReturn(Rule.visitRootOnly()); ruleTester.runTestFromString("the code", rule, report, dummyLanguage, false); verify(rule).start(any(RuleContext.class)); verify(rule).end(any(RuleContext.class)); - verify(rule, times(2)).getLanguage(); - verify(rule, times(2)).isRuleChain(); + verify(rule).getLanguage(); + verify(rule).getTargetingStrategy(); verify(rule).getMinimumLanguageVersion(); verify(rule).getMaximumLanguageVersion(); verify(rule).apply(anyList(), any(RuleContext.class)); @@ -62,6 +63,8 @@ public class RuleTstTest { public void shouldAssertLinenumbersSorted() { when(rule.getLanguage()).thenReturn(dummyLanguage.getLanguage()); when(rule.getName()).thenReturn("test rule"); + when(rule.getTargetingStrategy()).thenReturn(Rule.visitRootOnly()); + Mockito.doAnswer(new Answer() { private RuleViolation createViolation(RuleContext context, int beginLine, String message) { DummyNode node = new DummyNode();