Syntax tree: ignore implicit expressions at the top level of statements

[gribozavr]

Summary

I changed markStmtChild to ignore implicit expressions the same way as
markExprChild does it already. The test that I modified crashes
without this change.

[gribozavr]

Refactored makeStmtChild to have fewer redundant checks.

[hlopko]

LGTM :)

[eduucaldas]

clang/lib/Tooling/Syntax/BuildTree.cpp

1048

I thought this treatement of derived class should be done by RecursiveASTVisitor
This is kinda the task of WalkUpFrom*, unfortunately WalkUpFromThis follows the order:
WalkUpFromBase;
VisitThis;

And we would've wanted the opposite order:
VisitThis
WalkUpFromBase

[gribozavr2]

clang/lib/Tooling/Syntax/BuildTree.cpp

1048

Unfortunately I don't see how to implement this behavior (cheaply) in the visitation methods of RecursiveASTVisitor. The crux of the issue is that we are inserting a syntax tree node (ExpressionStatement) that does not correspond to any semantic AST node. We want to do it for every Expr that appears in a Stmt position -- so we need to know the parent node, or in this case, more precisely, that this Expr node is in a Stmt position.

clang/lib/Tooling/Syntax/BuildTree.cpp

void syntax::TreeBuilder::markChild(ASTPtr N, NodeRole R) {
   assert(SN != nullptr);
   setRole(SN, R);
 }
 
 void syntax::TreeBuilder::markStmtChild(Stmt *Child, NodeRole Role) {
   if (!Child)
     return;
 
-  syntax::Tree *ChildNode = Mapping.find(Child);
-  assert(ChildNode != nullptr);
-
+  syntax::Tree *ChildNode;
+  if (Expr *ChildExpr = dyn_cast<Expr>(Child)) {
     // This is an expression in a statement position, consume the trailing
     // semicolon and form an 'ExpressionStatement' node.
-  if (isa<Expr>(Child)) {
-    setRole(ChildNode, NodeRole::ExpressionStatement_expression);
+    markExprChild(ChildExpr, NodeRole::ExpressionStatement_expression);
     ChildNode = new (allocator()) syntax::ExpressionStatement;
     // (!) 'getStmtRange()' ensures this covers a trailing semicolon.
     Pending.foldChildren(Arena, getStmtRange(Child), ChildNode);
+  } else {
+    ChildNode = Mapping.find(Child);
   }
+  assert(ChildNode != nullptr);
   setRole(ChildNode, Role);
 }
 
 void syntax::TreeBuilder::markExprChild(Expr *Child, NodeRole Role) {
   if (!Child)
     return;
   Child = Child->IgnoreImplicit();
 

clang/unittests/Tooling/Syntax/TreeTest.cpp

`-CompoundStatement
     | | | `-a
     | | `---
     | `-;
     `-}
 )txt");
 }
 
 TEST_P(SyntaxTreeTest, PrefixUnaryOperator) {
-  if (!GetParam().isCXX()) {
-    // TODO: Split parts that depend on C++ into a separate test.
-    return;
-  }
   expectTreeDumpEqual(
       R"cpp(
-void test(int a, int *ap, bool b) {
+void test(int a, int *ap) {
   --a; ++a;
-  ~a; compl a;
+  ~a;
   -a;
   +a;
   &a;
   *ap;
-  !b; not b;
+  !a;
   __real a; __imag a;
 }
     )cpp",
       R"txt(
 *: TranslationUnit
 `-SimpleDeclaration
   |-void
   |-SimpleDeclarator
   | |-test
   | `-ParametersAndQualifiers
   |   |-(
   |   |-SimpleDeclaration
   |   | |-int
   |   | `-SimpleDeclarator
   |   |   `-a
   |   |-,
   |   |-SimpleDeclaration
   |   | |-int
   |   | `-SimpleDeclarator
   |   |   |-*
   |   |   `-ap
-  |   |-,
-  |   |-SimpleDeclaration
-  |   | |-bool
-  |   | `-SimpleDeclarator
-  |   |   `-b
   |   `-)
   `-CompoundStatement
     |-{
     |-ExpressionStatement
     | |-PrefixUnaryOperatorExpression
     | | |---
     | | `-UnknownExpression
     | |   `-a
     | `-;
     |-ExpressionStatement
     | |-PrefixUnaryOperatorExpression
     | | |-++
     | | `-UnknownExpression
     | |   `-a
     | `-;
     |-ExpressionStatement
     | |-PrefixUnaryOperatorExpression
     | | |-~
     | | `-UnknownExpression
     | |   `-a
     | `-;
     |-ExpressionStatement
     | |-PrefixUnaryOperatorExpression
-    | | |-compl
-    | | `-UnknownExpression
-    | |   `-a
-    | `-;
-    |-ExpressionStatement
-    | |-PrefixUnaryOperatorExpression
     | | |--
     | | `-UnknownExpression
     | |   `-a
     | `-;
     |-ExpressionStatement
     | |-PrefixUnaryOperatorExpression
     | | |-+
     | | `-UnknownExpression
`-CompoundStatement
     | | |-*
     | | `-UnknownExpression
     | |   `-ap
     | `-;
     |-ExpressionStatement
     | |-PrefixUnaryOperatorExpression
     | | |-!
     | | `-UnknownExpression
-    | |   `-b
+    | |   `-a
     | `-;
     |-ExpressionStatement
     | |-PrefixUnaryOperatorExpression
-    | | |-not
+    | | |-__real
     | | `-UnknownExpression
-    | |   `-b
+    | |   `-a
     | `-;
     |-ExpressionStatement
     | |-PrefixUnaryOperatorExpression
-    | | |-__real
+    | | |-__imag
     | | `-UnknownExpression
     | |   `-a
     | `-;
+    `-}
+)txt");
+}
+
+TEST_P(SyntaxTreeTest, PrefixUnaryOperatorCxx) {
+  if (!GetParam().isCXX()) {
+    return;
+  }
+  expectTreeDumpEqual(
+      R"cpp(
+void test(int a, bool b) {
+  compl a;
+  not b;
+}
+    )cpp",
+      R"txt(
+*: TranslationUnit
+`-SimpleDeclaration
+  |-void
+  |-SimpleDeclarator
+  | |-test
+  | `-ParametersAndQualifiers
+  |   |-(
+  |   |-SimpleDeclaration
+  |   | |-int
+  |   | `-SimpleDeclarator
+  |   |   `-a
+  |   |-,
+  |   |-SimpleDeclaration
+  |   | |-bool
+  |   | `-SimpleDeclarator
+  |   |   `-b
+  |   `-)
+  `-CompoundStatement
+    |-{
     |-ExpressionStatement
     | |-PrefixUnaryOperatorExpression
-    | | |-__imag
+    | | |-compl
     | | `-UnknownExpression
     | |   `-a
     | `-;
+    |-ExpressionStatement
+    | |-PrefixUnaryOperatorExpression
+    | | |-not
+    | | `-UnknownExpression
+    | |   `-b
+    | `-;
     `-}
 )txt");
 }
 
 TEST_P(SyntaxTreeTest, BinaryOperator) {
   if (!GetParam().isCXX()) {
     // TODO: Split parts that depend on C++ into a separate test.
     return;