From a0e1e40bcb75762d58bb8da7c7e07d16e9190d6e Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Fri, 27 Mar 2020 12:58:38 +0100 Subject: [PATCH] [core] saxon rulechain: don't use rule chain for other path expressions --- .../lang/rule/xpath/SaxonXPathRuleQuery.java | 2 +- .../xpath/internal/ExpressionPrinter.java | 6 ++++ .../xpath/internal/RuleChainAnalyzer.java | 10 ++++++ .../pmd/lang/rule/xpath/internal/Visitor.java | 8 +++++ .../pmd/lang/DummyLanguageModule.java | 11 +++++- .../rule/xpath/SaxonXPathRuleQueryTest.java | 35 ++++++++++++++++--- 6 files changed, 65 insertions(+), 7 deletions(-) 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 73cf58eb02..3f4178a754 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 @@ -77,7 +77,7 @@ public class SaxonXPathRuleQuery extends AbstractXPathRuleQuery { /** * Representation of an XPath query, created at {@link #initializeXPathExpression()} using {@link #xpath}. */ - private XPathExpression xpathExpression; + XPathExpression xpathExpression; /** * Holds the static context later used to match the variables in the dynamic context in diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/ExpressionPrinter.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/ExpressionPrinter.java index d1cd1d3e45..144b11ddd8 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/ExpressionPrinter.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/ExpressionPrinter.java @@ -13,6 +13,12 @@ import net.sf.saxon.om.Axis; /** * Simple printer for saxon expressions. Might be useful for debugging / during development. + * + *

Example: + * 

+ * ExpressionPrinter printer = new ExpressionPrinter();
+ * printer.visit(query.xpathExpression.getInternalExpression());
+ *
*/ public class ExpressionPrinter extends Visitor { private int depth = 0; diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/RuleChainAnalyzer.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/RuleChainAnalyzer.java index d621878023..69bb522a01 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/RuleChainAnalyzer.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/RuleChainAnalyzer.java @@ -13,6 +13,7 @@ import net.sf.saxon.Configuration; import net.sf.saxon.expr.AxisExpression; import net.sf.saxon.expr.Expression; import net.sf.saxon.expr.FilterExpression; +import net.sf.saxon.expr.LazyExpression; import net.sf.saxon.expr.PathExpression; import net.sf.saxon.expr.RootExpression; import net.sf.saxon.om.Axis; @@ -96,6 +97,15 @@ public class RuleChainAnalyzer extends Visitor { return super.visit(e); } + @Override + public Expression visit(LazyExpression e) { + if (e.getBaseExpression() instanceof PathExpression) { + this.rootElement = null; + return e; + } + return super.visit(e); + } + public static Comparator documentOrderComparator() { return net.sourceforge.pmd.lang.rule.xpath.internal.DocumentSorter.INSTANCE; } diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/Visitor.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/Visitor.java index 088724df7e..47c100880d 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/Visitor.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/internal/Visitor.java @@ -7,6 +7,7 @@ package net.sourceforge.pmd.lang.rule.xpath.internal; import net.sf.saxon.expr.AxisExpression; import net.sf.saxon.expr.Expression; import net.sf.saxon.expr.FilterExpression; +import net.sf.saxon.expr.LazyExpression; import net.sf.saxon.expr.LetExpression; import net.sf.saxon.expr.PathExpression; import net.sf.saxon.expr.QuantifiedExpression; @@ -63,6 +64,11 @@ abstract class Visitor { return result; } + public Expression visit(LazyExpression e) { + Expression base = visit(e.getBaseExpression()); + return LazyExpression.makeLazyExpression(base); + } + public Expression visit(Expression expr) { Expression result; if (expr instanceof DocumentSorter) { @@ -81,6 +87,8 @@ abstract class Visitor { result = visit((QuantifiedExpression) expr); } else if (expr instanceof LetExpression) { result = visit((LetExpression) expr); + } else if (expr instanceof LazyExpression) { + result = visit((LazyExpression) expr); } else { result = expr; } diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/lang/DummyLanguageModule.java b/pmd-core/src/test/java/net/sourceforge/pmd/lang/DummyLanguageModule.java index 7176915c8f..f956cbf85e 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/lang/DummyLanguageModule.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/lang/DummyLanguageModule.java @@ -19,11 +19,13 @@ import net.sourceforge.pmd.RuleViolation; import net.sourceforge.pmd.lang.ast.DummyNode; import net.sourceforge.pmd.lang.ast.Node; import net.sourceforge.pmd.lang.ast.ParseException; +import net.sourceforge.pmd.lang.ast.xpath.AbstractASTXPathHandler; import net.sourceforge.pmd.lang.ast.xpath.DocumentNavigator; import net.sourceforge.pmd.lang.rule.AbstractRuleChainVisitor; import net.sourceforge.pmd.lang.rule.AbstractRuleViolationFactory; import net.sourceforge.pmd.lang.rule.ParametricRuleViolation; +import net.sf.saxon.expr.XPathContext; import net.sf.saxon.sxpath.IndependentContext; /** @@ -67,11 +69,18 @@ public class DummyLanguageModule extends BaseLanguageModule { } public static class Handler extends AbstractLanguageVersionHandler { + public static class TestFunctions { + public static boolean typeIs(final XPathContext context, final String fullTypeName) { + return false; + } + } + @Override public XPathHandler getXPathHandler() { - return new XPathHandler() { + return new AbstractASTXPathHandler() { @Override public void initialize(IndependentContext context) { + super.initialize(context, LanguageRegistry.getLanguage(DummyLanguageModule.NAME), TestFunctions.class); } @Override 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 c2c48be729..b27861afe7 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 @@ -18,7 +18,6 @@ import net.sourceforge.pmd.properties.PropertyDescriptor; import net.sourceforge.pmd.properties.PropertyFactory; import net.sf.saxon.expr.Expression; -import net.sf.saxon.expr.XPathContext; public class SaxonXPathRuleQueryTest { @@ -64,10 +63,36 @@ public class SaxonXPathRuleQueryTest { assertExpression("((((/)/descendant::element(dummyNode, xs:anyType))[QuantifiedExpression(Atomizer(attribute::attribute(Test2, xs:anyAtomicType)), ($qq:qq1741979653 singleton eq true()))])[QuantifiedExpression(Atomizer(attribute::attribute(Test1, xs:anyAtomicType)), ($qq:qq1529060733 singleton eq false()))])", query.nodeNameToXPaths.get(SaxonXPathRuleQuery.AST_ROOT).get(0)); } - public static class TestFunctions { - public static boolean typeIs(final XPathContext context, final String fullTypeName) { - return false; - } + @Test + public void ruleChainVisitsCustomFunctions() { + SaxonXPathRuleQuery query = createQuery("//dummyNode[pmd-dummy:typeIs(@Image)]"); + List ruleChainVisits = query.getRuleChainVisits(); + Assert.assertEquals(1, ruleChainVisits.size()); + Assert.assertTrue(ruleChainVisits.contains("dummyNode")); + Assert.assertEquals(2, query.nodeNameToXPaths.size()); + System.out.println(query.nodeNameToXPaths); + assertExpression("(self::node()[pmd-dummy:typeIs(CardinalityChecker(ItemChecker(UntypedAtomicConverter(Atomizer(attribute::attribute(Image, xs:anyAtomicType))))))])", query.nodeNameToXPaths.get("dummyNode").get(0)); + assertExpression("DocumentSorter((((/)/descendant-or-self::node())/(child::element(dummyNode, xs:anyType)[pmd-dummy:typeIs(CardinalityChecker(ItemChecker(UntypedAtomicConverter(Atomizer(attribute::attribute(Image, xs:anyAtomicType))))))])))", query.nodeNameToXPaths.get(SaxonXPathRuleQuery.AST_ROOT).get(0)); + } + + /** + * If a query contains another unbounded path expression other than the first one, it must be + * excluded from rule chain execution. Saxon itself optimizes this quite good already. + */ + @Test + public void ruleChainVisitsUnboundedPathExpressions() { + SaxonXPathRuleQuery query = createQuery("//dummyNode[//ClassOrInterfaceType]"); + List ruleChainVisits = query.getRuleChainVisits(); + Assert.assertEquals(0, ruleChainVisits.size()); + Assert.assertEquals(1, query.nodeNameToXPaths.size()); + assertExpression("LetExpression(LazyExpression(((/)/descendant::element(ClassOrInterfaceType, xs:anyType))), (((/)/descendant::element(dummyNode, xs:anyType))[$zz:zz771775563]))", query.nodeNameToXPaths.get(SaxonXPathRuleQuery.AST_ROOT).get(0)); + + // second sample, more complex + query = createQuery("//dummyNode[ancestor::ClassOrInterfaceDeclaration[//ClassOrInterfaceType]]"); + ruleChainVisits = query.getRuleChainVisits(); + Assert.assertEquals(0, ruleChainVisits.size()); + Assert.assertEquals(1, query.nodeNameToXPaths.size()); + assertExpression("LetExpression(LazyExpression(((/)/descendant::element(ClassOrInterfaceType, xs:anyType))), (((/)/descendant::element(dummyNode, xs:anyType))[(ancestor::element(ClassOrInterfaceDeclaration, xs:anyType)[$zz:zz106374177])]))", query.nodeNameToXPaths.get(SaxonXPathRuleQuery.AST_ROOT).get(0)); } @Test