Add support for DeclRefExpr in SyntaxTree, by generating IdExpressions

[eduucaldas]

clang/unittests/Tooling/Syntax/TreeTest.cpp

829

Perhaps we shouldn't differ between specifiers as that is semantical information

[eduucaldas]

.

clang/unittests/Tooling/Syntax/TreeTest.cpp

829

Followed the grammar, but added specifier in the end as we also hold the :: and not only the names

[gribozavr2]

clang/unittests/Tooling/Syntax/TreeTest.cpp

815

This part does not seem to appear in the source.

[eduucaldas]

cleanup for upstreaming

[gribozavr2]

clang/include/clang/Tooling/Syntax/Nodes.h

191

Should there be getters for various parts of the specifier?

204

Could you add more details? Feel free to even quote the relevant grammar bits.

212

unqualified-id in the grammar can be more than one token.

213

Could you swap the order of getters to match the source order?

clang/lib/Tooling/Syntax/BuildTree.cpp

618

Do we need to mark the role if it is unknown?

clang/unittests/Tooling/Syntax/TreeTest.cpp

751

Could you add more tests even if they fail today due to missing implementation?

a::b::operator-(a::b::S())
a::b::S2<int>::f()

etc. Basically cover the whole grammar in tests, if possible.

[eduucaldas]

clang/lib/Tooling/Syntax/BuildTree.cpp

618

Thanks, that had slipped through, sorry for that

[eduucaldas]

This comment was removed by eduucaldas.

[eduucaldas]

Answering simple comments

[gribozavr2]

clang/include/clang/Tooling/Syntax/Nodes.h

209

Please add a TODO for the accessor for the 'template' keyword (and a test that has that keyword).

[eduucaldas]

• cleanup for upstreaming
• better coverage for unqualified-id
• Better coverage for qualified-id
• Finish implementing unqualified-id.

[eduucaldas]

clang/include/clang/Tooling/Syntax/Nodes.h

191

I think not.

209

I can implement accessor. But I couldn't write a test with this template keyword that uses DeclRefExpr. This is the test I came up with, note that both expressions are represented as DependentScopeDeclRefExpr

216

Just by looking at code it is impossible to know if the accessed thing is optional or not.
IdExpression ALWAYS has a UnqualifiedId
IdExpression MAY have a NestedNameSpecifier
This same issue repeats in other places

[eduucaldas]

Fix mistake on getting SourceRange for template-id

[gribozavr2]

clang/include/clang/Tooling/Syntax/Nodes.h

186

Could you provide examples or a grammar quote in the comment?

206

"the size in std::vector<int>::size"

213

Could you add TODOs about adding accessors?

clang/unittests/Tooling/Syntax/TreeTest.cpp

696

Could you split off the parts of this test that don't depend on C++11?

979

"WithTemplateKeyword"

1097

"QualifiedIdDecltype" for consistency with other tests.

1105

Indent -2?

1108

Could you indent the second line?

[eduucaldas]

Aswering comments

[eduucaldas]

clang/include/clang/Tooling/Syntax/Nodes.h

191

Pospone to interface discussion with Yitzhak

213

Pospone to interface discussion with Yitzhak

clang/lib/Tooling/Syntax/Nodes.cpp

176–195

I was thinking and our problem is bigger than not testing for NodeRoles, we don't test accessors at all!

180

This should be an assert for now, as if that is not true it would be a logic problem. Same reasoning applies to CompoundStatement

[gribozavr2]

clang/lib/Tooling/Syntax/Nodes.cpp

180

Yep, feel free to change it to an assert.

[eduucaldas]

change if -> asserts

[MaskRay]

@eduucaldas

Hi, you can drop Reviewers: Subscribers: Tags: and the text Summary: from the git commit with the following script:

arcfilter () {
        arc amend
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
}

Reviewed By: is considered important by some people. Please keep the tag. (I have updated my script to use --date=now (setting author date to committer date))

https://reviews.llvm.org/D80978 contains a git pre-push hook to automate this.

clang/include/clang/Tooling/Syntax/Nodes.h

enum class NodeKind : uint16_t {
   // Declarators.
   SimpleDeclarator,
   ParenDeclarator,
 
   ArraySubscript,
   TrailingReturnType,
   ParametersAndQualifiers,
   MemberPointer,
-  Operator,
   NestedNameSpecifier,
-  NameQualifier
+  NameSpecifier
 };
 /// For debugging purposes.
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, NodeKind K);
 
 /// A relation between a parent and child node, e.g. 'left-hand-side of
 /// a binary expression'. Used for implementing accessors.
 ///
 /// Some roles describe parent/child relations that occur multiple times in
enum class NodeRole : uint8_t {
   SimpleDeclaration_declarator,
   TemplateDeclaration_declaration,
   ExplicitTemplateInstantiation_declaration,
   ArraySubscript_sizeExpression,
   TrailingReturnType_declarator,
   ParametersAndQualifiers_parameter,
   ParametersAndQualifiers_trailingReturn,
   IdExpression_unqualifiedId,
-  IdExpression_qualifier
+  IdExpression_qualifier,
+  NestedNameSpecifier_specifier
 };
 /// For debugging purposes.
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, NodeRole R);
 
 class SimpleDeclarator;
 
 /// A root node for a translation unit. Parent is always null.
 class TranslationUnit final : public Tree {

 public:
   Expression(NodeKind K) : Tree(K) {}
   static bool classof(const Node *N) {
     return NodeKind::UnknownExpression <= N->kind() &&
            N->kind() <= NodeKind::UnknownExpression;
   }
 };
 
-class NameQualifier final : public Tree {
+class NameSpecifier final : public Tree {
 public:
-  NameQualifier() : Tree(NodeKind::NameQualifier) {}
+  NameSpecifier() : Tree(NodeKind::NameSpecifier) {}
   static bool classof(const Node *N) {
-    return N->kind() == NodeKind::NameQualifier;
+    return N->kind() == NodeKind::NameSpecifier;
   }
 };
 
 class NestedNameSpecifier final : public Tree {
 public:
   NestedNameSpecifier() : Tree(NodeKind::NestedNameSpecifier) {}
   static bool classof(const Node *N) {
     return N->kind() <= NodeKind::NestedNameSpecifier;
   }
 
-  std::vector<syntax::NameQualifier *> specifiers();
-};
-
-/// An expression of an unknown kind, i.e. one not currently handled by the
-/// syntax tree.
-class UnknownExpression final : public Expression {
-public:
-  UnknownExpression() : Expression(NodeKind::UnknownExpression) {}
-  static bool classof(const Node *N) {
-    return N->kind() == NodeKind::UnknownExpression;
-  }
+  std::vector<syntax::NameSpecifier *> specifiers();
 };
 
 /// An identifier expression, e.g. `n::S::a`. Modeled according to the grammar
+/// id-expression:
+///   unqualified-id
+///   qualified-id
+/// qualified-id:
+///   nested-name-specifier template_opt unqualified-id
 class IdExpression final : public Expression {
 public:
   IdExpression() : Expression(NodeKind::IdExpression) {}
   static bool classof(const Node *N) {
     return N->kind() == NodeKind::IdExpression;
   }
 
+  syntax::NestedNameSpecifier *qualifier();
   syntax::Leaf *unqualifiedId();
-  syntax::Leaf *qualifier();
+};
+
+/// An expression of an unknown kind, i.e. one not currently handled by the
+/// syntax tree.
+class UnknownExpression final : public Expression {
+public:
+  UnknownExpression() : Expression(NodeKind::UnknownExpression) {}
+  static bool classof(const Node *N) {
+    return N->kind() == NodeKind::UnknownExpression;
+  }
 };
 
 /// C++11 'nullptr' expression.
 class CxxNullPtrExpression final : public Expression {
 public:
   CxxNullPtrExpression() : Expression(NodeKind::CxxNullPtrExpression) {}
   static bool classof(const Node *N) {
     return N->kind() == NodeKind::CxxNullPtrExpression;

clang/lib/Tooling/Syntax/BuildTree.cpp

public:
   bool WalkUpFromExpr(Expr *E) {
     assert(!isImplicitExpr(E) && "should be handled by TraverseStmt");
     Builder.foldNode(Builder.getExprRange(E),
                      new (allocator()) syntax::UnknownExpression, E);
     return true;
   }
 
   syntax::NestedNameSpecifier *
-  BuildNestedNameSpecifier(NestedNameSpecifierLoc NNS) {
-    if (!NNS)
+  BuildNestedNameSpecifier(NestedNameSpecifierLoc QualifierLoc) {
+    if (!QualifierLoc)
       return nullptr;
-    NestedNameSpecifierLoc it = NNS;
-    while (it) {
-      auto *NS = new (allocator()) syntax::NameQualifier;
+    for (auto it = QualifierLoc; it; it = it.getPrefix()) {
+      auto *NS = new (allocator()) syntax::NameSpecifier;
       Builder.foldNode(Builder.getRange(it.getLocalSourceRange()), NS, nullptr);
-      Builder.markChild(NS, syntax::NodeRole::Unknown);
-      it = it.getPrefix();
+      Builder.markChild(NS, syntax::NodeRole::NestedNameSpecifier_specifier);
     }
-    syntax::NestedNameSpecifier *sNNS =
-        new (allocator()) syntax::NestedNameSpecifier;
-    Builder.foldNode(Builder.getRange(NNS.getSourceRange()), sNNS, nullptr);
-    return sNNS;
+    auto *NNS = new (allocator()) syntax::NestedNameSpecifier;
+    Builder.foldNode(Builder.getRange(QualifierLoc.getSourceRange()), NNS,
+                     nullptr);
+    return NNS;
   }
 
   bool WalkUpFromDeclRefExpr(DeclRefExpr *S) {
-    if (S->getNameInfo().getName().getNameKind() ==
-        clang::DeclarationName::CXXOperatorName) {
-      return true;
-    } else {
-      auto *sNNS = BuildNestedNameSpecifier(S->getQualifierLoc());
-      if (sNNS)
-        Builder.markChild(sNNS, syntax::NodeRole::IdExpression_qualifier);
-      Builder.markChildToken(S->getLocation(),
-                             syntax::NodeRole::IdExpression_unqualifiedId);
-      Builder.foldNode(Builder.getExprRange(S),
-                       new (allocator()) syntax::IdExpression, S);
-    }
+    if (auto *NNS = BuildNestedNameSpecifier(S->getQualifierLoc()))
+      Builder.markChild(NNS, syntax::NodeRole::IdExpression_qualifier);
+
     return true;
   }
 
   bool WalkUpFromIntegerLiteral(IntegerLiteral *S) {
     Builder.markChildToken(S->getLocation(), syntax::NodeRole::LiteralToken);
     Builder.foldNode(Builder.getExprRange(S),
                      new (allocator()) syntax::IntegerLiteralExpression, S);
     return true;

clang/lib/Tooling/Syntax/Nodes.cpp

llvm::raw_ostream &syntax::operator<<(llvm::raw_ostream &OS, NodeKind K) {
   case NodeKind::ArraySubscript:
     return OS << "ArraySubscript";
   case NodeKind::TrailingReturnType:
     return OS << "TrailingReturnType";
   case NodeKind::ParametersAndQualifiers:
     return OS << "ParametersAndQualifiers";
   case NodeKind::MemberPointer:
     return OS << "MemberPointer";
-  case NodeKind::Operator:
-    return OS << "Operator";
-  case NodeKind::NameQualifier:
-    return OS << "NameQualifier";
+  case NodeKind::NameSpecifier:
+    return OS << "NameSpecifier";
   case NodeKind::NestedNameSpecifier:
     return OS << "NestedNameSpecifier";
   }
   llvm_unreachable("unknown node kind");
 }
 
 llvm::raw_ostream &syntax::operator<<(llvm::raw_ostream &OS, NodeRole R) {
   switch (R) {
llvm::raw_ostream &syntax::operator<<(llvm::raw_ostream &OS, NodeRole R) {
   case syntax::NodeRole::ParametersAndQualifiers_parameter:
     return OS << "ParametersAndQualifiers_parameter";
   case syntax::NodeRole::ParametersAndQualifiers_trailingReturn:
     return OS << "ParametersAndQualifiers_trailingReturn";
   case syntax::NodeRole::IdExpression_unqualifiedId:
     return OS << "IdExpression_unqualifiedId";
   case syntax::NodeRole::IdExpression_qualifier:
     return OS << "IdExpression_qualifier";
+  case syntax::NodeRole::NestedNameSpecifier_specifier:
+    return OS << "NestedNameSpecifier_specifier";
   }
   llvm_unreachable("invalid role");
 }
 
-std::vector<syntax::NameQualifier *> syntax::NestedNameSpecifier::specifiers() {
-  std::vector<syntax::NameQualifier *> Children;
+std::vector<syntax::NameSpecifier *> syntax::NestedNameSpecifier::specifiers() {
+  std::vector<syntax::NameSpecifier *> Children;
   for (auto *C = firstChild(); C; C = C->nextSibling()) {
-    Children.push_back(llvm::cast<syntax::NameQualifier>(C));
+    if (C->role() == syntax::NodeRole::NestedNameSpecifier_specifier)
+      Children.push_back(llvm::cast<syntax::NameSpecifier>(C));
   }
   return Children;
 }
-syntax::Leaf *syntax::IdExpression::qualifier() {
-  return llvm::cast_or_null<syntax::Leaf>(
+
+syntax::NestedNameSpecifier *syntax::IdExpression::qualifier() {
+  return llvm::cast_or_null<syntax::NestedNameSpecifier>(
       findChild(syntax::NodeRole::IdExpression_qualifier));
 }
 
 syntax::Leaf *syntax::IdExpression::unqualifiedId() {
   return llvm::cast_or_null<syntax::Leaf>(
       findChild(syntax::NodeRole::IdExpression_unqualifiedId));
 }
 
 syntax::Leaf *syntax::IntegerLiteralExpression::literalToken() {
   return llvm::cast_or_null<syntax::Leaf>(
       findChild(syntax::NodeRole::LiteralToken));
 }
 
 syntax::Leaf *syntax::CxxNullPtrExpression::nullPtrKeyword() {
   return llvm::cast_or_null<syntax::Leaf>(
       findChild(syntax::NodeRole::LiteralToken));

clang/unittests/Tooling/Syntax/TreeTest.cpp

`-CompoundStatement
     |   | |-(
     |   | `-)
     |   `-;
     `-}
 )txt"));
 }
 
 TEST_P(SyntaxTreeTest, UnqualifiedId) {
   EXPECT_TRUE(treeDumpEqual(
       R"cpp(
 void test(int b) {
   int a;
   a = b;
 }
 )cpp",
       R"txt(
 *: TranslationUnit
namespace a {
   namespace b {
     struct S {
       int i;
       static void f(){}
     };
   }
 }
 void test(int b) {
   ::a::b::S::f();
 }
 )cpp",
       R"txt(
 *: TranslationUnit
 |-NamespaceDefinition
 | |-namespace
 | |-a
 | |-{
`-SimpleDeclaration
   |   |   `-b
   |   `-)
   `-CompoundStatement
     |-{
     |-ExpressionStatement
     | |-UnknownExpression
     | | |-IdExpression
     | | | |-NestedNameSpecifier
-    | | | | |-NameQualifier
+    | | | | |-NameSpecifier
     | | | | | `-::
-    | | | | |-NameQualifier
+    | | | | |-NameSpecifier
     | | | | | |-a
     | | | | | `-::
-    | | | | |-NameQualifier
+    | | | | |-NameSpecifier
     | | | | | |-b
     | | | | | `-::
-    | | | | `-NameQualifier
+    | | | | `-NameSpecifier
     | | | |   |-S
     | | | |   `-::
     | | | `-f
     | | |-(
     | | `-)
     | `-;
     `-}
 )txt"));
 }
 
 TEST_P(SyntaxTreeTest, CxxNullPtrLiteral) {
   if (!GetParam().isCXX11OrLater()) {
     return;
   }
   EXPECT_TRUE(treeDumpEqual(
       R"cpp(
 void test() {
   nullptr;
 }
 )cpp",
       R"txt(
 *: TranslationUnit
 `-SimpleDeclaration
   |-void
TEST_P(SyntaxTreeTest, IntegerLiteralWithDigitSeparators) {
   EXPECT_TRUE(treeDumpEqual(
       R"cpp(
 void test() {
   1'2'0ull;
 }
 )cpp",
       R"txt(
 *: TranslationUnit
 `-SimpleDeclaration
   |-void
   |-SimpleDeclarator
   | |-test
   | `-ParametersAndQualifiers
   |   |-(
   |   `-)
   `-CompoundStatement
     |-{
`-CompoundStatement
     | |-PrefixUnaryOperatorExpression
     | | |--
     | | `-IdExpression
     | |   `-a
     | `-;
     |-ExpressionStatement
     | |-PrefixUnaryOperatorExpression
     | | |-+
     | | `-IdExpression
     | |   `-a
     | `-;
     |-ExpressionStatement
     | |-PrefixUnaryOperatorExpression
     | | |-&
     | | `-IdExpression
     | |   `-a
     | `-;
     |-ExpressionStatement
     | |-PrefixUnaryOperatorExpression
     | | |-*
     | | `-IdExpression
     | |   `-ap
     | `-;
     |-ExpressionStatement
     | |-PrefixUnaryOperatorExpression
     | | |-!
     | | `-IdExpression
     | |   `-a
`-SimpleDeclaration
   |   |   `-y
   |   `-)
   `-CompoundStatement
     |-{
     |-ExpressionStatement
     | |-BinaryOperatorExpression
     | | |-IdExpression
     | | | `-x
-    | | |-=
+    | | |-IdExpression
+    | | | `-=
     | | `-IdExpression
     | |   `-y
     | `-;
     |-ExpressionStatement
     | |-BinaryOperatorExpression
     | | |-UnknownExpression
     | | | `-IdExpression
     | | |   `-x
-    | | |-+
+    | | |-IdExpression
+    | | | `-+
     | | `-IdExpression
     | |   `-y
     | `-;
     |-ExpressionStatement
     | |-BinaryOperatorExpression
     | | |-IdExpression
     | | | `-x
-    | | |-<
+    | | |-IdExpression
+    | | | `-<
     | | `-IdExpression
     | |   `-y
     | `-;
     `-}
 )txt"));
 }
 
 TEST_P(SyntaxTreeTest, MultipleDeclaratorsGrouping) {