[SyntaxTree] Ignore implicit non-leaf `CXXConstructExpr`

[eduucaldas]

Use IgnoreExpr.h infrastructure.

[eduucaldas]

nits

[eduucaldas]

clang/lib/Tooling/Syntax/BuildTree.cpp

48–58

Should this go into IgnoreExpr as well?

If yes, should we unify this with the lambda inside IgnoreUnlessSpelledInSource, thus removing the lambda and using this free function instead?

[gribozavr2]

clang/lib/Tooling/Syntax/BuildTree.cpp

48–58

That sounds like a good idea to me.

clang/unittests/Tooling/Syntax/BuildTreeTest.cpp

1749

Could you also add a test that invokes implicit conversions?

struct X {
  X(int);
};
void TakeX(const X&);

void test() {
  [[TakeX(1)]];
}

[eduucaldas]

Add further tests and extract IgnoreImplicitConstructorSingleStep

[eduucaldas]

Add more tests, not extract IgnoreImplicitConstructorSingleStep

[eduucaldas]

.

[eduucaldas]

Add comments

[eduucaldas]

clang/lib/Tooling/Syntax/BuildTree.cpp

48–50

Please give feedback on this comments and should I comment the rest of the function?

[gribozavr2]

clang/lib/Tooling/Syntax/BuildTree.cpp

48

48–50

Seems straightforward to me.

clang/unittests/Tooling/Syntax/BuildTreeTest.cpp

4045

This is not a conversion, this is an explicit constructor call (CXXTemporaryObjectExpr) -- so please rename the test. Same for other tests below.

[eduucaldas]

answer comments

clang/unittests/Tooling/Syntax/BuildTreeTest.cpp

4045

I am trying to follow the syntax to name things. And in the grammar this is under "Explicit type conversion (functional notation)", which englobes as well things of the type float(3).

[eduucaldas]

• Rename tests
• Add FIXMEs for init-declarators

clang/lib/Tooling/Syntax/BuildTree.cpp

 //===- BuildTree.cpp ------------------------------------------*- C++ -*-=====//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #include "clang/Tooling/Syntax/BuildTree.h"
 #include "clang/AST/ASTFwd.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclarationName.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
+#include "clang/AST/IgnoreExpr.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/AST/TypeLocVisitor.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/Specifiers.h"

 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cstddef>
 #include <map>
 
 using namespace clang;
 
+// Ignores the implicit `CXXConstructExpr` for copy/move constructor calls
+// generated by the compiler, as well as in implicit conversions like the one
+// wrapping `1` in `X x = 1;`.
+static Expr *IgnoreImplicitConstructorSingleStep(Expr *E) {
+  if (auto *C = dyn_cast<CXXConstructExpr>(E)) {
+    auto NumArgs = C->getNumArgs();
+    if (NumArgs == 1 || (NumArgs > 1 && isa<CXXDefaultArgExpr>(C->getArg(1)))) {
+      Expr *A = C->getArg(0);
+      if (C->getParenOrBraceRange().isInvalid())
+        return A;
+    }
+  }
+  return E;
+}
+
+static Expr *IgnoreImplicit(Expr *E) {
+  return IgnoreExprNodes(E, IgnoreImplicitSingleStep,
+                         IgnoreImplicitConstructorSingleStep);
+}
+
 LLVM_ATTRIBUTE_UNUSED
-static bool isImplicitExpr(Expr *E) { return E->IgnoreImplicit() != E; }
+static bool isImplicitExpr(Expr *E) { return IgnoreImplicit(E) != E; }
 
 namespace {
 /// Get start location of the Declarator from the TypeLoc.
 /// E.g.:
 ///   loc of `(` in `int (a)`
 ///   loc of `*` in `int *(a)`
 ///   loc of the first `(` in `int (*a)(int)`
 ///   loc of the `*` in `int *(a)(int)`
public:
   }
 
   bool TraverseStmt(Stmt *S) {
     if (auto *DS = dyn_cast_or_null<DeclStmt>(S)) {
       // We want to consume the semicolon, make sure SimpleDeclaration does not.
       for (auto *D : DS->decls())
         Builder.noticeDeclWithoutSemicolon(D);
     } else if (auto *E = dyn_cast_or_null<Expr>(S)) {
-      return RecursiveASTVisitor::TraverseStmt(E->IgnoreImplicit());
+      return RecursiveASTVisitor::TraverseStmt(IgnoreImplicit(E));
     }
     return RecursiveASTVisitor::TraverseStmt(S);
   }
 
   // Some expressions are not yet handled by syntax trees.
   bool WalkUpFromExpr(Expr *E) {
     assert(!isImplicitExpr(E) && "should be handled by TraverseStmt");
     Builder.foldNode(Builder.getExprRange(E),
void syntax::TreeBuilder::markStmtChild(Stmt *Child, NodeRole Role) {
   }
   assert(ChildNode != nullptr);
   setRole(ChildNode, Role);
 }
 
 void syntax::TreeBuilder::markExprChild(Expr *Child, NodeRole Role) {
   if (!Child)
     return;
-  Child = Child->IgnoreImplicit();
+  Child = IgnoreImplicit(Child);
 
   syntax::Tree *ChildNode = Mapping.find(Child);
   assert(ChildNode != nullptr);
   setRole(ChildNode, Role);
 }
 
 const syntax::Token *syntax::TreeBuilder::findToken(SourceLocation L) const {
   if (L.isInvalid())

clang/unittests/Tooling/Syntax/BuildTreeTest.cpp

TEST_P(SyntaxTreeTest, OverloadedOperator_Plus) {
   if (!GetParam().isCXX()) {
     return;
   }
   EXPECT_TRUE(treeDumpEqualOnAnnotations(
       R"cpp(
 struct X {
   friend X operator+(X, const X&);
 };
-// FIXME: Remove additional `UnknownExpression` wrapping `x`. For that, ignore
-// implicit copy constructor called on `x`. This should've been ignored already,
-// as we `IgnoreImplicit` when traversing an `Stmt`.
 void test(X x, X y) {
   [[x + y]];
 }
 )cpp",
       {R"txt(
 BinaryOperatorExpression Expression
-|-UnknownExpression LeftHandSide
-| `-IdExpression
-|   `-UnqualifiedId UnqualifiedId
-|     `-'x'
+|-IdExpression LeftHandSide
+| `-UnqualifiedId UnqualifiedId
+|   `-'x'
 |-'+' OperatorToken
 `-IdExpression RightHandSide
   `-UnqualifiedId UnqualifiedId
     `-'y'
 )txt"}));
 }
 
 TEST_P(SyntaxTreeTest, OverloadedOperator_Less) {
`-CompoundStatement
     | | |-IntegerLiteralExpression Expression
     | | | `-'2' LiteralToken
     | | `-';'
     | `-'}' CloseParen
     `-'}' CloseParen
 )txt"));
 }
 
+TEST_P(SyntaxTreeTest, InitDeclarator_Equal) {
+  if (!GetParam().isCXX()) {
+    return;
+  }
+  EXPECT_TRUE(treeDumpEqualOnAnnotations(
+      R"cpp(
+struct S { S(int);};
+void test() {
+  [[S s = 1]];
+}
+)cpp",
+      {R"txt(
+SimpleDeclaration
+|-'S'
+`-SimpleDeclarator Declarator
+  |-'s'
+  |-'='
+  `-IntegerLiteralExpression
+    `-'1' LiteralToken
+)txt"}));
+}
+
 TEST_P(SyntaxTreeTest, InitDeclarator_Brace) {
   if (!GetParam().isCXX11OrLater()) {
     return;
   }
-  EXPECT_TRUE(treeDumpEqual(
+  EXPECT_TRUE(treeDumpEqualOnAnnotations(
       R"cpp(
-int a {};
+struct S { 
+  S();
+  S(int);
+  S(int, float);
+};
+void test(){
+  // FIXME: 's...' is a declarator and '{...}' is initializer
+  [[S s0{}]];
+  [[S s1{1}]];
+  [[S s2{1, 2.}]];
+}
 )cpp",
+      {R"txt(
+SimpleDeclaration
+|-'S'
+`-SimpleDeclarator Declarator
+  `-UnknownExpression
+    |-'s0'
+    |-'{'
+    `-'}'
+  )txt",
        R"txt(
-TranslationUnit Detached
-`-SimpleDeclaration
-  |-'int'
-  |-SimpleDeclarator Declarator
-  | |-'a'
-  | `-UnknownExpression
-  |   `-UnknownExpression
-  |     |-'{'
-  |     `-'}'
-  `-';'
-)txt"));
+SimpleDeclaration
+|-'S'
+`-SimpleDeclarator Declarator
+  `-UnknownExpression
+    |-'s1'
+    |-'{'
+    |-IntegerLiteralExpression
+    | `-'1' LiteralToken
+    `-'}'
+  )txt",
+       R"txt(
+SimpleDeclaration
+|-'S'
+`-SimpleDeclarator Declarator
+  `-UnknownExpression
+    |-'s2'
+    |-'{'
+    |-IntegerLiteralExpression
+    | `-'1' LiteralToken
+    |-','
+    |-FloatingLiteralExpression
+    | `-'2.' LiteralToken
+    `-'}'
+)txt"}));
+}
+
+TEST_P(SyntaxTreeTest, InitDeclarator_EqualBrace) {
+  if (!GetParam().isCXX11OrLater()) {
+    return;
+  }
+  EXPECT_TRUE(treeDumpEqualOnAnnotations(
+      R"cpp(
+struct S { 
+  S();
+  S(int);
+  S(int, float);
+};
+void test() {
+  // FIXME: '= {...}' is initializer
+  [[S s0 = {}]];
+  [[S s1 = {1}]];
+  [[S s2 = {1, 2.}]];
+}
+)cpp",
+      {R"txt(
+SimpleDeclaration
+|-'S'
+`-SimpleDeclarator Declarator
+  |-'s0'
+  |-'='
+  `-UnknownExpression
+    |-'{'
+    `-'}'
+  )txt",
+       R"txt(
+SimpleDeclaration
+|-'S'
+`-SimpleDeclarator Declarator
+  |-'s1'
+  |-'='
+  `-UnknownExpression
+    |-'{'
+    |-IntegerLiteralExpression
+    | `-'1' LiteralToken
+    `-'}'
+  )txt",
+       R"txt(
+SimpleDeclaration
+|-'S'
+`-SimpleDeclarator Declarator
+  |-'s2'
+  |-'='
+  `-UnknownExpression
+    |-'{'
+    |-IntegerLiteralExpression
+    | `-'1' LiteralToken
+    |-','
+    |-FloatingLiteralExpression
+    | `-'2.' LiteralToken
+    `-'}'
+)txt"}));
 }
 
 TEST_P(SyntaxTreeTest, InitDeclarator_Paren) {
   if (!GetParam().isCXX()) {
     return;
   }
   EXPECT_TRUE(treeDumpEqualOnAnnotations(
       R"cpp(
 struct S {
   S(int);
+  S(int, float);
 };
-[[S s(1);]]
+// FIXME: 's...' is a declarator and '(...)' is initializer
+[[S s1(1);]]
+[[S s2(1, 2.);]]
 )cpp",
       {R"txt(
 SimpleDeclaration
 |-'S'
 |-SimpleDeclarator Declarator
 | `-UnknownExpression
-|   |-'s'
-|   |-'('
-|   |-IntegerLiteralExpression
-|   | `-'1' LiteralToken
-|   `-')'
+|   |-'s1'
+|   |-'('
+|   |-IntegerLiteralExpression
+|   | `-'1' LiteralToken
+|   `-')'
+`-';'
+  )txt",
+       R"txt(
+SimpleDeclaration
+|-'S'
+|-SimpleDeclarator Declarator
+| `-UnknownExpression
+|   |-'s2'
+|   |-'('
+|   |-IntegerLiteralExpression
+|   | `-'1' LiteralToken
+|   |-','
+|   |-FloatingLiteralExpression
+|   | `-'2.' LiteralToken
+|   `-')'
+`-';'
+)txt"}));
+}
+
+TEST_P(SyntaxTreeTest, ImplicitConversion_Argument) {
+  if (!GetParam().isCXX()) {
+    return;
+  }
+  EXPECT_TRUE(treeDumpEqualOnAnnotations(
+      R"cpp(
+struct X {
+  X(int);
+};
+void TakeX(const X&);
+void test() {
+  [[TakeX(1)]];
+}
+)cpp",
+      {R"txt(
+CallExpression Expression
+|-IdExpression Callee
+| `-UnqualifiedId UnqualifiedId
+|   `-'TakeX'
+|-'(' OpenParen
+|-CallArguments Arguments
+| `-IntegerLiteralExpression ListElement
+|   `-'1' LiteralToken
+`-')' CloseParen
+)txt"}));
+}
+
+TEST_P(SyntaxTreeTest, ImplicitConversion_Return) {
+  if (!GetParam().isCXX()) {
+    return;
+  }
+  EXPECT_TRUE(treeDumpEqualOnAnnotations(
+      R"cpp(
+struct X {
+  X(int);
+};
+X CreateX(){
+  [[return 1;]]
+}
+)cpp",
+      {R"txt(
+ReturnStatement Statement
+|-'return' IntroducerKeyword
+|-IntegerLiteralExpression ReturnValue
+| `-'1' LiteralToken
+`-';'
+)txt"}));
+}
+
+TEST_P(SyntaxTreeTest, ConstructorCall_ZeroArguments) {
+  if (!GetParam().isCXX()) {
+    return;
+  }
+  EXPECT_TRUE(treeDumpEqualOnAnnotations(
+      R"cpp(
+struct X {
+  X();
+};
+X test() {
+  [[return X();]]
+}
+)cpp",
+      {R"txt(
+ReturnStatement Statement
+|-'return' IntroducerKeyword
+|-UnknownExpression ReturnValue
+| |-'X'
+| |-'('
+| `-')'
+`-';'
+)txt"}));
+}
+
+TEST_P(SyntaxTreeTest, ConstructorCall_OneArgument) {
+  if (!GetParam().isCXX()) {
+    return;
+  }
+  EXPECT_TRUE(treeDumpEqualOnAnnotations(
+      R"cpp(
+struct X {
+  X(int);
+};
+X test() {
+  // FIXME: Remove `UnknownExpression` due to implicit `CXXFunctionalCastExpr`
+  [[return X(1);]]
+}
+)cpp",
+      {R"txt(
+ReturnStatement Statement
+|-'return' IntroducerKeyword
+|-UnknownExpression ReturnValue
+| `-UnknownExpression
+|   |-'X'
+|   |-'('
+|   |-IntegerLiteralExpression
+|   | `-'1' LiteralToken
+|   `-')'
+`-';'
+)txt"}));
+}
+
+TEST_P(SyntaxTreeTest, ConstructorCall_MultipleArguments) {
+  if (!GetParam().isCXX()) {
+    return;
+  }
+  EXPECT_TRUE(treeDumpEqualOnAnnotations(
+      R"cpp(
+struct X {
+  X(int, char);
+};
+X test() {
+  [[return X(1, '2');]]
+}
+)cpp",
+      {R"txt(
+ReturnStatement Statement
+|-'return' IntroducerKeyword
+|-UnknownExpression ReturnValue
+| |-'X'
+| |-'('
+| |-IntegerLiteralExpression
+| | `-'1' LiteralToken
+| |-','
+| |-CharacterLiteralExpression
+| | `-''2'' LiteralToken
+| `-')'
+`-';'
+)txt"}));
+}
+
+TEST_P(SyntaxTreeTest, TypeConversion_FunctionalNotation) {
+  if (!GetParam().isCXX()) {
+    return;
+  }
+  EXPECT_TRUE(treeDumpEqualOnAnnotations(
+      R"cpp(
+float test() {
+  [[return float(1);]]
+}
+)cpp",
+      {R"txt(
+ReturnStatement Statement
+|-'return' IntroducerKeyword
+|-UnknownExpression ReturnValue
+| |-'float'
+| |-'('
+| |-IntegerLiteralExpression
+| | `-'1' LiteralToken
+| `-')'
 `-';'
 )txt"}));
 }
 
 TEST_P(SyntaxTreeTest, ArrayDeclarator_Simple) {
   EXPECT_TRUE(treeDumpEqual(
       R"cpp(
 int a[10];