[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 ↗

(On Diff #187957)

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 ↗

(On Diff #187957)

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

77 ↗

(On Diff #187957)

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 ↗

(On Diff #187957)

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 ↗

(On Diff #187957)

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 ↗

(On Diff #187957)

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 ↗

(On Diff #187957)

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

clang/lib/Tooling/FixIt.cpp

52 ↗

(On Diff #187957)

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 ↗

(On Diff #187957)

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 ↗

(On Diff #187957)

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 ↗

(On Diff #187957)

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 ↗

(On Diff #187957)

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 ↗

(On Diff #187957)

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 ↗

(On Diff #187957)

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

73 ↗

(On Diff #187957)

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!

cfe/trunk/include/clang/Tooling/FixIt.h

 //    tooling::fixit::createReplacement(*B, *A);
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLING_FIXIT_H
 #define LLVM_CLANG_TOOLING_FIXIT_H
 
 #include "clang/AST/ASTContext.h"
+#include "clang/Basic/TokenKinds.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; }
+/// Returns the token CharSourceRange corresponding to \p 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);
+/// Returns the CharSourceRange of the token at Location \p Loc.
+inline CharSourceRange getSourceRange(const SourceLocation &Loc) {
+  return CharSourceRange::getTokenRange(Loc, Loc);
 }
 
-/// Returns the SourceRange of an given Node. \p Node is typically a
+/// Returns the CharSourceRange 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());
 }
+
+/// Extends \p Range to include the token \p Next, if it immediately follows the
+/// end of the range. Otherwise, returns \p Range unchanged.
+CharSourceRange maybeExtendRange(CharSourceRange Range, tok::TokenKind Next,
+                                 ASTContext &Context);
 } // end namespace internal
 
-// Returns a textual representation of \p Node.
+/// 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 node, extended to include \p Next, if
+/// it immediately follows \p Node. Otherwise, returns the normal range of \p
+/// Node.  See comments on `getExtendedText()` for examples.
+template <typename T>
+CharSourceRange getExtendedRange(const T &Node, tok::TokenKind Next,
+                                 ASTContext &Context) {
+  return internal::maybeExtendRange(internal::getSourceRange(Node), Next,
+                                    Context);
+}
+
+/// Returns the source text of the node, extended to include \p Next, if it
+/// immediately follows the node. Otherwise, returns the text of just \p Node.
+///
+/// For example, given statements S1 and S2 below:
+/// \code
+///   {
+///     // S1:
+///     if (!x) return foo();
+///     // S2:
+///     if (!x) { return 3; }
+//    }
+/// \endcode
+/// then
+/// \code
+///   getText(S1, Context) = "if (!x) return foo()"
+///   getExtendedText(S1, tok::TokenKind::semi, Context)
+///     = "if (!x) return foo();"
+///   getExtendedText(*S1.getThen(), tok::TokenKind::semi, Context)
+///     = "return foo();"
+///   getExtendedText(*S2.getThen(), tok::TokenKind::semi, Context)
+///     = getText(S2, Context) = "{ return 3; }"
+/// \endcode
+template <typename T>
+StringRef getExtendedText(const T &Node, tok::TokenKind Next,
+                          ASTContext &Context) {
+  return internal::getText(getExtendedRange(Node, Next, 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>

cfe/trunk/lib/Tooling/FixIt.cpp

 #include "clang/Tooling/FixIt.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
+
+CharSourceRange maybeExtendRange(CharSourceRange Range, tok::TokenKind Next,
+                                 ASTContext &Context) {
+  Optional<Token> Tok = Lexer::findNextToken(
+      Range.getEnd(), Context.getSourceManager(), Context.getLangOpts());
+  if (!Tok || !Tok->is(Next))
+    return Range;
+  return CharSourceRange::getTokenRange(Range.getBegin(), Tok->getLocation());
+}
+} // namespace internal
 
 } // end namespace fixit
 } // end namespace tooling
 } // end namespace clang

cfe/trunk/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::getExtendedText;
 using tooling::fixit::createRemoval;
 using tooling::fixit::createReplacement;
 
 namespace {
 
 struct CallsVisitor : TestVisitor<CallsVisitor> {
   bool VisitCallExpr(CallExpr *Expr) {
     OnCall(Expr, 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, getExtendedText) {
+  CallsVisitor Visitor;
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    EXPECT_EQ("foo(x, y);",
+              getExtendedText(*CE, tok::TokenKind::semi, *Context));
+
+    Expr *P0 = CE->getArg(0);
+    Expr *P1 = CE->getArg(1);
+    EXPECT_EQ("x", getExtendedText(*P0, tok::TokenKind::semi, *Context));
+    EXPECT_EQ("x,", getExtendedText(*P0, tok::TokenKind::comma, *Context));
+    EXPECT_EQ("y", getExtendedText(*P1, tok::TokenKind::semi, *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("int foo(int x, int y) { if (true) return 3 + foo(x, y); }");
+  Visitor.runOver("void foo(int x, int y) { for (foo(x, y);;) ++x; }");
+  Visitor.runOver(
+      "bool foo(int x, int y) { for (;foo(x, y);) x = 1; return true; }");
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    EXPECT_EQ("foo()", getExtendedText(*CE, tok::TokenKind::semi, *Context));
+  };
+  Visitor.runOver("bool foo() { if (foo()) return true; return false; }");
+  Visitor.runOver("void foo() { int x; for (;; foo()) ++x; }");
+  Visitor.runOver("int foo() { return foo() + 3; }");
+}
+
 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());