[libTooling] Change Transformer's `cat` to handle some cases of text in macros.

[ymandel]

Summary

Currently, cat validates range selections before extracting the corresponding
source text. However, this means that any range inside a macro is rejected as an
error. This patch changes the implementation to first try to map the range to
something reasonable. This makes the behavior consistent with handling of ranges
used for selecting portions of the source to edit.

[tdl-g]

LGTM. I found the change description confusing, since it talks about the selection() stencil but the code is all about the cat() stencil. I realize (now) that the former is deprecated in favor of the latter. But the change description is still confusing.

[ymandel]

In D82126#2103772, @tdl-g wrote:

LGTM. I found the change description confusing, since it talks about the selection() stencil but the code is all about the cat() stencil. I realize (now) that the former is deprecated in favor of the latter. But the change description is still confusing.

Whoops, I'd forgotten that we'd deprecated that combinator. Thanks for catching that. I've updated the description correspondingly.

[ymandel]

Tests show that this breaks the test for the clang tidy abseil-string-find-str-contains. Curious if this is a desirable change in behavior (in which case I'll update your test file) or the wrong behavior:

https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp#L246-L252

void no_macros() {
   std::string s;
-  COMPARE_MACRO(s.find("a"), std::string::npos);
-  FIND_MACRO(s, "a") == std::string::npos;
-  FIND_COMPARE_MACRO(s, "a", std::string::npos);
+  !absl::StrContains(s, "a");
+  !absl::StrContains(s, "a");
+  !absl::StrContains(s, "a");
 }

[tdl-g]

Interesting, in all three of those cases, it's reasonable to replace the entire expression, thus eliminating the macro. None of those "tear" the macro; if we had a case like

#define FOO(a,b,c,d) ((a).find(b) == std::string::npos ? (c) : (d))
FOO("helo", "x", 5, 6);

I guess in that case we'd want to suppress an edit change, since it would have to modify the macro to make the change. But I guess all the existing test cases are actually safe to convert with the macro.

Do you want to remove the existing macro cases and add the "tearing" one above to confirm that it doesn't propose an edit?

In D82126#2103934, @ymandel wrote:

Tests show that this breaks the test for the clang tidy abseil-string-find-str-contains. Curious if this is a desirable change in behavior (in which case I'll update your test file) or the wrong behavior:

https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp#L246-L252

void no_macros() {
   std::string s;
-  COMPARE_MACRO(s.find("a"), std::string::npos);
-  FIND_MACRO(s, "a") == std::string::npos;
-  FIND_COMPARE_MACRO(s, "a", std::string::npos);
+  !absl::StrContains(s, "a");
+  !absl::StrContains(s, "a");
+  !absl::StrContains(s, "a");
 }

[ymandel]

Fixed clang tidy lit test.

[ymandel]

In D82126#2104088, @tdl-g wrote:

Interesting, in all three of those cases, it's reasonable to replace the entire expression, thus eliminating the macro. None of those "tear" the macro; if we had a case like

#define FOO(a,b,c,d) ((a).find(b) == std::string::npos ? (c) : (d))
FOO("helo", "x", 5, 6);

I guess in that case we'd want to suppress an edit change, since it would have to modify the macro to make the change. But I guess all the existing test cases are actually safe to convert with the macro.

Do you want to remove the existing macro cases and add the "tearing" one above to confirm that it doesn't propose an edit?

Done.

In D82126#2103934, @ymandel wrote:

Tests show that this breaks the test for the clang tidy abseil-string-find-str-contains. Curious if this is a desirable change in behavior (in which case I'll update your test file) or the wrong behavior:

https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp#L246-L252

void no_macros() {
   std::string s;
-  COMPARE_MACRO(s.find("a"), std::string::npos);
-  FIND_MACRO(s, "a") == std::string::npos;
-  FIND_COMPARE_MACRO(s, "a", std::string::npos);
+  !absl::StrContains(s, "a");
+  !absl::StrContains(s, "a");
+  !absl::StrContains(s, "a");
 }

clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp

void no_char_param_tests() {
 
   std::string_view ssv;
   ssv.find('c') == std::string_view::npos;
 
   absl::string_view asv;
   asv.find('c') == absl::string_view::npos;
 }
 
-#define COMPARE_MACRO(x, y) ((x) == (y))
-#define FIND_MACRO(x, y) ((x).find(y))
-#define FIND_COMPARE_MACRO(x, y, z) ((x).find(y) == (z))
-
-// Confirms that it does not match when a macro is involved.
-void no_macros() {
-  std::string s;
-  COMPARE_MACRO(s.find("a"), std::string::npos);
-  FIND_MACRO(s, "a") == std::string::npos;
-  FIND_COMPARE_MACRO(s, "a", std::string::npos);
+#define FOO(a, b, c, d) ((a).find(b) == std::string::npos ? (c) : (d))
+
+// Confirms that it does not match when a macro would be "torn" by the fix.
+void no_tearing_macros() {
+  std::string h = "helo";
+  FOO(h, "x", 5, 6);
 }
 
 // Confirms that it does not match when the pos parameter is non-zero.
 void no_nonzero_pos() {
   std::string ss;
   ss.find("a", 1) == std::string::npos;
 
   std::string_view ssv;

clang/lib/Tooling/Transformer/Stencil.cpp

 //===--- Stencil.cpp - Stencil implementation -------------------*- 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/Tooling/Transformer/Stencil.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/Expr.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/SourceLocation.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Tooling/Transformer/SourceCode.h"
 #include "clang/Tooling/Transformer/SourceCodeBuilders.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/Errc.h"
 #include <atomic>
 #include <memory>
return llvm::make_error<StringError>(
         errc::invalid_argument,
         "Could not construct expression source from ID: " + Data.Id);
   *Result += *Source;
   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))
+  auto RawRange = Data.Selector(Match);
+  if (!RawRange)
+    return RawRange.takeError();
+  CharSourceRange Range = Lexer::makeFileCharRange(
+      *RawRange, *Match.SourceManager, Match.Context->getLangOpts());
+  if (auto Err = tooling::validateEditRange(Range, *Match.SourceManager))
     return Err;
-  *Result += tooling::getText(*Range, *Match.Context);
+  *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)
     return llvm::make_error<StringError>(errc::invalid_argument,

clang/unittests/Tooling/StencilTest.cpp

 //===- unittest/Tooling/StencilTest.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/Stencil.h"
+#include "clang/AST/ASTTypeTraits.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Tooling/FixIt.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
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, CatOfMacroRangeSucceeds) {
+  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), HasValue("MACRO"));
+}
+
+TEST_F(StencilTest, CatOfMacroArgRangeSucceeds) {
+  StringRef Snippet = R"cpp(
+#define MACRO(a, b) a + b
+  MACRO(2, 3);)cpp";
+
+  auto StmtMatch =
+      matchStmt(Snippet, binaryOperator(hasRHS(expr().bind("rhs"))));
+  ASSERT_TRUE(StmtMatch);
+  Stencil S = cat(node("rhs"));
+  EXPECT_THAT_EXPECTED(S->eval(StmtMatch->Result), HasValue("3"));
+}
+
+TEST_F(StencilTest, CatOfMacroArgSubRangeSucceeds) {
+  StringRef Snippet = R"cpp(
+#define MACRO(a, b) a + b
+  int foo(int);
+  MACRO(2, foo(3));)cpp";
+
+  auto StmtMatch = matchStmt(
+      Snippet, binaryOperator(hasRHS(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), HasValue("3"));
+}
+
 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"))),