Issue checkstyle#11604: separates detail ast from xpath

rnveach · Aug 28, 2022 · a0c22d9 · a0c22d9
1 parent d3d8a71
commit a0c22d9
Show file tree

Hide file tree

Showing 16 changed files with 654 additions and 525 deletions.
diff --git a/.ci/pitest-suppressions/pitest-xpath-suppressions.xml b/.ci/pitest-suppressions/pitest-xpath-suppressions.xml
@@ -1,32 +1,59 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <suppressedMutations>
   <mutation unstable="false">
-    <sourceFile>DescendantIterator.java</sourceFile>
-    <mutatedClass>com.puppycrawl.tools.checkstyle.xpath.iterators.DescendantIterator</mutatedClass>
-    <mutatedMethod>&lt;init&gt;</mutatedMethod>
-    <mutator>org.pitest.mutationtest.engine.gregor.mutators.experimental.MemberVariableMutator</mutator>
-    <description>Removed assignment to member variable descendantEnum</description>
-    <lineContent>descendantEnum = null;</lineContent>
+    <sourceFile>AbstractElementNode.java</sourceFile>
+    <mutatedClass>com.puppycrawl.tools.checkstyle.xpath.RootNode</mutatedClass>
+    <mutatedMethod>iterateAxis</mutatedMethod>
+    <mutator>org.pitest.mutationtest.engine.gregor.mutators.experimental.RemoveSwitchMutator_1</mutator>
+    <description>RemoveSwitch 1 mutation</description>
+    <lineContent>switch (axisNumber) {</lineContent>
   </mutation>
 
   <mutation unstable="false">
-    <sourceFile>ElementNode.java</sourceFile>
-    <mutatedClass>com.puppycrawl.tools.checkstyle.xpath.ElementNode</mutatedClass>
-    <mutatedMethod>getAttributeNode</mutatedMethod>
-    <mutator>org.pitest.mutationtest.engine.gregor.mutators.experimental.MemberVariableMutator</mutator>
-    <description>Removed assignment to member variable attributeNode</description>
-    <lineContent>attributeNode = null;</lineContent>
+    <sourceFile>AbstractElementNode.java</sourceFile>
+    <mutatedClass>com.puppycrawl.tools.checkstyle.xpath.RootNode</mutatedClass>
+    <mutatedMethod>iterateAxis</mutatedMethod>
+    <mutator>org.pitest.mutationtest.engine.gregor.mutators.experimental.RemoveSwitchMutator_2</mutator>
+    <description>RemoveSwitch 2 mutation</description>
+    <lineContent>switch (axisNumber) {</lineContent>
   </mutation>
 
   <mutation unstable="false">
-    <sourceFile>ElementNode.java</sourceFile>
-    <mutatedClass>com.puppycrawl.tools.checkstyle.xpath.ElementNode</mutatedClass>
+    <sourceFile>AbstractElementNode.java</sourceFile>
+    <mutatedClass>com.puppycrawl.tools.checkstyle.xpath.RootNode</mutatedClass>
+    <mutatedMethod>iterateAxis</mutatedMethod>
+    <mutator>org.pitest.mutationtest.engine.gregor.mutators.experimental.RemoveSwitchMutator_5</mutator>
+    <description>RemoveSwitch 5 mutation</description>
+    <lineContent>switch (axisNumber) {</lineContent>
+  </mutation>
+
+  <mutation unstable="false">
+    <sourceFile>AbstractElementNode.java</sourceFile>
+    <mutatedClass>com.puppycrawl.tools.checkstyle.xpath.AbstractElementNode</mutatedClass>
     <mutatedMethod>iterateAxis</mutatedMethod>
     <mutator>org.pitest.mutationtest.engine.gregor.mutators.experimental.RemoveSwitchMutator_8</mutator>
     <description>RemoveSwitch 8 mutation</description>
     <lineContent>switch (axisNumber) {</lineContent>
   </mutation>
 
+  <mutation unstable="false">
+    <sourceFile>AbstractElementNode.java</sourceFile>
+    <mutatedClass>com.puppycrawl.tools.checkstyle.xpath.AbstractElementNode</mutatedClass>
+    <mutatedMethod>getPrecedingSiblings</mutatedMethod>
+    <mutator>org.pitest.mutationtest.engine.gregor.mutators.experimental.ArgumentPropagationMutator</mutator>
+    <description>replaced call to java/util/Collections::unmodifiableList with argument</description>
+    <lineContent>return Collections.unmodifiableList(siblings.subList(0, indexAmongSiblings));</lineContent>
+  </mutation>
+
+  <mutation unstable="false">
+    <sourceFile>DescendantIterator.java</sourceFile>
+    <mutatedClass>com.puppycrawl.tools.checkstyle.xpath.iterators.DescendantIterator</mutatedClass>
+    <mutatedMethod>&lt;init&gt;</mutatedMethod>
+    <mutator>org.pitest.mutationtest.engine.gregor.mutators.experimental.MemberVariableMutator</mutator>
+    <description>Removed assignment to member variable descendantEnum</description>
+    <lineContent>descendantEnum = null;</lineContent>
+  </mutation>
+
   <mutation unstable="false">
     <sourceFile>FollowingIterator.java</sourceFile>
     <mutatedClass>com.puppycrawl.tools.checkstyle.xpath.iterators.FollowingIterator</mutatedClass>
@@ -90,42 +117,6 @@
     <lineContent>return detailAst.getColumnNo();</lineContent>
   </mutation>
 
-  <mutation unstable="false">
-    <sourceFile>RootNode.java</sourceFile>
-    <mutatedClass>com.puppycrawl.tools.checkstyle.xpath.RootNode</mutatedClass>
-    <mutatedMethod>iterateAxis</mutatedMethod>
-    <mutator>org.pitest.mutationtest.engine.gregor.mutators.experimental.RemoveSwitchMutator_1</mutator>
-    <description>RemoveSwitch 1 mutation</description>
-    <lineContent>switch (axisNumber) {</lineContent>
-  </mutation>
-
-  <mutation unstable="false">
-    <sourceFile>RootNode.java</sourceFile>
-    <mutatedClass>com.puppycrawl.tools.checkstyle.xpath.RootNode</mutatedClass>
-    <mutatedMethod>iterateAxis</mutatedMethod>
-    <mutator>org.pitest.mutationtest.engine.gregor.mutators.experimental.RemoveSwitchMutator_2</mutator>
-    <description>RemoveSwitch 2 mutation</description>
-    <lineContent>switch (axisNumber) {</lineContent>
-  </mutation>
-
-  <mutation unstable="false">
-    <sourceFile>RootNode.java</sourceFile>
-    <mutatedClass>com.puppycrawl.tools.checkstyle.xpath.RootNode</mutatedClass>
-    <mutatedMethod>iterateAxis</mutatedMethod>
-    <mutator>org.pitest.mutationtest.engine.gregor.mutators.experimental.RemoveSwitchMutator_5</mutator>
-    <description>RemoveSwitch 5 mutation</description>
-    <lineContent>switch (axisNumber) {</lineContent>
-  </mutation>
-
-  <mutation unstable="false">
-    <sourceFile>RootNode.java</sourceFile>
-    <mutatedClass>com.puppycrawl.tools.checkstyle.xpath.RootNode</mutatedClass>
-    <mutatedMethod>iterateAxis</mutatedMethod>
-    <mutator>org.pitest.mutationtest.engine.gregor.mutators.experimental.RemoveSwitchMutator_8</mutator>
-    <description>RemoveSwitch 8 mutation</description>
-    <lineContent>switch (axisNumber) {</lineContent>
-  </mutation>
-
   <mutation unstable="false">
     <sourceFile>XpathQueryGenerator.java</sourceFile>
     <mutatedClass>com.puppycrawl.tools.checkstyle.xpath.XpathQueryGenerator</mutatedClass>

diff --git a/config/import-control.xml b/config/import-control.xml
@@ -234,6 +234,13 @@
     <allow pkg="com.puppycrawl.tools.checkstyle.xpath.iterators" local-only="true"/>
     <allow class="com.puppycrawl.tools.checkstyle.TreeWalkerAuditEvent" local-only="true"/>
 
+    <file name="Abstract.*Node" regex="true">
+      <allow class="com.puppycrawl.tools.checkstyle.api.AbstractSyntaxTree"/>
+      <allow pkg="com.puppycrawl.tools.checkstyle.xpath.iterators"/>
+      <!-- These are more saxon utilities, and should not have anything checkstyle specific -->
+      <disallow pkg="com\.puppycrawl.*" regex="true"/>
+    </file>
+
     <subpackage name="iterators">
       <!-- These are more saxon utilities, and should not have anything checkstyle specific -->
       <disallow pkg="com\.puppycrawl.*" regex="true"/>

diff --git a/config/pmd.xml b/config/pmd.xml
@@ -129,12 +129,17 @@
   </rule>
   <rule ref="category/java/codestyle.xml/EmptyMethodInAbstractClassShouldBeAbstract">
     <properties>
-      <!-- Can not change the API. -->
+      <!--
+       Can not change the API.
+       AbstractRootNode is what a root node is.
+      -->
       <property name="violationSuppressXPath"
                 value="//ClassOrInterfaceDeclaration[@SimpleName='AbstractFileSetCheck'
         or @SimpleName='AbstractCheck' or @SimpleName='AbstractJavadocCheck'
         or @SimpleName='AbstractNode'
-        or @SimpleName='AbstractViolationReporter']"/>
+        or @SimpleName='AbstractViolationReporter']
+        | //ClassOrInterfaceDeclaration[@SimpleName='AbstractRootNode']
+          //MethodDeclaration[@Name='getParent' or @Name='getDepth']"/>
     </properties>
   </rule>
   <rule ref="category/java/codestyle.xml/UseUnderscoresInNumericLiterals">
@@ -280,7 +285,8 @@
            The methods visitToken/leaveToken are a big SWITCH block with a number of IF blocks.
              If we split the block to several methods it will damage the readability.
            XMLLogger.encode, SarifLogger.escape, FallThroughCheck.isTerminated,
-             ElementNode.iterateAxis, NoWhitespaceAfterCheck.getArrayDeclaratorPreviousElement
+             AbstractElementNode.iterateAxis,
+             NoWhitespaceAfterCheck.getArrayDeclaratorPreviousElement
              are also huge switches, they had to be monolithic.
            SuppressFilterElement is a single constructor and can't be split easily -->
       <property name="violationSuppressXPath"
@@ -300,7 +306,8 @@
         or @SimpleName='NPathComplexityCheck']//MethodDeclaration[@Name='leaveToken']
       | //ClassOrInterfaceDeclaration[@SimpleName='NoWhitespaceAfterCheck']
              //MethodDeclaration[@Name='getArrayDeclaratorPreviousElement']
-      | //ClassOrInterfaceDeclaration[@SimpleName='ElementNode' or @SimpleName='RootNode']
+      | //ClassOrInterfaceDeclaration[@SimpleName='AbstractElementNode'
+        or @SimpleName='AbstractRootNode']
              //MethodDeclaration[@Name='iterateAxis']
       | //ClassOrInterfaceDeclaration[@SimpleName='SuppressFilterElement']
              //ConstructorDeclaration

diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/api/AbstractSyntaxTree.java b/src/main/java/com/puppycrawl/tools/checkstyle/api/AbstractSyntaxTree.java
@@ -0,0 +1,30 @@
+///////////////////////////////////////////////////////////////////////////////////////////////
+// checkstyle: Checks Java source code and other text files for adherence to a set of rules.
+// Copyright (C) 2001-2022 the original author or authors.
+//
+// This library is free software; you can redistribute it and/or
+// modify it under the terms of the GNU Lesser General Public
+// License as published by the Free Software Foundation; either
+// version 2.1 of the License, or (at your option) any later version.
+//
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+// Lesser General Public License for more details.
+//
+// You should have received a copy of the GNU Lesser General Public
+// License along with this library; if not, write to the Free Software
+// Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+///////////////////////////////////////////////////////////////////////////////////////////////
+
+package com.puppycrawl.tools.checkstyle.api;
+
+/**
+ * Base interface used for all ASTs of Checkstyle.
+ *
+ * @noinspection MarkerInterface
+ * @noinspectionreason MarkerInterface - This interface is currently just a starting point.
+ */
+public interface AbstractSyntaxTree {
+    // no methods yet
+}
diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/api/DetailAST.java b/src/main/java/com/puppycrawl/tools/checkstyle/api/DetailAST.java
@@ -26,7 +26,7 @@
  *
  * @see <a href="https://www.antlr.org/">ANTLR Website</a>
  */
-public interface DetailAST {
+public interface DetailAST extends AbstractSyntaxTree {
 
     /**
      * Returns the number of child nodes one level below this node. That is,

diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/api/DetailNode.java b/src/main/java/com/puppycrawl/tools/checkstyle/api/DetailNode.java
@@ -26,7 +26,7 @@
  * @see com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocNodeImpl
  * @see com.puppycrawl.tools.checkstyle.checks.javadoc.AbstractJavadocCheck
  */
-public interface DetailNode {
+public interface DetailNode extends AbstractSyntaxTree {
 
     /**
      * Node type.

diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/MatchXpathCheck.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/MatchXpathCheck.java
@@ -290,7 +290,7 @@ private List<DetailAST> findMatchingNodesByXpathQuery(DetailAST rootAST) {
                     xpathExpression.createDynamicContext(rootNode);
             final List<Item> matchingItems = xpathExpression.evaluate(xpathDynamicContext);
             return matchingItems.stream()
-                    .map(item -> ((AbstractNode) item).getUnderlyingNode())
+                    .map(item -> (DetailAST) ((AbstractNode) item).getUnderlyingNode())
                     .collect(Collectors.toList());
         }
         catch (XPathException ex) {

diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/gui/TreeTable.java b/src/main/java/com/puppycrawl/tools/checkstyle/gui/TreeTable.java
@@ -215,6 +215,7 @@ public void selectNodeByXpath() {
                               .stream()
                               .map(AbstractNode.class::cast)
                               .map(AbstractNode::getUnderlyingNode)
+                              .map(DetailAST.class::cast)
                               .collect(Collectors.toCollection(ArrayDeque::new));
                 updateTreeTable(xpath, nodes);
             }

diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/utils/XpathUtil.java b/src/main/java/com/puppycrawl/tools/checkstyle/utils/XpathUtil.java
@@ -188,7 +188,7 @@ public static String printXpathBranch(String xpath, File file) throws Checkstyle
                 JavaParser.Options.WITH_COMMENTS));
             final List<NodeInfo> matchingItems = getXpathItems(xpath, rootNode);
             return matchingItems.stream()
-                .map(item -> ((AbstractNode) item).getUnderlyingNode())
+                .map(item -> (DetailAST) ((AbstractNode) item).getUnderlyingNode())
                 .map(AstTreeStringPrinter::printBranch)
                 .collect(Collectors.joining(DELIMITER));
         }