diff --git a/docs/pages/pmd/userdocs/extending/writing_xpath_rules.md b/docs/pages/pmd/userdocs/extending/writing_xpath_rules.md index 5b8f886623..a24a512cf7 100644 --- a/docs/pages/pmd/userdocs/extending/writing_xpath_rules.md +++ b/docs/pages/pmd/userdocs/extending/writing_xpath_rules.md @@ -127,6 +127,18 @@ represented by our 1.0 implementation as strings, meaning that `@BeginLine > "1" worked ---that's not the case in 2.0 mode. * @ArgumentCount > '1' → `@ArgumentCount > 1` +* In XPath 1.0, the expression `/Foo` matches the *children* of the root named `Foo`. +In XPath 2.0, that expression matches the root, if it is named `Foo`. Consider the following tree: +```java +Foo +└─ Foo +└─ Foo +``` +Then `/Foo` will match the root in XPath 2, and the other nodes (but not the root) in XPath 1. +See eg [an issue caused by this](https://github.com/pmd/pmd/issues/1919#issuecomment-512865434) in Apex, +with nested classes. + + ## Rule properties **See [Defining rule properties](pmd_userdocs_extending_defining_properties.html#for-xpath-rules)** diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index b8f8783555..1f2ee87b35 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -16,9 +16,12 @@ This is a {{ site.pmd.release_type }} release. ### Fixed Issues +* pmd-core + * [#1939](https://github.com/pmd/pmd/issues/1939): \[core] XPath expressions return handling + + ### API Changes ### External Contributions {% endtocmaker %} - diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/xpath/internal/AstNodeOwner.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/xpath/internal/AstNodeOwner.java new file mode 100644 index 0000000000..3d7f3a7f7e --- /dev/null +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/xpath/internal/AstNodeOwner.java @@ -0,0 +1,15 @@ +/* + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.ast.xpath.internal; + +import net.sourceforge.pmd.lang.ast.Node; + +/** + * Marker interface. + */ +public interface AstNodeOwner { + + Node getUnderlyingNode(); +} diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/xpath/saxon/DocumentNode.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/xpath/saxon/DocumentNode.java index d91f1828a6..65c5ecec8b 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/xpath/saxon/DocumentNode.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/xpath/saxon/DocumentNode.java @@ -10,6 +10,7 @@ import java.util.Map; import net.sourceforge.pmd.annotation.InternalApi; import net.sourceforge.pmd.lang.ast.Node; +import net.sourceforge.pmd.lang.ast.xpath.internal.AstNodeOwner; import net.sourceforge.pmd.lang.ast.xpath.internal.DeprecatedAttrLogger; import net.sourceforge.pmd.lang.rule.xpath.SaxonXPathRuleQuery; @@ -27,7 +28,7 @@ import net.sf.saxon.type.Type; */ @Deprecated @InternalApi -public class DocumentNode extends BaseNodeInfo implements DocumentInfo { +public class DocumentNode extends BaseNodeInfo implements DocumentInfo, AstNodeOwner { /** * The root ElementNode of the DocumentNode. @@ -94,11 +95,20 @@ public class DocumentNode extends BaseNodeInfo implements DocumentInfo { return new Navigator.DescendantEnumeration(this, true, true); case Axis.CHILD: return SingleNodeIterator.makeIterator(rootNode); + case Axis.SELF: + return SingleNodeIterator.makeIterator(this); default: return super.iterateAxis(axisNumber); } } + @Override + public Node getUnderlyingNode() { + // this is a concession to the model, so that the expression "/" + // may be interpreted as the root node + return rootNode.getUnderlyingNode(); + } + public DeprecatedAttrLogger getAttrCtx() { return attrCtx == null ? DeprecatedAttrLogger.noop() : attrCtx; } diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/xpath/saxon/ElementNode.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/xpath/saxon/ElementNode.java index 6db157eac4..e9341542ea 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/xpath/saxon/ElementNode.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/xpath/saxon/ElementNode.java @@ -11,6 +11,7 @@ import java.util.Map; import net.sourceforge.pmd.annotation.InternalApi; import net.sourceforge.pmd.lang.ast.Node; import net.sourceforge.pmd.lang.ast.xpath.Attribute; +import net.sourceforge.pmd.lang.ast.xpath.internal.AstNodeOwner; import net.sourceforge.pmd.lang.rule.xpath.SaxonXPathRuleQuery; import net.sf.saxon.om.Axis; @@ -38,7 +39,7 @@ import net.sf.saxon.value.Value; */ @Deprecated @InternalApi -public class ElementNode extends BaseNodeInfo { +public class ElementNode extends BaseNodeInfo implements AstNodeOwner { protected final DocumentNode document; protected final ElementNode parent; @@ -94,7 +95,7 @@ public class ElementNode extends BaseNodeInfo { } @Override - public Object getUnderlyingNode() { + public Node getUnderlyingNode() { return node; } diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQuery.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQuery.java index 4bcb8f7423..2a8815798c 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQuery.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQuery.java @@ -17,6 +17,7 @@ import java.util.regex.Pattern; import net.sourceforge.pmd.RuleContext; import net.sourceforge.pmd.annotation.InternalApi; import net.sourceforge.pmd.lang.ast.Node; +import net.sourceforge.pmd.lang.ast.xpath.internal.AstNodeOwner; import net.sourceforge.pmd.lang.ast.xpath.internal.DeprecatedAttrLogger; import net.sourceforge.pmd.lang.ast.xpath.saxon.DocumentNode; import net.sourceforge.pmd.lang.ast.xpath.saxon.ElementNode; @@ -120,25 +121,21 @@ public class SaxonXPathRuleQuery extends AbstractXPathRuleQuery { assert rootElementNode != null : "Cannot find " + node; final XPathDynamicContext xpathDynamicContext = createDynamicContext(rootElementNode); - final List nodes = new LinkedList<>(); + final List results = new LinkedList<>(); List expressions = getXPathExpressionForNodeOrDefault(node.getXPathNodeName()); for (Expression expression : expressions) { SequenceIterator iterator = expression.iterate(xpathDynamicContext.getXPathContextObject()); Item current = iterator.next(); while (current != null) { - nodes.add((ElementNode) current); + if (current instanceof AstNodeOwner) { + results.add(((AstNodeOwner) current).getUnderlyingNode()); + } else { + throw new RuntimeException("XPath rule expression returned a non-node (" + current.getClass() + "): " + current); + } current = iterator.next(); } } - /* - Map List of Saxon Nodes -> List of AST Nodes, which were detected to match the XPath expression - (i.e. violation found) - */ - final List results = new ArrayList<>(nodes.size()); - for (final ElementNode elementNode : nodes) { - results.add((Node) elementNode.getUnderlyingNode()); - } Collections.sort(results, RuleChainAnalyzer.documentOrderComparator()); return results; } catch (final XPathException e) { diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQueryTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQueryTest.java index 0920d7c4e4..bfe925bac3 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQueryTest.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQueryTest.java @@ -9,10 +9,14 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import org.hamcrest.CoreMatchers; import org.junit.Assert; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import net.sourceforge.pmd.RuleContext; +import net.sourceforge.pmd.lang.ast.DummyNode; import net.sourceforge.pmd.lang.ast.DummyNodeWithListAndEnum; import net.sourceforge.pmd.lang.ast.Node; import net.sourceforge.pmd.properties.PropertyDescriptor; @@ -22,6 +26,9 @@ import net.sf.saxon.expr.Expression; public class SaxonXPathRuleQueryTest { + @Rule + public final ExpectedException expected = ExpectedException.none(); + @Test public void testListAttribute() { DummyNodeWithListAndEnum dummy = new DummyNodeWithListAndEnum(1); @@ -37,6 +44,44 @@ public class SaxonXPathRuleQueryTest { assertQuery(0, "//dummyNode[@EmptyList = (\"A\")]", dummy); } + @Test + public void testInvalidReturn() { + DummyNodeWithListAndEnum dummy = new DummyNodeWithListAndEnum(1); + + + expected.expect(RuntimeException.class); + expected.expectMessage(CoreMatchers.containsString("XPath rule expression returned a non-node")); + expected.expectMessage(CoreMatchers.containsString("Int64Value")); + + createQuery("1+2").evaluate(dummy, new RuleContext()); + } + + @Test + public void testRootExpression() { + DummyNode dummy = new DummyNode(); // todo in pmd 7 this should be a RootNode + + List result = assertQuery(1, "/", dummy); + Assert.assertEquals(dummy, result.get(0)); + } + + @Test + public void testRootExpressionIsADocumentNode() { + DummyNode dummy = new DummyNode(); // todo in pmd 7 this should be a RootNode + + List result = assertQuery(1, "(/)[self::document-node()]", dummy); + Assert.assertEquals(dummy, result.get(0)); + } + + @Test + public void testRootExpressionWithName() { + DummyNode dummy = new DummyNode(0, false, "DummyNodeA"); // todo in pmd 7 this should be a RootNode + + List result = assertQuery(1, "(/)[self::document-node(element(DummyNodeA))]", dummy); + Assert.assertEquals(dummy, result.get(0)); + + assertQuery(0, "(/)[self::document-node(element(DummyNodeX))]", dummy); + } + @Test public void ruleChainVisits() { SaxonXPathRuleQuery query = createQuery("//dummyNode[@Image='baz']/foo | //bar[@Public = 'true'] | //dummyNode[@Public = false()] | //dummyNode"); @@ -175,10 +220,11 @@ public class SaxonXPathRuleQueryTest { assertExpression("((DocumentSorter(((((/)/descendant::element(dummyNode, xs:anyType))[QuantifiedExpression(Atomizer(attribute::attribute(Image, xs:anyAtomicType)), ($qq:qq692331943 singleton eq \"baz\"))])/child::element(foo, xs:anyType))) | (((/)/descendant::element(bar, xs:anyType))[QuantifiedExpression(Atomizer(attribute::attribute(Public, xs:anyAtomicType)), ($qq:qq2127036371 singleton eq \"true\"))])) | (((/)/descendant::element(dummyNode, xs:anyType))[QuantifiedExpression(Atomizer(attribute::attribute(Public, xs:anyAtomicType)), ($qq:qq1529060733 singleton eq \"false\"))]))", query.nodeNameToXPaths.get(SaxonXPathRuleQuery.AST_ROOT).get(0)); } - private static void assertQuery(int resultSize, String xpath, Node node) { + private static List assertQuery(int resultSize, String xpath, Node node) { SaxonXPathRuleQuery query = createQuery(xpath); List result = query.evaluate(node, new RuleContext()); Assert.assertEquals(resultSize, result.size()); + return result; } private static SaxonXPathRuleQuery createQuery(String xpath, PropertyDescriptor ...descriptors) {