Fix crash on overloaded postfix unary operators due to invalid SourceLocation

[eduucaldas]

Code that reproduces the crash
Notice that when using a postfix unary operator ++ one argument is introduced to differ it from its prefix counterpart, but that argument is not used. This phantom argument shows up in the ClangAST in the form of an IntegerLiteral with invalid sloc. This invalid sloc in a valid AST node was causing the SyntaxTree generation to crash.
We can address this problems at two different levels:

1. At the TraverseCXXOperatorCallExpr, by overriding it and replacing the children iteration to not iterate over this phantom argument.
2. At the WalkUpFromIntegerLiteral, by skipping the visitation of the phantom node.

We preferred the latter.

1. Cons: We would have to duplicate the implementation of RecursiveASTVisitor in BuildTree, to handle this subtle change in traversal. That would add code complexity.
2. Cons: We are handling a problem of CXXOperatorCallExpr in IntegerLiteral.

We chose the latter as, anyways, if an AST node has an invalid sloc, it was either a problem with parsing or the node is supposed to be ignored

clang/include/clang/AST/ExprCXX.h

143–148

Should new additions to CXXOperatorCallExpr go on a different patch?
I'll also add unit tests for those. I'm waiting for review on https://reviews.llvm.org/D82937, to do that.

[riccibruno]

clang/lib/AST/ExprCXX.cpp

82

This will be true for operator->.

[eduucaldas]

clang/lib/AST/ExprCXX.cpp

82

Thank you ! I had missed that! I'll propose a more robust solution. I'll probably split this patch. But I'll take care to subscribe you to anything related

[eduucaldas]

• Remove predicates in CXXOperatorCallExpr.
• Implement getOperatorNodeKind to dispatch from CXXOperatorCallExpr to syntax constructs like BinaryOperatorExpression, PrefixUnaryOperatorExpression...

[eduucaldas]

Nits

[eduucaldas]

Revert mistake on last update, used arc diff --update with the wrong diff

[eduucaldas]

clang/lib/Tooling/Syntax/BuildTree.cpp

117

Where to put this logic?

The pro of having this function inside BuildTree.cpp is that it is closer to it's *only* usage. And also for now BuildTree.cpp agglomerates everything that takes a semantic AST as an input, so it would be coherent.

Another option is to put it in Nodes, as it might serve as documentation for *OperatorExpressions.
For example, it would document that the semantic node CXXOperatorCallExpr can also generate the syntax node BinaryOperatorExpression, which was previously only generated by a semantic BinaryOperator

Another option is to add put this function as a lambda inside WalkUpFromCXXOperatorCallExpr as probably it will only be used there.

[eduucaldas]

clang/lib/Tooling/Syntax/BuildTree.cpp

187

Actually arrow star is treated like a normal binary operator in the grammar

[gribozavr2]

clang/lib/Tooling/Syntax/BuildTree.cpp

117

Placing this helper here makes sense to me. However, please mark it static.

177

"Not supported by syntax tree *yet*"

651

WDYT about overriding TraverseCXXOperatorCallExpr, so that we don't even visit the synthetic argument of the postfix unary ++? I would prefer to not introduce blanket "if invalid then ignore" checks in the code.

761

"getOperatorNodeKind() does not return this value"

clang/unittests/Tooling/Syntax/TreeTest.cpp

2278

Could you also add tests for a prefix unary operator?

2321

I'm not sure about this part where ++ is wrapped in IdExpression -- shouldn't the syntax tree look identical to a builtin postfix ++ (see another test in this file)?

[eduucaldas]

->* and , are binary operators.
Unit tests covering most of the operator kinds

[eduucaldas]

Answering comments

[eduucaldas]

clang/lib/Tooling/Syntax/BuildTree.cpp

651

In D82954#2125300, @eduucaldas wrote:

Code that reproduces the crash
Notice that when using a postfix unary operator ++ one argument is introduced to differ it from its prefix counterpart, but that argument is not used. This phantom argument shows up in the ClangAST in the form of an IntegerLiteral with invalid sloc. This invalid sloc in a valid AST node was causing the SyntaxTree generation to crash.
We can address this problems at two different levels:

1. At the TraverseCXXOperatorCallExpr, by overriding it and replacing the children iteration to not iterate over this phantom argument.
2. At the WalkUpFromIntegerLiteral, by skipping the visitation of the phantom node.

We preferred the latter.

1. Cons: We would have to duplicate the implementation of RecursiveASTVisitor in BuildTree, to handle this subtle change in traversal. That would add code complexity.
2. Cons: We are handling a problem of CXXOperatorCallExpr in IntegerLiteral.

We chose the latter as, anyways, if an AST node has an invalid sloc, it was either a problem with parsing or the node is supposed to be ignored

I've explained my reasoning in my first comment for this patch. But as it was a long time ago, I guess it got lost, even by me.

I'll sketch how the Traverse solution would look like, to be able to give more concrete arguments.

clang/unittests/Tooling/Syntax/TreeTest.cpp

2321

This comes back to a discussion we had a month ago about operators ( +, !, etc)
Question: Should we represent operators (built-in or overloaded) in the syntax tree uniformly? If so in which way?
Context: The ClangAST stores built-in operators as mere tokens, whereas overloaded operators are represented as a DeclRefExpr. That makes a lot of sense for the ClangAST, as we might refer back to the declaration of the overloaded operator, but the declaration of built-in operator doesn't exist.
Conclusion: Have the same representation for both types of operators. I have implemented the "unboxed" representation of overloaded operators, i.e. just storing their token in the syntax tree. I have not committed that, but I can do it just after this patch.

[eduucaldas]

Unifying user defined binary operator tests

[gribozavr2]

clang/lib/Tooling/Syntax/BuildTree.cpp

148

Please drop this comment, I don't think it helps understand the code.

clang/unittests/Tooling/Syntax/TreeTest.cpp

2137

I don't think we need a separate class to show the left shift operator. The declaration below can be:

friend X operator<<(X&, const X&);

2143

"TODO: Fix the crash on ->* and add a test for it."

2321

SGTM. Please just leave a FIXME or TODO in this patch that shows that certain parts of tests are not final.

[eduucaldas]

answering minor comments

clang/unittests/Tooling/Syntax/TreeTest.cpp

2137

If we don't bother much about "realistic" operator declarations we could drop all the friend and declare every operator in their most concise form. WDYT

[gribozavr2]

clang/unittests/Tooling/Syntax/TreeTest.cpp

2137

I think we shouldn't try to make tests realistic in terms of function names etc., but we should try to cover as many different AST shapes as possible. In the case of binary operators, we have three cases -- free function, friend function, member function, that all generate slightly different ASTs, so I believe we should try to cover them all.

[eduucaldas]

clang/unittests/Tooling/Syntax/TreeTest.cpp

2137

But those all regard the operators declaration.
Here we test the operator call expression - CXXOperatorCallExpr.

I think we should test friend function declarations when we add support for them in the tree, and then we add tests for declaration of friend operators, friend member functions and whatnot.

[gribozavr2]

clang/unittests/Tooling/Syntax/TreeTest.cpp

2137

The call expression AST node can be meaningfully different between calls to member and non-member functions.

[eduucaldas]

Switch to TraverseCXXOperatorCallExpr

[gribozavr2]

clang/lib/Tooling/Syntax/BuildTree.cpp

728

"is used to distinguish"

731

"... because does not correspond to anything written in the source code"

[eduucaldas]

minor fix comments

clang/include/clang/AST/ExprCXX.h

public:
 
   static bool isAssignmentOp(OverloadedOperatorKind Opc) {
     return Opc == OO_Equal || Opc == OO_StarEqual || Opc == OO_SlashEqual ||
            Opc == OO_PercentEqual || Opc == OO_PlusEqual ||
            Opc == OO_MinusEqual || Opc == OO_LessLessEqual ||
            Opc == OO_GreaterGreaterEqual || Opc == OO_AmpEqual ||
            Opc == OO_CaretEqual || Opc == OO_PipeEqual;
   }
+
   bool isAssignmentOp() const { return isAssignmentOp(getOperator()); }
 
   static bool isComparisonOp(OverloadedOperatorKind Opc) {
     switch (Opc) {
     case OO_EqualEqual:
     case OO_ExclaimEqual:
     case OO_Greater:
     case OO_GreaterEqual:
     case OO_Less:
     case OO_LessEqual:
     case OO_Spaceship:
       return true;
     default:
       return false;
     }
   }
   bool isComparisonOp() const { return isComparisonOp(getOperator()); }
 
-  /// Is this written as an infix binary operator?
+  bool isPostfixUnaryOp() const;
+
+  bool isPrefixUnaryOp() const;
+
+  bool isUnaryOp() const;
+
   bool isInfixBinaryOp() const;
 
   /// Returns the location of the operator symbol in the expression.
   ///
   /// When \c getOperator()==OO_Call, this is the location of the right
   /// parentheses; when \c getOperator()==OO_Subscript, this is the location
   /// of the right bracket.
   SourceLocation getOperatorLoc() const { return getRParenLoc(); }

clang/lib/AST/ExprCXX.cpp

 
 //===----------------------------------------------------------------------===//
 //  Child Iterators for iterating over subexpressions/substatements
 //===----------------------------------------------------------------------===//
 
 bool CXXOperatorCallExpr::isInfixBinaryOp() const {
   if (getNumArgs() != 2)
     return false;
-
   switch (getOperator()) {
   // operator() may have two arguments, but it's not a binary operator
   case OO_Call:
   // operator[] takes two arguments but it's not infix
   case OO_Subscript:
   // Postfix unary operators (++ and --) take 2 arguments to differ from their
   // prefix counterparts
   case OO_PlusPlus:
   case OO_MinusMinus:
     return false;
   default:
     return true;
   }
 }
 
+bool CXXOperatorCallExpr::isPostfixUnaryOp() const {
+  switch (getOperator()) {
+  case OO_PlusPlus:
+  case OO_MinusMinus:
+    return getNumArgs() == 2;
+  default:
+    return false;
+  }
+}
+
+bool CXXOperatorCallExpr::isPrefixUnaryOp() const {
+  switch (getOperator()) {
+  case OO_Call:
+  case OO_Subscript:
+    return false;
+  default:
+    return getNumArgs() == 1;
+  }
+}
+
+bool CXXOperatorCallExpr::isUnaryOp() const {
+  return isPrefixUnaryOp() || isPostfixUnaryOp();
+}
+
 CXXRewrittenBinaryOperator::DecomposedForm
 CXXRewrittenBinaryOperator::getDecomposedForm() const {
   DecomposedForm Result = {};
   const Expr *E = getSemanticForm()->IgnoreImplicit();
 
   // Remove an outer '!' if it exists (only happens for a '!=' rewrite).
   bool SkippedNot = false;
   if (auto *NotEq = dyn_cast<UnaryOperator>(E)) {

clang/lib/Tooling/Syntax/BuildTree.cpp

template <class PtrLoc> SourceLocation HandlePointer(PtrLoc T) {
     auto L = Visit(T.getPointeeLoc());
     if (L.isValid())
       return L;
     return T.getLocalSourceRange().getBegin();
   }
 };
 } // namespace
 
 /// Gets the range of declarator as defined by the C++ grammar. E.g.
 ///     `int a;` -> range of `a`,
 ///     `int *a;` -> range of `*a`,
 ///     `int a[10];` -> range of `a[10]`,
 ///     `int a[1][2][3];` -> range of `a[1][2][3]`,
 ///     `int *a = nullptr` -> range of `*a = nullptr`.
 /// FIMXE: \p Name must be a source range, e.g. for `operator+`.
 static SourceRange getDeclaratorRange(const SourceManager &SM, TypeLoc T,
                                       SourceLocation Name,
static SourceRange getDeclaratorRange(const SourceManager &SM, TypeLoc T,
   }
   return SourceRange(Start, End);
 }
 
 namespace {
 /// All AST hierarchy roots that can be represented as pointers.
 using ASTPtr = llvm::PointerUnion<Stmt *, Decl *>;
 /// Maintains a mapping from AST to syntax tree nodes. This class will get more
 /// complicated as we support more kinds of AST nodes, e.g. TypeLocs.
 /// FIXME: expose this as public API.
 class ASTToSyntaxMapping {
 public:
   void add(ASTPtr From, syntax::Tree *To) {
     assert(To != nullptr);
     assert(!From.isNull());
 
     bool Added = Nodes.insert({From, To}).second;

 /// AST.
 ///
 /// At each point of the traversal we maintain a list of pending nodes.
 /// Initially all tokens are added as pending nodes. When processing a clang AST
 /// node, the clients need to:
 ///   - create a corresponding syntax node,
 ///   - assign roles to all pending child nodes with 'markChild' and
 ///     'markChildToken',
 ///   - replace the child nodes with the new syntax node in the pending list
 ///     with 'foldNode'.
 ///
 /// Note that all children are expected to be processed when building a node.
 ///
 /// Call finalize() to finish building the tree and consume the root node.
 class syntax::TreeBuilder {
 public:
   TreeBuilder(syntax::Arena &Arena) : Arena(Arena), Pending(Arena) {
     for (const auto &T : Arena.tokenBuffer().expandedTokens())
       LocationToToken.insert({T.location().getRawEncoding(), &T});
   }
 
   llvm::BumpPtrAllocator &allocator() { return Arena.allocator(); }
   const SourceManager &sourceManager() const { return Arena.sourceManager(); }
 
   /// Populate children for \p New node, assuming it covers tokens from \p
   /// Range.
   void foldNode(llvm::ArrayRef<syntax::Token> Range, syntax::Tree *New,
bool WalkUpFromDeclRefExpr(DeclRefExpr *S) {
     Builder.markChild(unqualifiedId, syntax::NodeRole::IdExpression_id);
 
     Builder.foldNode(Builder.getExprRange(S),
                      new (allocator()) syntax::IdExpression, S);
     return true;
   }
 
   bool WalkUpFromIntegerLiteral(IntegerLiteral *S) {
+    if (S->getLocation().isInvalid())
+      return true;
     Builder.markChildToken(S->getLocation(), syntax::NodeRole::LiteralToken);
     Builder.foldNode(Builder.getExprRange(S),
                      new (allocator()) syntax::IntegerLiteralExpression, S);
     return true;
   }
 
   bool WalkUpFromCharacterLiteral(CharacterLiteral *S) {
     Builder.markChildToken(S->getLocation(), syntax::NodeRole::LiteralToken);
Builder.foldNode(Builder.getExprRange(S),
                      new (allocator()) syntax::BinaryOperatorExpression, S);
     return true;
   }
 
   bool WalkUpFromCXXOperatorCallExpr(CXXOperatorCallExpr *S) {
     if (S->isInfixBinaryOp()) {
       Builder.markExprChild(
           S->getArg(0),
           syntax::NodeRole::BinaryOperatorExpression_leftHandSide);
       Builder.markChildToken(
           S->getOperatorLoc(),
           syntax::NodeRole::OperatorExpression_operatorToken);
       Builder.markExprChild(
           S->getArg(1),
           syntax::NodeRole::BinaryOperatorExpression_rightHandSide);
       Builder.foldNode(Builder.getExprRange(S),
                        new (allocator()) syntax::BinaryOperatorExpression, S);
       return true;
-    }
+    } else if (S->isUnaryOp()) {
+      Builder.markChildToken(
+          S->getOperatorLoc(),
+          syntax::NodeRole::OperatorExpression_operatorToken);
+      Builder.markExprChild(S->getArg(0),
+                            syntax::NodeRole::UnaryOperatorExpression_operand);
+      if (S->isPostfixUnaryOp())
+        Builder.foldNode(
+            Builder.getExprRange(S),
+            new (allocator()) syntax::PostfixUnaryOperatorExpression, S);
+      else
+        Builder.foldNode(
+            Builder.getExprRange(S),
+            new (allocator()) syntax::PrefixUnaryOperatorExpression, S);
+      return true;
+    } else
       return RecursiveASTVisitor::WalkUpFromCXXOperatorCallExpr(S);
   }
 
   bool WalkUpFromNamespaceDecl(NamespaceDecl *S) {
     auto Tokens = Builder.getDeclarationRange(S);
     if (Tokens.front().kind() == tok::coloncolon) {
       // Handle nested namespace definitions. Those start at '::' token, e.g.
       // namespace a^::b {}
       // FIXME: build corresponding nodes for the name of this namespace.
       return true;
     }
     Builder.foldNode(Tokens, new (allocator()) syntax::NamespaceDefinition, S);
     return true;
   }
 
   bool TraverseParenTypeLoc(ParenTypeLoc L) {

clang/unittests/Tooling/Syntax/TreeTest.cpp

 }
 
 TEST_P(SyntaxTreeTest, UserDefinedBinaryOperator) {
   if (!GetParam().isCXX()) {
     return;
   }
   EXPECT_TRUE(treeDumpEqual(
       R"cpp(
 struct X {
   X& operator=(const X&);
   friend X operator+(X, const X&);
   friend bool operator<(const X&, const X&);
 };
 void test(X x, X y) {
   x = y;
   x + y;
   x < y;
 }
 )cpp",
       R"txt(
 *: TranslationUnit
 |-SimpleDeclaration
 | |-struct
`-CompoundStatement
     | | `-IdExpression
     | |   `-UnqualifiedId
     | |     `-y
     | `-;
     `-}
 )txt"));
 }
 
+TEST_P(SyntaxTreeTest, UserDefinedUnaryPostfixOperator) {
+  if (!GetParam().isCXX()) {
+    return;
+  }
+  EXPECT_TRUE(treeDumpEqual(
+      R"cpp(
+struct X {
+  X operator++(int);
+};
+void test(X x) {
+  x++;
+}
+)cpp",
+      R"txt(
+*: TranslationUnit
+|-SimpleDeclaration
+| |-struct
+| |-X
+| |-{
+| |-SimpleDeclaration
+| | |-X
+| | |-SimpleDeclarator
+| | | |-operator
+| | | |-++
+| | | `-ParametersAndQualifiers
+| | |   |-(
+| | |   |-SimpleDeclaration
+| | |   | `-int
+| | |   `-)
+| | `-;
+| |-}
+| `-;
+`-SimpleDeclaration
+  |-void
+  |-SimpleDeclarator
+  | |-test
+  | `-ParametersAndQualifiers
+  |   |-(
+  |   |-SimpleDeclaration
+  |   | |-X
+  |   | `-SimpleDeclarator
+  |   |   `-x
+  |   `-)
+  `-CompoundStatement
+    |-{
+    |-ExpressionStatement
+    | |-PostfixUnaryOperatorExpression
+    | | |-IdExpression
+    | | | `-UnqualifiedId
+    | | |   `-x
+    | | `-IdExpression
+    | |   `-UnqualifiedId
+    | |     `-++
+    | `-;
+    `-}
+)txt"));
+}
+
 TEST_P(SyntaxTreeTest, MultipleDeclaratorsGrouping) {
   EXPECT_TRUE(treeDumpEqual(
       R"cpp(
 int *a, b;
 int *c, d;
 )cpp",
       R"txt(
 *: TranslationUnit