[SyntaxTree][Synthesis] Add support for simple Leafs and test based on tree dump

[eduucaldas]

Remove unused include

[gribozavr2]

clang/include/clang/Tooling/Syntax/BuildTree.h

30–31

30–31

I think putting token kind first might be more readable at the call site -- going top-down, first the kind (general), then spelling (specific).

clang/lib/Tooling/Syntax/Synthesis.cpp

26

+assert(spelling != nullptr); ?

62

Could we make a combined function that does not require the user to make a distinction between punctuation and keywords?

We should also allow creating tokens that have a user-specified spelling, for example, identifiers and string literals.

So maybe define two functions:

// Uses the provided spelling.
syntax::Leaf *createLeaf(syntax::Arena &A, tok::TokenKind K, StringRef Spelling);

// Infers spelling if possible.
syntax::Leaf *createLeaf(syntax::Arena &A, tok::TokenKind K);

clang/unittests/Tooling/Syntax/SynthesisTest.cpp

66–73

Could you extract the part that I highlighted into a function, and deduplicate it across all tests?

[eduucaldas]

Answer inline comments

[eduucaldas]

clang/lib/Tooling/Syntax/Synthesis.cpp

62

First, thanks for the great comments!

Could we make a combined function that does not require the user to make a distinction between punctuation and keywords?

We could!
Bui I wouldn't call it createLeaf, as it would work strictly for Keywords or Punctuators.
As an alternative we could unify the createPunctuatorand createKeyword into createPunctuatorOrKeyword but I would argue that the previous two names are more readable. Effectively, we would be unifying those 2 functions because they have the same signature.

We should also allow creating tokens that have a user-specified spelling, for example, identifiers and string literals.

That exists already! It is createLeaf as is!

[gribozavr2]

clang/lib/Tooling/Syntax/Synthesis.cpp

62

but I would argue that the previous two names are more readable. Effectively, we would be unifying those 2 functions because they have the same signature.

I don't think the user cares very much about whether their token is a keyword or a punctuation. Furthermore, most callsites will have the token kind as an argument, so saying that it is a keyword or punctuation is redundant:

createKeyword(Arena, tok::kw_if); // Yeah, 'if' is a keyword.

The semantics of the combined function I'm proposing is "create a token that has one fixed spelling". The other function is "create a token where the spelling is user-provided".

[eduucaldas]

createPunctuation, createKeyword -> createLeaf

[eduucaldas]

clang/lib/Tooling/Syntax/Synthesis.cpp

62

I think the user cares about whether their token is a keyword.

It's is somewhat redundant yes, but I don't think that hurts, we are still specifying that the keyword being used is if.

I see your points, and I don't have a strong opinion so I'm updating the patch.

[eduucaldas]

fix clang-tidy warning

[gribozavr2]

clang/lib/Tooling/Syntax/Synthesis.cpp

49

createLeafLowLevel now seems to be used only once, inline it?

clang/unittests/Tooling/Syntax/SynthesisTest.cpp

46

What does 'C' stand for?

[eduucaldas]

Remove createLeafLowLevel

clang/include/clang/Tooling/Syntax/BuildTree.h

 namespace syntax {
 
 /// Build a syntax tree for the main file.
 syntax::TranslationUnit *buildSyntaxTree(Arena &A,
                                          const clang::TranslationUnitDecl &TU);
 
 // Create syntax trees from subtrees not backed by the source code.
 
-clang::syntax::Leaf *createPunctuation(clang::syntax::Arena &A,
-                                       clang::tok::TokenKind K);
+// Synthesis of Leafs
+/// Create `Leaf` from token with `Spelling` and assert it has the desired
+/// `TokenKind`.
+syntax::Leaf *createLeaf(syntax::Arena &A, tok::TokenKind K,
+                         StringRef Spelling);
+
+/// Infer the token spelling from its `TokenKind`, then create `Leaf` from
+/// this token
+syntax::Leaf *createLeaf(syntax::Arena &A, tok::TokenKind K);
+
+// Synthesis of Syntax Nodes
 clang::syntax::EmptyStatement *createEmptyStatement(clang::syntax::Arena &A);
 
 } // namespace syntax
 } // namespace clang
 #endif

clang/lib/Tooling/Syntax/Synthesis.cpp

 //===- Synthesis.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/Basic/TokenKinds.h"
 #include "clang/Tooling/Syntax/BuildTree.h"
 
 using namespace clang;
 
 /// Exposes private syntax tree APIs required to implement node synthesis.
 /// Should not be used for anything else.
-class syntax::FactoryImpl {
+class clang::syntax::FactoryImpl {
 public:
   static void setCanModify(syntax::Node *N) { N->CanModify = true; }
 
   static void prependChildLowLevel(syntax::Tree *T, syntax::Node *Child,
                                    syntax::NodeRole R) {
     T->prependChildLowLevel(Child, R);
   }
 };
 
-clang::syntax::Leaf *syntax::createPunctuation(clang::syntax::Arena &A,
-                                               clang::tok::TokenKind K) {
-  auto Tokens = A.lexBuffer(llvm::MemoryBuffer::getMemBuffer(
-                                clang::tok::getPunctuatorSpelling(K)))
-                    .second;
+syntax::Leaf *clang::syntax::createLeaf(syntax::Arena &A, tok::TokenKind K,
+                                        StringRef Spelling) {
+  auto Tokens = A.lexBuffer(llvm::MemoryBuffer::getMemBuffer(Spelling)).second;
   assert(Tokens.size() == 1);
-  assert(Tokens.front().kind() == K);
-  auto *L = new (A.getAllocator()) clang::syntax::Leaf(Tokens.begin());
-  FactoryImpl::setCanModify(L);
-  L->assertInvariants();
-  return L;
+  assert(Tokens.front().kind() == K &&
+         "spelling is not lexed into the expected kind of token");
+
+  auto *Leaf = new (A.getAllocator()) syntax::Leaf(Tokens.begin());
+  syntax::FactoryImpl::setCanModify(Leaf);
+  Leaf->assertInvariants();
+  return Leaf;
+}
+
+syntax::Leaf *clang::syntax::createLeaf(syntax::Arena &A, tok::TokenKind K) {
+  const auto *Spelling = tok::getPunctuatorSpelling(K);
+  if (!Spelling)
+    Spelling = tok::getKeywordSpelling(K);
+  assert(Spelling &&
+         "Cannot infer the spelling of the token from its token kind.");
+  return createLeaf(A, K, Spelling);
 }
 
-clang::syntax::EmptyStatement *
-syntax::createEmptyStatement(clang::syntax::Arena &A) {
-  auto *S = new (A.getAllocator()) clang::syntax::EmptyStatement;
+syntax::EmptyStatement *clang::syntax::createEmptyStatement(syntax::Arena &A) {
+  auto *S = new (A.getAllocator()) syntax::EmptyStatement;
   FactoryImpl::setCanModify(S);
-  FactoryImpl::prependChildLowLevel(S, createPunctuation(A, clang::tok::semi),
+  FactoryImpl::prependChildLowLevel(S, createLeaf(A, tok::semi),
                                     NodeRole::Unknown);
   S->assertInvariants();
   return S;
 }

clang/unittests/Tooling/Syntax/SynthesisTest.cpp

 //===- SynthesisTest.cpp --------------------------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 //
 // This file tests synthesis API for syntax trees.
 //
 //===----------------------------------------------------------------------===//
 
 #include "TreeTestBase.h"
 #include "clang/Tooling/Syntax/BuildTree.h"
+#include "gtest/gtest.h"
 
 using namespace clang;
 using namespace clang::syntax;
 
 namespace {
 
-INSTANTIATE_TEST_CASE_P(SyntaxTreeTests, SyntaxTreeTest,
+class SynthesisTest : public SyntaxTreeTest {
+protected:
+  ::testing::AssertionResult treeDumpEqual(syntax::Node *Root, StringRef Dump) {
+    if (!Root)
+      return ::testing::AssertionFailure()
+             << "Root was not built successfully.";
+
+    auto Actual = StringRef(Root->dump(Arena->getSourceManager())).trim().str();
+    auto Expected = Dump.trim().str();
+    // EXPECT_EQ shows the diff between the two strings if they are different.
+    EXPECT_EQ(Expected, Actual);
+    if (Actual != Expected) {
+      return ::testing::AssertionFailure();
+    }
+    return ::testing::AssertionSuccess();
+  }
+};
+
+INSTANTIATE_TEST_CASE_P(SynthesisTests, SynthesisTest,
                         ::testing::ValuesIn(allTestClangConfigs()), );
 
-TEST_P(SyntaxTreeTest, Leaf_Punctuation) {
+TEST_P(SynthesisTest, Leaf_Punctuation) {
+  buildTree("", GetParam());
+
+  auto *Leaf = createLeaf(*Arena, tok::comma);
+
+  EXPECT_TRUE(treeDumpEqual(Leaf, R"txt(
+',' Detached synthesized
+  )txt"));
+}
+
+TEST_P(SynthesisTest, Leaf_Keyword) {
   buildTree("", GetParam());
 
-  auto *C = syntax::createPunctuation(*Arena, tok::comma);
-  ASSERT_NE(C, nullptr);
-  EXPECT_EQ(C->getToken()->kind(), tok::comma);
-  EXPECT_TRUE(C->canModify());
-  EXPECT_FALSE(C->isOriginal());
-  EXPECT_TRUE(C->isDetached());
+  auto *Leaf = createLeaf(*Arena, tok::kw_if);
+
+  EXPECT_TRUE(treeDumpEqual(Leaf, R"txt(
+'if' Detached synthesized
+  )txt"));
+}
+
+TEST_P(SynthesisTest, Leaf_Identifier) {
+  buildTree("", GetParam());
+
+  auto *Leaf = createLeaf(*Arena, tok::identifier, "a");
+
+  EXPECT_TRUE(treeDumpEqual(Leaf, R"txt(
+'a' Detached synthesized
+  )txt"));
+}
+
+TEST_P(SynthesisTest, Leaf_Number) {
+  buildTree("", GetParam());
+
+  auto *Leaf = createLeaf(*Arena, tok::numeric_constant, "1");
+
+  EXPECT_TRUE(treeDumpEqual(Leaf, R"txt(
+'1' Detached synthesized
+  )txt"));
 }
 
-TEST_P(SyntaxTreeTest, Statement_Empty) {
+TEST_P(SynthesisTest, Statement_EmptyStatement) {
   buildTree("", GetParam());
 
-  auto *S = syntax::createEmptyStatement(*Arena);
-  ASSERT_NE(S, nullptr);
-  EXPECT_TRUE(S->canModify());
-  EXPECT_FALSE(S->isOriginal());
-  EXPECT_TRUE(S->isDetached());
+  auto *S = createEmptyStatement(*Arena);
+  EXPECT_TRUE(treeDumpEqual(S, R"txt(
+EmptyStatement Detached synthesized
+`-';' synthesized
+  )txt"));
 }
 } // namespace