[LibTooling] Add retrieval of extended AST-node source to FixIt library

[ymandel]

Summary

Introduces variants of getText and getSourceRange that extract the source text of an AST node potentially with a trailing token.

Some of the new functions manipulate CharSourceRanges, rather than SourceRanges, because they document and dynamically enforce their type. So, this revision also updates the corresponding existing FixIt functions to manipulate CharSourceRanges. This change is not strictly necessary, but seems like the correct choice, to keep the API self-consistent.

This revision is the first in a series intended to improve the abstractions available to users for writing source-to-source transformations. A full discussion of the end goal can be found on the cfe-dev list with subject "[RFC] Easier source-to-source transformations with clang tooling".

[ilya-biryukov]

Thanks, the APIs totally make sense. And seem to fit into the other functions we have in FixIt.h, although the name of the file is somewhat misleading.
Mostly comments about naming and one comment about the necessity of using matchers from my side.

clang/include/clang/Tooling/FixIt.h

60

Do you have alternative names in mind? It would be nice to (1) not mention the SourceRange now that we return CharSourceRange, (2) change "auto" to something more descriptive.

Was thinking about getNodeRange() or getSpannedRange(), but that completely misses the "auto" part (maybe it's fine, though).
WDYT? Maybe other ideas?

73

What are other tricky cases you have in mind for the future?

77

Could you add an example of the API use here? The anticipated use-case are removing or textually replacing a node, right?

clang/lib/Tooling/FixIt.cpp

52

Do you expect this function to be on the hot path?
If so, I'd advice against using the matchers here. They do add enough overhead to be avoided in hot functions.

I guess the problem is that we can't get a hold of the parent node with using the matchers, right?
Not sure if there's an easy way out of it in that case.

[ymandel]

clang/include/clang/Tooling/FixIt.h

60

I completely agree. I went through quite a few iterations on this name and disliked this one the least. ;) I think you're right, though, that once we're choosing a different name, the "auto" part doesn't really need to be in it. I like getSpannedRange better than this. Other thoughts:

getLogicalRange
getExtendedRange
getAssociatedRange

any preference?

73

I just assumed that we'd hit more as we dig into them, but, I'm not so sure now. The two others I can think of offhand are 1) extending to include associated comments, 2) semicolons after declarations. Commas present a similar challenge (if a bit simpler) when used in a list (vs. the comma operator). Are there any other separators in C++?

At a higher level, it would be nice to align this with your work on tree transformations. That is, think of these functions as short-term shims to simulate the behavior we'll ultimately get from that new library. However, it might be premature to consider those details here.

77

Yes, that's right. Will do (on next edit, once we've resolved naming, etc.)

clang/lib/Tooling/FixIt.cpp

52

In the context of transformer, I expect that this will be called in proportion to the number of times that a match callback is invoked. so, I expect there to already be matcher use in the control flow.

Yes, I'm using the matchers almost entirely for the hasParent() functionality.

Note that we can change the order of the guards in lines 67-69 and first check for a trailing semi which I'd guess is cheaper than calling the matcher. In that case, matching will only happen for expressions followed by semicolons.

Alternatively, I think I could restructure the uses of this function to *provide* the parent node. In that case, callers can decide what makes the most sense performance-wise for getting the parent.

[kimgr]

clang/include/clang/Tooling/FixIt.h

73

Peanut gallery comment on this:

The two others I can think of offhand are

1. extending to include associated comments,
2. semicolons after declarations.

Commas present a similar challenge (if a bit simpler) when used in a list (vs. the comma operator).
Are there any other separators in C++?

Would it make sense to let callers choose what level of expansion they want with a flag mask? Somehow I think that makes it easier to name the function, too, e.g.:

StringRef getExpandedRange(const Stmt &S, ASTContext &Context, ExpansionFlags Flags);

[ymandel]

clang/include/clang/Tooling/FixIt.h

73

Yes, I think that's a good idea. I even like the name except that it will likely be confused with Expansion locs. Maybe getExtendedRange?

[kimgr]

clang/include/clang/Tooling/FixIt.h

73

Extended sounds good to me too!

[ymandel]

This update significantly simplifies the change. It removes any "smarts", instead allowing the user to request the extension of the source range to include the next specified token (if present). This turns out to be a more basic and useful function for the needs of the upcoming changes.

[ymandel]

clang/include/clang/Tooling/FixIt.h

73

I went with "getExtended..." but ended up drastically simplifying the function, since the "smarts" turned out to be not smart enough. Fundamentally, the caller needs to know when to look for a trailing token (and which one).

clang/lib/Tooling/FixIt.cpp

52

Removed, so no longer relevant.

[ymandel]

Remove unneeded includes.

[ilya-biryukov]

Ah, had some comments and forgot to send them out before I went on vacation :-(

clang/include/clang/Tooling/FixIt.h

60

getSpannedRange and getAssociatedRange would be my favorites.
Both seem to leave no room for interpretation.

73

Would it be fair to characterize those as "the text range that you needs to be removed when removing a node"?
Trying to come up with a comment that does not rely on the intuition, since that might differ from one reader to the other.

Maybe move this comment to getSourceRangeAuto and in this function's comment simply mention that we return the text for the node covered with getSourceRangeAuto?

At a higher level, it would be nice to align this with your work on tree transformations. That is, think of these functions as short-term shims to simulate the behavior we'll ultimately get from that new library. However, it might be premature to consider those details here.

Agree, we can figure it out as soon as the syntax trees are in a good enough shape.

[ilya-biryukov]

Ignore my previous comment, I'll take another look

[ilya-biryukov]

LGTM.
getExtendedRange is a really good choice!

clang/include/clang/Tooling/FixIt.h

 
 #include "clang/AST/ASTContext.h"
 
 namespace clang {
 namespace tooling {
 namespace fixit {
 
 namespace internal {
-StringRef getText(SourceRange Range, const ASTContext &Context);
+StringRef getText(CharSourceRange Range, const ASTContext &Context);
 
 /// Returns the SourceRange of a SourceRange. This identity function is
 ///        used by the following template abstractions.
-inline SourceRange getSourceRange(const SourceRange &Range) { return Range; }
+inline CharSourceRange getSourceRange(const SourceRange &Range) {
+  return CharSourceRange::getTokenRange(Range);
+}
 
 /// Returns the SourceRange of the token at Location \p Loc.
-inline SourceRange getSourceRange(const SourceLocation &Loc) {
-  return SourceRange(Loc);
+inline CharSourceRange getSourceRange(const SourceLocation &Loc) {
+  return CharSourceRange::getTokenRange(Loc, Loc);
 }
 
 /// Returns the SourceRange of an given Node. \p Node is typically a
 ///        'Stmt', 'Expr' or a 'Decl'.
-template <typename T> SourceRange getSourceRange(const T &Node) {
-  return Node.getSourceRange();
+template <typename T> CharSourceRange getSourceRange(const T &Node) {
+  return CharSourceRange::getTokenRange(Node.getSourceRange());
 }
 } // end namespace internal
 
 // Returns a textual representation of \p Node.
 template <typename T>
 StringRef getText(const T &Node, const ASTContext &Context) {
   return internal::getText(internal::getSourceRange(Node), Context);
 }
 
+// Returns the source range spanning the statement and any trailing semicolon
+// that belongs with that statement.
+//
+// N.B. The API of this function is still evolving and might change in the
+// future to include more associated text (like comments).
+CharSourceRange getSourceRangeAuto(const Stmt &S, ASTContext &Context);
+
+CharSourceRange getSourceRangeAuto(const ast_type_traits::DynTypedNode &Node,
+                                   ASTContext &Context);
+// Catch all for any nodes that aren't DynTypedNode or derived from Stmt.
+template <typename T, typename = typename std::enable_if<
+                          (!std::is_base_of<Stmt, T>::value)>::type>
+CharSourceRange getSourceRangeAuto(const T &Node, ASTContext &Context) {
+  return internal::getSourceRange(Node);
+}
+
+// Gets the source text of the node, taking into account the node's type and
+// context. In contrast with \p getText(), this function selects a source range
+// "automatically", extracting text that a reader might intuitively associate
+// with a node.  Currently, only specialized for \p clang::Stmt, where it will
+// include any associated trailing semicolon.
+template <typename T>
+StringRef getTextAuto(const T &Node, ASTContext &Context) {
+  return internal::getText(getSourceRangeAuto(Node, Context), Context);
+}
+
 // Returns a FixItHint to remove \p Node.
 // TODO: Add support for related syntactical elements (i.e. comments, ...).
 template <typename T> FixItHint createRemoval(const T &Node) {
   return FixItHint::CreateRemoval(internal::getSourceRange(Node));
 }
 
 // Returns a FixItHint to replace \p Destination by \p Source.
 template <typename D, typename S>

clang/lib/Tooling/FixIt.cpp

 //===--- FixIt.cpp - FixIt Hint utilities -----------------------*- 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
 //
 //===----------------------------------------------------------------------===//
 //
 // This file contains implementations of utitilies to ease source code rewriting
 // by providing helper functions related to FixItHint.
 //
 //===----------------------------------------------------------------------===//
 #include "clang/Tooling/FixIt.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Lex/Lexer.h"
 
 namespace clang {
 namespace tooling {
 namespace fixit {
 
 namespace internal {
-StringRef getText(SourceRange Range, const ASTContext &Context) {
-  return Lexer::getSourceText(CharSourceRange::getTokenRange(Range),
-                              Context.getSourceManager(),
+StringRef getText(CharSourceRange Range, const ASTContext &Context) {
+  return Lexer::getSourceText(Range, Context.getSourceManager(),
                               Context.getLangOpts());
 }
 } // end namespace internal
 
+// For a given range, returns the lexed token immediately after the range if
+// and only if it's a semicolon.
+static Optional<Token> getTrailingSemi(SourceLocation EndLoc,
+                                       const ASTContext &Context) {
+  if (Optional<Token> Next = Lexer::findNextToken(
+          EndLoc, Context.getSourceManager(), Context.getLangOpts())) {
+    return Next->is(clang::tok::TokenKind::semi) ? Next : None;
+  }
+  return None;
+}
+
+// Determines whether `S` is a strictly a statement. Clang's class hierchy
+// implicitly categorizes all expressions as statements. This function
+// distinguishs expressions that appear in a context for which they are
+// first-class statements.  In essence, this excludes expressions that are
+// either part of another expression, the condition of a statement or a
+// for-loop's init or increment.
+static bool isStrictStatement(const Stmt &S, ASTContext &Context) {
+  using namespace ast_matchers;
+
+  if (!isa<Expr>(S))
+    return true;
+
+  auto NotCondition = unless(hasCondition(equalsNode(&S)));
+  auto Standalone =
+      stmt(hasParent(stmt(anyOf(compoundStmt(), ifStmt(NotCondition),
+                                whileStmt(NotCondition), doStmt(NotCondition),
+                                switchStmt(NotCondition), switchCase(),
+                                forStmt(unless(hasLoopInit(equalsNode(&S))),
+                                        unless(hasCondition(equalsNode(&S))),
+                                        unless(hasIncrement(equalsNode(&S)))),
+                                labelStmt()))))
+          .bind("stmt");
+  return !match(Standalone, S, Context).empty();
+}
+
+CharSourceRange getSourceRangeAuto(const Stmt &S, ASTContext &Context) {
+  // Only exlude non-statement expressions.
+  if (!isa<Expr>(S) || isStrictStatement(S, Context)) {
+    // TODO: exclude case where last token is a right brace?
+    if (auto Tok = getTrailingSemi(S.getEndLoc(), Context))
+      return CharSourceRange::getTokenRange(S.getBeginLoc(),
+                                            Tok->getLocation());
+  }
+  return CharSourceRange::getTokenRange(S.getSourceRange());
+}
+
+CharSourceRange getSourceRangeAuto(const ast_type_traits::DynTypedNode &Node,
+                                   ASTContext &Context) {
+  if (const auto *S = Node.get<Stmt>())
+    return getSourceRangeAuto(*S, Context);
+  return CharSourceRange::getTokenRange(Node.getSourceRange());
+}
+
 } // end namespace fixit
 } // end namespace tooling
 } // end namespace clang

clang/unittests/Tooling/FixItTest.cpp

 //===- unittest/Tooling/FixitTest.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
 //
 //===----------------------------------------------------------------------===//
 
 #include "TestVisitor.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Tooling/FixIt.h"
 
 using namespace clang;
 
 using tooling::fixit::getText;
+using tooling::fixit::getTextAuto;
 using tooling::fixit::createRemoval;
 using tooling::fixit::createReplacement;
 
 namespace {
 
 struct CallsVisitor : TestVisitor<CallsVisitor> {
   bool VisitCallExpr(CallExpr *Expr) {
     OnCall(Expr, Context);
     return true;
   }
 
   std::function<void(CallExpr *, ASTContext *Context)> OnCall;
 };
 
+struct IfVisitor : TestVisitor<IfVisitor> {
+  bool VisitIfStmt(IfStmt* S) {
+    OnIfStmt(S, Context);
+    return true;
+  }
+
+  std::function<void(IfStmt *, ASTContext *Context)> OnIfStmt;
+};
+
+struct VarDeclVisitor : TestVisitor<VarDeclVisitor> {
+  bool VisitVarDecl(VarDecl* Decl) {
+    OnVarDecl(Decl, Context);
+    return true;
+  }
+
+  std::function<void(VarDecl *, ASTContext *Context)> OnVarDecl;
+};
+
 std::string LocationToString(SourceLocation Loc, ASTContext *Context) {
   return Loc.printToString(Context->getSourceManager());
 }
 
 TEST(FixItTest, getText) {
   CallsVisitor Visitor;
 
   Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
     Expr *P1 = CE->getArg(1);
     EXPECT_EQ("x", getText(*P0, *Context));
     EXPECT_EQ("y", getText(*P1, *Context));
   };
   Visitor.runOver("#define FOO(x, y) (void)x; (void)y; foo(x, y);\n"
                   "void foo(int x, int y) { FOO(x,y) }");
 }
 
+TEST(FixItTest, getTextAuto) {
+  CallsVisitor Visitor;
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    EXPECT_EQ("foo(x, y);", getTextAuto(*CE, *Context));
+
+    Expr *P0 = CE->getArg(0);
+    Expr *P1 = CE->getArg(1);
+    EXPECT_EQ("x", getTextAuto(*P0, *Context));
+    EXPECT_EQ("y", getTextAuto(*P1, *Context));
+  };
+  Visitor.runOver("void foo(int x, int y) { foo(x, y); }");
+  Visitor.runOver("void foo(int x, int y) { if (true) foo(x, y); }");
+  Visitor.runOver("void foo(int x, int y) { switch(x) foo(x, y); }");
+  Visitor.runOver("void foo(int x, int y) { switch(x) case 3: foo(x, y); }");
+  Visitor.runOver("void foo(int x, int y) { switch(x) default: foo(x, y); }");
+  Visitor.runOver("void foo(int x, int y) { while (true) foo(x, y); }");
+  Visitor.runOver("void foo(int x, int y) { do foo(x, y); while (true); }");
+  Visitor.runOver("void foo(int x, int y) { for (;;) foo(x, y); }");
+  Visitor.runOver("void foo(int x, int y) { bar: foo(x, y); }");
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    EXPECT_EQ("foo()", getTextAuto(*CE, *Context));
+  };
+  Visitor.runOver("int foo() { return foo(); }");
+  Visitor.runOver("int foo() { 3 + foo(); return 0; }");
+  Visitor.runOver("bool foo() { if (foo()) true; return true; }");
+  Visitor.runOver("bool foo() { switch(foo()) true; return true; }");
+  Visitor.runOver("bool foo() { while (foo()) true; return true; }");
+  Visitor.runOver("bool foo() { do true; while (foo()); return true; }");
+  Visitor.runOver("void foo() { for (foo();;) true; }");
+  Visitor.runOver("bool foo() { for (;foo();) true; return true; }");
+  Visitor.runOver("void foo() { for (;; foo()) true; }");
+}
+
+TEST(FixItTest, getTextAutoNonStatement) {
+  VarDeclVisitor Visitor;
+  Visitor.OnVarDecl = [](VarDecl *D, ASTContext *Context) {
+    EXPECT_EQ(getText(*D, *Context), getTextAuto(*D, *Context));
+  };
+  Visitor.runOver("int foo() { int x = 3; return x; }");
+}
+
+TEST(FixItTest, getTextAutoNonExprStatement) {
+  IfVisitor Visitor;
+  Visitor.OnIfStmt = [](IfStmt *S, ASTContext *Context) {
+    EXPECT_EQ(getText(*S, *Context), getTextAuto(*S, *Context));
+  };
+  Visitor.runOver("int foo() { int x = 3; return x; }");
+}
+
 TEST(FixItTest, createRemoval) {
   CallsVisitor Visitor;
 
   Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
     FixItHint Hint = createRemoval(*CE);
     EXPECT_EQ("foo(x, y)", getText(Hint.RemoveRange.getAsRange(), *Context));
     EXPECT_TRUE(Hint.InsertFromRange.isInvalid());
     EXPECT_TRUE(Hint.CodeToInsert.empty());