[LibTooling] Relax Transformer to allow rewriting macro expansions

Summary:
Currently, Transformer rejects any changes to source locations inside macro
expansions. This change relaxes that constraint to allow rewrites when the
entirety of the expansion is replaced, since that can be mapped to replacing the
entirety of the expansion range in the file source.  This change makes
Transformer consistent with the handling of edit ranges in `clang::edit::Commit`
(which is used, for example, for applying `FixItHint`s from diagnostics).

Reviewers: ilya-biryukov

Subscribers: gribozavr, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D64518

llvm-svn: 366473

clang/lib/Tooling/Refactoring/Transformer.cpp

@@ -36,37 +36,6 @@ using llvm::StringError;
 
 using MatchResult = MatchFinder::MatchResult;
 
-// Did the text at this location originate in a macro definition (aka. body)?
-// For example,
-//
-//   #define NESTED(x) x
-//   #define MACRO(y) { int y  = NESTED(3); }
-//   if (true) MACRO(foo)
-//
-// The if statement expands to
-//
-//   if (true) { int foo = 3; }
-//                   ^     ^
-//                   Loc1  Loc2
-//
-// For SourceManager SM, SM.isMacroArgExpansion(Loc1) and
-// SM.isMacroArgExpansion(Loc2) are both true, but isOriginMacroBody(sm, Loc1)
-// is false, because "foo" originated in the source file (as an argument to a
-// macro), whereas isOriginMacroBody(SM, Loc2) is true, because "3" originated
-// in the definition of MACRO.
-static bool isOriginMacroBody(const clang::SourceManager &SM,
-                              clang::SourceLocation Loc) {
-  while (Loc.isMacroID()) {
-    if (SM.isMacroBodyExpansion(Loc))
-      return true;
-    // Otherwise, it must be in an argument, so we continue searching up the
-    // invocation stack. getImmediateMacroCallerLoc() gives the location of the
-    // argument text, inside the call text.
-    Loc = SM.getImmediateMacroCallerLoc(Loc);
-  }
-  return false;
-}
-
 Expected<SmallVector<tooling::detail::Transformation, 1>>
 tooling::detail::translateEdits(const MatchResult &Result,
                                 llvm::ArrayRef<ASTEdit> Edits) {
@@ -75,14 +44,17 @@ tooling::detail::translateEdits(const MatchResult &Result,
     Expected<CharSourceRange> Range = Edit.TargetRange(Result);
     if (!Range)
       return Range.takeError();
-    if (Range->isInvalid() ||
-        isOriginMacroBody(*Result.SourceManager, Range->getBegin()))
+    llvm::Optional<CharSourceRange> EditRange =
+        getRangeForEdit(*Range, *Result.Context);
+    // FIXME: let user specify whether to treat this case as an error or ignore
+    // it as is currently done.
+    if (!EditRange)
       return SmallVector<Transformation, 0>();
     auto Replacement = Edit.Replacement(Result);
     if (!Replacement)
       return Replacement.takeError();
     tooling::detail::Transformation T;
-    T.Range = *Range;
+    T.Range = *EditRange;
     T.Replacement = std::move(*Replacement);
     Transformations.push_back(std::move(T));
   }

clang/unittests/Tooling/TransformerTest.cpp

@@ -137,7 +137,7 @@ protected:
   TransformerTest() { appendToHeader(KHeaderContents); }
 };
 
-// Given string s, change strlen($s.c_str()) to $s.size().
+// Given string s, change strlen($s.c_str()) to REPLACED.
 static RewriteRule ruleStrlenSize() {
   StringRef StringExpr = "strexpr";
   auto StringType = namedDecl(hasAnyName("::basic_string", "::string"));
@@ -163,17 +163,6 @@ TEST_F(TransformerTest, NoMatch) {
   testRule(ruleStrlenSize(), Input, Input);
 }
 
-// Tests that expressions in macro arguments are rewritten (when applicable).
-TEST_F(TransformerTest, StrlenSizeMacro) {
-  std::string Input = R"cc(
-#define ID(e) e
-    int f(string s) { return ID(strlen(s.c_str())); })cc";
-  std::string Expected = R"cc(
-#define ID(e) e
-    int f(string s) { return ID(REPLACED); })cc";
-  testRule(ruleStrlenSize(), Input, Expected);
-}
-
 // Tests replacing an expression.
 TEST_F(TransformerTest, Flag) {
   StringRef Flag = "flag";
@@ -619,23 +608,114 @@ TEST_F(TransformerTest, ErrorOccurredMatchSkipped) {
   EXPECT_EQ(ErrorCount, 0);
 }
 
-TEST_F(TransformerTest, NoTransformationInMacro) {
+// Transformation of macro source text when the change encompasses the entirety
+// of the expanded text.
+TEST_F(TransformerTest, SimpleMacro) {
   std::string Input = R"cc(
-#define MACRO(str) strlen((str).c_str())
-    int f(string s) { return MACRO(s); })cc";
-  testRule(ruleStrlenSize(), Input, Input);
+#define ZERO 0
+    int f(string s) { return ZERO; }
+  )cc";
+  std::string Expected = R"cc(
+#define ZERO 0
+    int f(string s) { return 999; }
+  )cc";
+
+  StringRef zero = "zero";
+  RewriteRule R = makeRule(integerLiteral(equals(0)).bind(zero),
+                           change(node(zero), text("999")));
+  testRule(R, Input, Expected);
 }
 
-// This test handles the corner case where a macro called within another macro
-// expands to matching code, but the matched code is an argument to the nested
-// macro.  A simple check of isMacroArgExpansion() vs. isMacroBodyExpansion()
-// will get this wrong, and transform the code. This test verifies that no such
-// transformation occurs.
-TEST_F(TransformerTest, NoTransformationInNestedMacro) {
+// Transformation of macro source text when the change encompasses the entirety
+// of the expanded text, for the case of function-style macros.
+TEST_F(TransformerTest, FunctionMacro) {
+  std::string Input = R"cc(
+#define MACRO(str) strlen((str).c_str())
+    int f(string s) { return MACRO(s); }
+  )cc";
+  std::string Expected = R"cc(
+#define MACRO(str) strlen((str).c_str())
+    int f(string s) { return REPLACED; }
+  )cc";
+
+  testRule(ruleStrlenSize(), Input, Expected);
+}
+
+// Tests that expressions in macro arguments can be rewritten.
+TEST_F(TransformerTest, MacroArg) {
+  std::string Input = R"cc(
+#define PLUS(e) e + 1
+    int f(string s) { return PLUS(strlen(s.c_str())); }
+  )cc";
+  std::string Expected = R"cc(
+#define PLUS(e) e + 1
+    int f(string s) { return PLUS(REPLACED); }
+  )cc";
+
+  testRule(ruleStrlenSize(), Input, Expected);
+}
+
+// Tests that expressions in macro arguments can be rewritten, even when the
+// macro call occurs inside another macro's definition.
+TEST_F(TransformerTest, MacroArgInMacroDef) {
   std::string Input = R"cc(
 #define NESTED(e) e
 #define MACRO(str) NESTED(strlen((str).c_str()))
-    int f(string s) { return MACRO(s); })cc";
+    int f(string s) { return MACRO(s); }
+  )cc";
+  std::string Expected = R"cc(
+#define NESTED(e) e
+#define MACRO(str) NESTED(strlen((str).c_str()))
+    int f(string s) { return REPLACED; }
+  )cc";
+
+  testRule(ruleStrlenSize(), Input, Expected);
+}
+
+// Tests the corner case of the identity macro, specifically that it is
+// discarded in the rewrite rather than preserved (like PLUS is preserved in the
+// previous test).  This behavior is of dubious value (and marked with a FIXME
+// in the code), but we test it to verify (and demonstrate) how this case is
+// handled.
+TEST_F(TransformerTest, IdentityMacro) {
+  std::string Input = R"cc(
+#define ID(e) e
+    int f(string s) { return ID(strlen(s.c_str())); }
+  )cc";
+  std::string Expected = R"cc(
+#define ID(e) e
+    int f(string s) { return REPLACED; }
+  )cc";
+
+  testRule(ruleStrlenSize(), Input, Expected);
+}
+
+// No rewrite is applied when the changed text does not encompass the entirety
+// of the expanded text. That is, the edit would have to be applied to the
+// macro's definition to succeed and editing the expansion point would not
+// suffice.
+TEST_F(TransformerTest, NoPartialRewriteOMacroExpansion) {
+  std::string Input = R"cc(
+#define ZERO_PLUS 0 + 3
+    int f(string s) { return ZERO_PLUS; })cc";
+
+  StringRef zero = "zero";
+  RewriteRule R = makeRule(integerLiteral(equals(0)).bind(zero),
+                           change(node(zero), text("0")));
+  testRule(R, Input, Input);
+}
+
+// This test handles the corner case where a macro expands within another macro
+// to matching code, but that code is an argument to the nested macro call.  A
+// simple check of isMacroArgExpansion() vs. isMacroBodyExpansion() will get
+// this wrong, and transform the code.
+TEST_F(TransformerTest, NoPartialRewriteOfMacroExpansionForMacroArgs) {
+  std::string Input = R"cc(
+#define NESTED(e) e
+#define MACRO(str) 1 + NESTED(strlen((str).c_str()))
+    int f(string s) { return MACRO(s); }
+  )cc";
+
   testRule(ruleStrlenSize(), Input, Input);
 }
 } // namespace