[apex] Handle blockstatements without curly braces

The new parser seems to always return a BlockStatement, regardless whether it's a real block or just an expression statement.
SumitXLR15 · Aug 12, 2017 · 9a71a6f · 9a71a6f
1 parent 174184d
commit 9a71a6f
Show file tree

Hide file tree

Showing 6 changed files with 55 additions and 30 deletions.
diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/ASTBlockStatement.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/ASTBlockStatement.java
@@ -7,6 +7,7 @@
 import apex.jorje.semantic.ast.statement.BlockStatement;
 
 public class ASTBlockStatement extends AbstractApexNode<BlockStatement> {
+    private boolean curlyBrace;
 
     public ASTBlockStatement(BlockStatement blockStatement) {
         super(blockStatement);
@@ -15,4 +16,21 @@ public ASTBlockStatement(BlockStatement blockStatement) {
     public Object jjtAccept(ApexParserVisitor visitor, Object data) {
         return visitor.visit(this, data);
     }
+
+    public boolean hasCurlyBrace() {
+        return curlyBrace;
+    }
+
+    @Override
+    protected void handleSourceCode(final String source) {
+        if (!hasRealLoc()) {
+            return;
+        }
+
+        // check, whether the this block statement really begins with a curly brace
+        // unfortunately, for-loop and if-statements always contain a block statement,
+        // regardless whether curly braces where present or not.
+        char firstChar = source.charAt(node.getLoc().startIndex);
+        curlyBrace = firstChar == '{';
+    }
 }
diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/AbstractApexNode.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/AbstractApexNode.java
@@ -44,6 +44,10 @@ void calculateLineNumbers(SourceCodePositioner positioner) {
         }
     }
 
+    protected void handleSourceCode(String source) {
+        // default implementation does nothing
+    }
+
     @Override
     public int getBeginLine() {
         if (this.beginLine > 0) {

diff --git a/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/ApexTreeBuilder.java b/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/ApexTreeBuilder.java
@@ -108,7 +108,7 @@
 
 public final class ApexTreeBuilder extends AstVisitor<AdditionalPassScope> {
 
-    private static final Map<Class<? extends AstNode>, Constructor<? extends ApexNode<?>>> NODE_TYPE_TO_NODE_ADAPTER_TYPE = new HashMap<>();
+    private static final Map<Class<? extends AstNode>, Constructor<? extends AbstractApexNode<?>>> NODE_TYPE_TO_NODE_ADAPTER_TYPE = new HashMap<>();
 
     static {
         register(Annotation.class, ASTAnnotation.class);
@@ -201,7 +201,7 @@ public final class ApexTreeBuilder extends AstVisitor<AdditionalPassScope> {
         register(WhileLoopStatement.class, ASTWhileLoopStatement.class);
     }
 
-    private static <T extends AstNode> void register(Class<T> nodeType, Class<? extends ApexNode<T>> nodeAdapterType) {
+    private static <T extends AstNode> void register(Class<T> nodeType, Class<? extends AbstractApexNode<T>> nodeAdapterType) {
         try {
             NODE_TYPE_TO_NODE_ADAPTER_TYPE.put(nodeType, nodeAdapterType.getConstructor(nodeType));
         } catch (SecurityException e) {
@@ -217,20 +217,22 @@ private static <T extends AstNode> void register(Class<T> nodeType, Class<? exte
     // The Apex nodes with children to build.
     private Stack<AstNode> parents = new Stack<>();
 
-    private SourceCodePositioner sourceCodePositioner;
+    private final SourceCodePositioner sourceCodePositioner;
+    private final String sourceCode;
 
     public ApexTreeBuilder(String sourceCode) {
+        this.sourceCode = sourceCode;
         sourceCodePositioner = new SourceCodePositioner(sourceCode);
     }
 
     AdditionalPassScope scope = new AdditionalPassScope(new Errors());
 
-    static <T extends AstNode> ApexNode<T> createNodeAdapter(T node) {
+    static <T extends AstNode> AbstractApexNode<T> createNodeAdapter(T node) {
         try {
             @SuppressWarnings("unchecked")
             // the register function makes sure only ApexNode<T> can be added,
             // where T is "T extends AstNode".
-            Constructor<? extends ApexNode<T>> constructor = (Constructor<? extends ApexNode<T>>) NODE_TYPE_TO_NODE_ADAPTER_TYPE
+            Constructor<? extends AbstractApexNode<T>> constructor = (Constructor<? extends AbstractApexNode<T>>) NODE_TYPE_TO_NODE_ADAPTER_TYPE
                     .get(node.getClass());
             if (constructor == null) {
                 throw new IllegalArgumentException(
@@ -248,8 +250,9 @@ static <T extends AstNode> ApexNode<T> createNodeAdapter(T node) {
 
     public <T extends AstNode> ApexNode<T> build(T astNode) {
         // Create a Node
-        ApexNode<T> node = createNodeAdapter(astNode);
-        calculateLineNumbers(node);
+        AbstractApexNode<T> node = createNodeAdapter(astNode);
+        node.calculateLineNumbers(sourceCodePositioner);
+        node.handleSourceCode(sourceCode);
 
         // Append to parent
         Node parent = nodes.isEmpty() ? null : nodes.peek();
@@ -268,11 +271,6 @@ public <T extends AstNode> ApexNode<T> build(T astNode) {
         return node;
     }
 
-    private void calculateLineNumbers(ApexNode<?> node) {
-        AbstractApexNode<?> apexNode = (AbstractApexNode<?>) node;
-        apexNode.calculateLineNumbers(sourceCodePositioner);
-    }
-
     private boolean visit(AstNode node) {
         if (parents.peek() == node) {
             return true;

diff --git a/pmd-apex/src/main/resources/rulesets/apex/braces.xml b/pmd-apex/src/main/resources/rulesets/apex/braces.xml
@@ -24,7 +24,7 @@ controlled from the rest.
            <property name="xpath">
                <value>
                    <![CDATA[
-//IfBlockStatement/ExpressionStatement
+//IfBlockStatement/BlockStatement[@CurlyBrace='false']
                    ]]>
                </value>
            </property>
@@ -59,7 +59,7 @@ controlled from the rest.
             <property name="xpath">
                 <value>
 <![CDATA[
-//WhileLoopStatement/ExpressionStatement
+//WhileLoopStatement/BlockStatement[@CurlyBrace='false']
 ]]>
                 </value>
             </property>
@@ -92,11 +92,9 @@ from the rest.
              <property name="xpath">
                  <value>
 <![CDATA[
-//ExpressionStatement[parent::IfBlockStatement]
+//IfBlockStatement/BlockStatement[@CurlyBrace='false'][count(child::*) > 0]
 |
-//ExpressionStatement[parent::IfElseBlockStatement]
-|
-//IfElseBlockStatement[parent::IfBlockStatement]
+//IfElseBlockStatement/BlockStatement[@CurlyBrace='false'][count(child::*) > 0]
  ]]>
                  </value>
              </property>
@@ -131,11 +129,9 @@ from the rest.
              <property name="xpath">
                  <value>
  <![CDATA[
-//ForLoopStatement
-[child::ExpressionStatement]
+//ForLoopStatement/BlockStatement[@CurlyBrace='false']
 |
-//ForEachStatement
-[child::ExpressionStatement]
+//ForEachStatement/BlockStatement[@CurlyBrace='false']
  ]]>
                  </value>
              </property>

diff --git a/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/ast/ApexLexerTest.java b/pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/ast/ApexLexerTest.java
@@ -32,7 +32,6 @@ public void testLexer() throws Exception {
         int tokenCount = 0;
         while (token.getType() != Token.EOF) {
             tokenCount++;
-            System.out.println(token);
             token = lexer.nextToken();
         }
         Assert.assertEquals(43, tokenCount);
@@ -45,6 +44,5 @@ public void testParser() throws Exception {
         ApexParser parser = new ApexParser(new CommonTokenStream(lexer));
         CompilationUnit compilationUnit = parser.compilationUnit();
         Assert.assertNotNull(compilationUnit);
-        System.out.println(compilationUnit);
     }
 }
diff --git a/...rc/test/resources/net/sourceforge/pmd/lang/apex/rule/braces/xml/ForLoopsMustUseBraces.xml b/...rc/test/resources/net/sourceforge/pmd/lang/apex/rule/braces/xml/ForLoopsMustUseBraces.xml
@@ -31,7 +31,7 @@ public class Foo {
     </test-code>
     <test-code>
         <description><![CDATA[
-
+for loop without increment
      ]]></description>
         <expected-problems>1</expected-problems>
         <code><![CDATA[
@@ -45,7 +45,7 @@ public class Foo {
     </test-code>
     <test-code>
         <description><![CDATA[
-
+for loop without condition and increment
      ]]></description>
         <expected-problems>1</expected-problems>
         <code><![CDATA[
@@ -59,7 +59,7 @@ public class Foo {
     </test-code>
     <test-code>
         <description><![CDATA[
-
+for loop without initializer, condition and increment
      ]]></description>
         <expected-problems>1</expected-problems>
         <code><![CDATA[
@@ -72,16 +72,27 @@ public class Foo {
      ]]></code>
     </test-code>
     <test-code>
-        <description><![CDATA[
-for-each
-     ]]></description>
+        <description>for-each, not ok</description>
         <expected-problems>1</expected-problems>
         <code><![CDATA[
 public class Foo {
  void foo() {   
   for (Account a : accounts) 
        foo();
  }
+}
+     ]]></code>
+    </test-code>
+    <test-code>
+        <description>for-each, ok</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Foo {
+ void foo() {   
+  for (Account a : accounts) {
+       foo();
+  }
+ }
 }
      ]]></code>
     </test-code>