[libTooling] Fix bug in Stencil handling of macro ranges

[ymandel]

Summary

Currently, an attempt to rewrite source code inside a macro expansion succeeds, but results in empty text, rather than failing with an error. This patch restructures to the code to explicitly validate ranges before attempting to edit them.

[gribozavr2]

The only functional change that I see in this patch is in clang/lib/Tooling/Transformer/Stencil.cpp. However, I don't understand how that change in the (deprecated) selection() stencil can affect other stencils.

clang/unittests/Tooling/StencilTest.cpp

375

"foo(MACRO);" will fail parsing at the top level, it should be within a function, I think.

[ymandel]

In D72274#1826477, @gribozavr2 wrote:

The only functional change that I see in this patch is in clang/lib/Tooling/Transformer/Stencil.cpp. However, I don't understand how that change in the (deprecated) selection() stencil can affect other stencils.

Good point. Actually, selection and text are still used internally. See, for example, lines 301-303 in Stencil.cpp. We deprecated their use for clients.

clang/unittests/Tooling/StencilTest.cpp

375

matchStmt *(below) handles wrapping the snippet in a function.

[gribozavr2]

In D72274#1826614, @ymandel wrote:

In D72274#1826477, @gribozavr2 wrote:

The only functional change that I see in this patch is in clang/lib/Tooling/Transformer/Stencil.cpp. However, I don't understand how that change in the (deprecated) selection() stencil can affect other stencils.

Good point. Actually, selection and text are still used internally. See, for example, lines 301-303 in Stencil.cpp. We deprecated their use for clients.

Oh, that explains everything.

[ymandel]

Thanks for the review!

In D72274#1826634, @gribozavr2 wrote:

In D72274#1826614, @ymandel wrote:

In D72274#1826477, @gribozavr2 wrote:

The only functional change that I see in this patch is in clang/lib/Tooling/Transformer/Stencil.cpp. However, I don't understand how that change in the (deprecated) selection() stencil can affect other stencils.

Good point. Actually, selection and text are still used internally. See, for example, lines 301-303 in Stencil.cpp. We deprecated their use for clients.

Oh, that explains everything.

I have to clean up that implementation file...

clang/include/clang/Tooling/Transformer/SourceCode.h

 ///     = getText(S2, Context) = "{ return 3; }"
 /// \endcode
 template <typename T>
 StringRef getExtendedText(const T &Node, tok::TokenKind Next,
                           ASTContext &Context) {
   return getText(getExtendedRange(Node, Next, Context), Context);
 }
 
-// Attempts to resolve the given range to one that can be edited by a rewrite;
-// generally, one that starts and ends within a particular file. It supports
-// a limited set of cases involving source locations in macro expansions.
+/// Determines whether \p Range is one that can be edited by a rewrite;
+/// generally, one that starts and ends within a particular file.
+llvm::Error validateEditRange(const CharSourceRange &Range,
+                              const SourceManager &SM);
+
+/// Attempts to resolve the given range to one that can be edited by a rewrite;
+/// generally, one that starts and ends within a particular file. It supports a
+/// limited set of cases involving source locations in macro expansions. If a
+/// value is returned, it satisfies \c validateEditRange.
 llvm::Optional<CharSourceRange>
 getRangeForEdit(const CharSourceRange &EditRange, const SourceManager &SM,
                 const LangOptions &LangOpts);
-
 inline llvm::Optional<CharSourceRange>
 getRangeForEdit(const CharSourceRange &EditRange, const ASTContext &Context) {
   return getRangeForEdit(EditRange, Context.getSourceManager(),
                          Context.getLangOpts());
 }
 } // namespace tooling
 } // namespace clang
 #endif // LLVM_CLANG_TOOLING_TRANSFORMER_SOURCE_CODE_H

clang/lib/Tooling/Transformer/SourceCode.cpp

 //===--- SourceCode.cpp - Source code manipulation routines -----*- 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 provides functions that simplify extraction of source code.
 //
 //===----------------------------------------------------------------------===//
 #include "clang/Tooling/Transformer/SourceCode.h"
 #include "clang/Lex/Lexer.h"
+#include "llvm/Support/Errc.h"
 
 using namespace clang;
 
+using llvm::errc;
+using llvm::StringError;
+
 StringRef clang::tooling::getText(CharSourceRange Range,
                                   const ASTContext &Context) {
   return Lexer::getSourceText(Range, Context.getSourceManager(),
                               Context.getLangOpts());
 }
 
 CharSourceRange clang::tooling::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());
 }
 
+llvm::Error clang::tooling::validateEditRange(const CharSourceRange &Range,
+                                              const SourceManager &SM) {
+  if (Range.isInvalid())
+    return llvm::make_error<StringError>(errc::invalid_argument,
+                                         "Invalid range");
+
+  if (Range.getBegin().isMacroID() || Range.getEnd().isMacroID())
+    return llvm::make_error<StringError>(
+        errc::invalid_argument, "Range starts or ends in a macro expansion");
+
+  if (SM.isInSystemHeader(Range.getBegin()) ||
+      SM.isInSystemHeader(Range.getEnd()))
+    return llvm::make_error<StringError>(errc::invalid_argument,
+                                         "Range is in system header");
+
+  std::pair<FileID, unsigned> BeginInfo = SM.getDecomposedLoc(Range.getBegin());
+  std::pair<FileID, unsigned> EndInfo = SM.getDecomposedLoc(Range.getEnd());
+  if (BeginInfo.first != EndInfo.first)
+    return llvm::make_error<StringError>(
+        errc::invalid_argument, "Range begins and ends in different files");
+
+  if (BeginInfo.second > EndInfo.second)
+    return llvm::make_error<StringError>(
+        errc::invalid_argument, "Range's begin is past its end");
+
+  return llvm::Error::success();
+}
+
 llvm::Optional<CharSourceRange>
 clang::tooling::getRangeForEdit(const CharSourceRange &EditRange,
                                 const SourceManager &SM,
                                 const LangOptions &LangOpts) {
   // FIXME: makeFileCharRange() has the disadvantage of stripping off "identity"
   // macros. For example, if we're looking to rewrite the int literal 3 to 6,
   // and we have the following definition:
   //    #define DO_NOTHING(x) x
   // then
   //    foo(DO_NOTHING(3))
   // will be rewritten to
   //    foo(6)
   // rather than the arguably better
   //    foo(DO_NOTHING(6))
   // Decide whether the current behavior is desirable and modify if not.
   CharSourceRange Range = Lexer::makeFileCharRange(EditRange, SM, LangOpts);
-  if (Range.isInvalid())
-    return None;
-
-  if (Range.getBegin().isMacroID() || Range.getEnd().isMacroID())
-    return None;
-  if (SM.isInSystemHeader(Range.getBegin()) ||
-      SM.isInSystemHeader(Range.getEnd()))
-    return None;
-
-  std::pair<FileID, unsigned> BeginInfo = SM.getDecomposedLoc(Range.getBegin());
-  std::pair<FileID, unsigned> EndInfo = SM.getDecomposedLoc(Range.getEnd());
-  if (BeginInfo.first != EndInfo.first ||
-      BeginInfo.second > EndInfo.second)
-    return None;
-
+  bool IsInvalid = llvm::errorToBool(validateEditRange(Range, SM));
+  if (IsInvalid)
+    return llvm::None;
   return Range;
+
 }

clang/lib/Tooling/Transformer/Stencil.cpp

Error evalData(const UnaryOperationData &Data,
   return Error::success();
 }
 
 Error evalData(const SelectorData &Data, const MatchFinder::MatchResult &Match,
                std::string *Result) {
   auto Range = Data.Selector(Match);
   if (!Range)
     return Range.takeError();
+  if (auto Err = tooling::validateEditRange(*Range, *Match.SourceManager))
+    return Err;
   *Result += tooling::getText(*Range, *Match.Context);
   return Error::success();
 }
 
 Error evalData(const AccessData &Data, const MatchFinder::MatchResult &Match,
                std::string *Result) {
   const auto *E = Match.Nodes.getNodeAs<Expr>(Data.BaseId);
   if (E == nullptr)

clang/unittests/Tooling/SourceCodeTest.cpp

 //===- unittest/Tooling/SourceCodeTest.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 "clang/Tooling/Transformer/SourceCode.h"
 #include "TestVisitor.h"
 #include "clang/Basic/Diagnostic.h"
 #include "llvm/Testing/Support/Annotations.h"
+#include "llvm/Testing/Support/Error.h"
 #include "llvm/Testing/Support/SupportHelpers.h"
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
 using namespace clang;
 
+using llvm::Failed;
+using llvm::Succeeded;
 using llvm::ValueIs;
 using tooling::getExtendedText;
 using tooling::getRangeForEdit;
 using tooling::getText;
+using tooling::validateEditRange;
 
 namespace {
 
 struct IntLitVisitor : TestVisitor<IntLitVisitor> {
   bool VisitIntegerLiteral(IntegerLiteral *Expr) {
     OnIntLit(Expr, Context);
     return true;
   }
)cpp");
   Visitor.OnIntLit = [&Code](IntegerLiteral *Expr, ASTContext *Context) {
     auto Range = CharSourceRange::getTokenRange(Expr->getSourceRange());
     EXPECT_THAT(getRangeForEdit(Range, *Context),
                 ValueIs(AsRange(Context->getSourceManager(), Code.range("r"))));
   };
   Visitor.runOver(Code.code());
 }
 
+TEST(SourceCodeTest, EditRangeWithMacroExpansionsIsValid) {
+  // The call expression, whose range we are extracting, includes two macro
+  // expansions.
+  llvm::StringRef Code = R"cpp(
+#define M(a) a * 13
+int foo(int x, int y);
+int a = foo(M(1), M(2));
+)cpp";
+
+  CallsVisitor Visitor;
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    auto Range = CharSourceRange::getTokenRange(CE->getSourceRange());
+    EXPECT_THAT_ERROR(validateEditRange(Range, Context->getSourceManager()),
+                      Succeeded());
+  };
+  Visitor.runOver(Code);
+}
+
+TEST(SourceCodeTest, SpellingRangeOfMacroArgIsValid) {
+  llvm::StringRef Code = R"cpp(
+#define FOO(a) a + 7.0;
+int a = FOO(10);
+)cpp";
+
+  IntLitVisitor Visitor;
+  Visitor.OnIntLit = [](IntegerLiteral *Expr, ASTContext *Context) {
+    SourceLocation ArgLoc =
+        Context->getSourceManager().getSpellingLoc(Expr->getBeginLoc());
+    // The integer literal is a single token.
+    auto ArgRange = CharSourceRange::getTokenRange(ArgLoc);
+    EXPECT_THAT_ERROR(validateEditRange(ArgRange, Context->getSourceManager()),
+                      Succeeded());
+  };
+  Visitor.runOver(Code);
+}
+
+TEST(SourceCodeTest, InvalidEditRangeIsInvalid) {
+  llvm::StringRef Code = "int c = 10;";
+
+  // We use the visitor just to get a valid context.
+  IntLitVisitor Visitor;
+  Visitor.OnIntLit = [](IntegerLiteral *, ASTContext *Context) {
+    CharSourceRange Invalid;
+    EXPECT_THAT_ERROR(validateEditRange(Invalid, Context->getSourceManager()),
+                      Failed());
+  };
+  Visitor.runOver(Code);
+}
+
+TEST(SourceCodeTest, InvertedEditRangeIsInvalid) {
+  llvm::StringRef Code = R"cpp(
+int foo(int x);
+int a = foo(2);
+)cpp";
+
+  CallsVisitor Visitor;
+  Visitor.OnCall = [](CallExpr *Expr, ASTContext *Context) {
+    auto InvertedRange = CharSourceRange::getTokenRange(
+        SourceRange(Expr->getEndLoc(), Expr->getBeginLoc()));
+    EXPECT_THAT_ERROR(
+        validateEditRange(InvertedRange, Context->getSourceManager()),
+        Failed());
+  };
+  Visitor.runOver(Code);
+}
+
+TEST(SourceCodeTest, MacroArgIsInvalid) {
+  llvm::StringRef Code = R"cpp(
+#define FOO(a) a + 7.0;
+int a = FOO(10);
+)cpp";
+
+  IntLitVisitor Visitor;
+  Visitor.OnIntLit = [](IntegerLiteral *Expr, ASTContext *Context) {
+    auto Range = CharSourceRange::getTokenRange(Expr->getSourceRange());
+    EXPECT_THAT_ERROR(validateEditRange(Range, Context->getSourceManager()),
+                      Failed());
+  };
+  Visitor.runOver(Code);
+}
+
+TEST(SourceCodeTest, EditWholeMacroExpansionIsInvalid) {
+  llvm::StringRef Code = R"cpp(
+#define FOO 10
+int a = FOO;
+)cpp";
+
+  IntLitVisitor Visitor;
+  Visitor.OnIntLit = [](IntegerLiteral *Expr, ASTContext *Context) {
+    auto Range = CharSourceRange::getTokenRange(Expr->getSourceRange());
+    EXPECT_THAT_ERROR(validateEditRange(Range, Context->getSourceManager()),
+                      Failed());
+
+  };
+  Visitor.runOver(Code);
+}
+
+TEST(SourceCodeTest, EditPartialMacroExpansionIsInvalid) {
+  llvm::StringRef Code = R"cpp(
+#define BAR 10+
+int c = BAR 3.0;
+)cpp";
+
+  IntLitVisitor Visitor;
+  Visitor.OnIntLit = [](IntegerLiteral *Expr, ASTContext *Context) {
+    auto Range = CharSourceRange::getTokenRange(Expr->getSourceRange());
+    EXPECT_THAT_ERROR(validateEditRange(Range, Context->getSourceManager()),
+                      Failed());
+  };
+  Visitor.runOver(Code);
+}
 } // end anonymous namespace

clang/unittests/Tooling/StencilTest.cpp

TEST_F(StencilTest, RunOp) {
   StringRef Id = "id";
   auto SimpleFn = [Id](const MatchResult &R) {
     return std::string(R.Nodes.getNodeAs<Stmt>(Id) != nullptr ? "Bound"
                                                               : "Unbound");
   };
   testExpr(Id, "3;", run(SimpleFn), "Bound");
 }
 
+TEST_F(StencilTest, CatOfInvalidRangeFails) {
+  StringRef Snippet = R"cpp(
+#define MACRO (3.77)
+  double foo(double d);
+  foo(MACRO);)cpp";
+
+  auto StmtMatch =
+      matchStmt(Snippet, callExpr(callee(functionDecl(hasName("foo"))),
+                                  argumentCountIs(1),
+                                  hasArgument(0, expr().bind("arg"))));
+  ASSERT_TRUE(StmtMatch);
+  Stencil S = cat(node("arg"));
+  EXPECT_THAT_EXPECTED(S->eval(StmtMatch->Result), Failed<StringError>());
+}
+
 TEST(StencilToStringTest, RawTextOp) {
   auto S = cat("foo bar baz");
   StringRef Expected = R"("foo bar baz")";
   EXPECT_EQ(S->toString(), Expected);
 }
 
 TEST(StencilToStringTest, RawTextOpEscaping) {
   auto S = cat("foo \"bar\" baz\\n");