[libTooling] Update Transformer's `node` combinator to include the trailing semicolon for decls.

[ymandel]

Summary

Currently, node only includes the semicolon for (some) statements. However, declarations have the same issue of (potentially) trailing semicolons, so node should behave the same for them.

[aaron.ballman]

Drive-by question from the peanut gallery, sorry if this is an ignorant one -- not all declarations have a trailing semicolon; is that handled properly? e.g., int x; has a trailing semicolon but int x, y; only has a trailing semicolon for one of the two declarations. Relatedly, in int f(int x);, the declaration of f has a trailing semicolon, but the declaration of x does not.

[ymandel]

Clarified that semicolons are only removed if present. Fixed test.

[ymandel]

In D91872#2408278, @aaron.ballman wrote:

Drive-by question from the peanut gallery, sorry if this is an ignorant one -- not all declarations have a trailing semicolon; is that handled properly? e.g., int x; has a trailing semicolon but int x, y; only has a trailing semicolon for one of the two declarations. Relatedly, in int f(int x);, the declaration of f has a trailing semicolon, but the declaration of x does not.

No, it's a good question -- the comments and the patch description should have been clearer. I've updated both. Also, I found a test that needed to be fixed (and also, hopefully, illustrates why the new behavior is preferred. The old behavior left the semicolon out of the replacement (which looks weird) because it was working around the fact that the source being removed didn't include the trailing semicolon, while the new version can just specify the replacement correctly).

Thanks!

[aaron.ballman]

In D91872#2408321, @ymandel wrote:

In D91872#2408278, @aaron.ballman wrote:

Drive-by question from the peanut gallery, sorry if this is an ignorant one -- not all declarations have a trailing semicolon; is that handled properly? e.g., int x; has a trailing semicolon but int x, y; only has a trailing semicolon for one of the two declarations. Relatedly, in int f(int x);, the declaration of f has a trailing semicolon, but the declaration of x does not.

No, it's a good question -- the comments and the patch description should have been clearer. I've updated both. Also, I found a test that needed to be fixed (and also, hopefully, illustrates why the new behavior is preferred. The old behavior left the semicolon out of the replacement (which looks weird) because it was working around the fact that the source being removed didn't include the trailing semicolon, while the new version can just specify the replacement correctly).

Thanks!

Thanks, this is more clear now!

clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp

 }
 
 // A trivial rewrite rule generator that checks config options.
 Optional<RewriteRule> noSkip(const LangOptions &LangOpts,
                              const ClangTidyCheck::OptionsView &Options) {
   if (Options.get("Skip", "false") == "true")
     return None;
   return tooling::makeRule(clang::ast_matchers::functionDecl(),
-                           change(cat("void nothing()")), cat("no message"));
+                           changeTo(cat("void nothing();")), cat("no message"));
 }
 
 class ConfigurableCheck : public TransformerClangTidyCheck {
 public:
   ConfigurableCheck(StringRef Name, ClangTidyContext *Context)
       : TransformerClangTidyCheck(noSkip, Name, Context) {}
 };
 

clang/include/clang/Tooling/Transformer/RangeSelector.h

 /// * the TokenRange [B,E'] where the token at E' spans the range [E',E).
 RangeSelector after(RangeSelector Selector);
 
 /// Selects the range between `R1` and `R2.
 inline RangeSelector between(RangeSelector R1, RangeSelector R2) {
   return enclose(after(std::move(R1)), before(std::move(R2)));
 }
 
-/// Selects a node, including trailing semicolon (for non-expression
-/// statements). \p ID is the node's binding in the match result.
+/// Selects a node, including trailing semicolon, if any (for declarations and
+/// non-expression statements). \p ID is the node's binding in the match result.
 RangeSelector node(std::string ID);
 
 /// Selects a node, including trailing semicolon (always). Useful for selecting
 /// expression statements. \p ID is the node's binding in the match result.
 RangeSelector statement(std::string ID);
 
 /// Given a \c MemberExpr, selects the member token. \p ID is the node's
 /// binding in the match result.

clang/lib/Tooling/Transformer/RangeSelector.cpp

RangeSelector transformer::after(RangeSelector Selector) {
   };
 }
 
 RangeSelector transformer::node(std::string ID) {
   return [ID](const MatchResult &Result) -> Expected<CharSourceRange> {
     Expected<DynTypedNode> Node = getNode(Result.Nodes, ID);
     if (!Node)
       return Node.takeError();
-    return Node->get<Stmt>() != nullptr && Node->get<Expr>() == nullptr
+    return (Node->get<Decl>() != nullptr ||
+            (Node->get<Stmt>() != nullptr && Node->get<Expr>() == nullptr))
                ? tooling::getExtendedRange(*Node, tok::TokenKind::semi,
                                            *Result.Context)
                : CharSourceRange::getTokenRange(Node->getSourceRange());
   };
 }
 
 RangeSelector transformer::statement(std::string ID) {
   return [ID](const MatchResult &Result) -> Expected<CharSourceRange> {

clang/unittests/Tooling/TransformerTest.cpp

void instantiate()
   TemplStruct<int> ti;
 }
 )cpp";
 
   auto MatchedField = fieldDecl(hasType(asString("int"))).bind("theField");
 
   // Changes the 'int' in 'S', but not the 'T' in 'TemplStruct':
   testRule(makeRule(traverse(TK_IgnoreUnlessSpelledInSource, MatchedField),
-                    changeTo(cat("safe_int ", name("theField")))),
+                    changeTo(cat("safe_int ", name("theField"), ";"))),
            NonTemplatesInput + TemplatesInput,
            NonTemplatesExpected + TemplatesInput);
 
   // In AsIs mode, template instantiations are modified, which is
   // often not desired:
 
   std::string IncorrectTemplatesExpected = R"cpp(
 template<typename T>
 struct TemplStruct {
   TemplStruct() {}
   ~TemplStruct() {}
 
 private:
   safe_int m_t;
 };
 
 void instantiate()
 {
   TemplStruct<int> ti;
 }
 )cpp";
 
   // Changes the 'int' in 'S', and (incorrectly) the 'T' in 'TemplStruct':
   testRule(makeRule(traverse(TK_AsIs, MatchedField),
-                    changeTo(cat("safe_int ", name("theField")))),
+                    changeTo(cat("safe_int ", name("theField"), ";"))),
 
            NonTemplatesInput + TemplatesInput,
            NonTemplatesExpected + IncorrectTemplatesExpected);
 }
 
 // Transformation of macro source text when the change encompasses the entirety
 // of the expanded text.
 TEST_F(TransformerTest, SimpleMacro) {
TEST_F(TransformerTest, MultipleFiles) {
   ASSERT_EQ(Changes[0].getFilePath(), "./input.h");
   EXPECT_THAT(Changes[0].getInsertedHeaders(), IsEmpty());
   EXPECT_THAT(Changes[0].getRemovedHeaders(), IsEmpty());
   llvm::Expected<std::string> UpdatedCode =
       clang::tooling::applyAllReplacements(Header,
                                            Changes[0].getReplacements());
   ASSERT_TRUE(static_cast<bool>(UpdatedCode))
       << "Could not update code: " << llvm::toString(UpdatedCode.takeError());
-  EXPECT_EQ(format(*UpdatedCode), format(R"cc(;)cc"));
+  EXPECT_EQ(format(*UpdatedCode), "");
 
   ASSERT_EQ(Changes[1].getFilePath(), "input.cc");
   EXPECT_THAT(Changes[1].getInsertedHeaders(), IsEmpty());
   EXPECT_THAT(Changes[1].getRemovedHeaders(), IsEmpty());
   UpdatedCode = clang::tooling::applyAllReplacements(
       Source, Changes[1].getReplacements());
   ASSERT_TRUE(static_cast<bool>(UpdatedCode))
       << "Could not update code: " << llvm::toString(UpdatedCode.takeError());
-  EXPECT_EQ(format(*UpdatedCode), format(R"cc(#include "input.h"
-                        ;)cc"));
+  EXPECT_EQ(format(*UpdatedCode), format("#include \"input.h\"\n"));
 }
 
 TEST_F(TransformerTest, AddIncludeMultipleFiles) {
   std::string Header = R"cc(void RemoveThisFunction();)cc";
   std::string Source = R"cc(#include "input.h"
                             void Foo() {RemoveThisFunction();})cc";
   Transformer T(
       makeRule(callExpr(callee(