[LibTooling] Relax Transformer to allow rewriting macro expansions

[ymandel]

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 FixItHints from diagnostics).

[ilya-biryukov]

This clearly increases the utility of the library, but also seems to add corner cases that the library won't handle (see the comment about unittests for an example).
WDYT about those? Are they important, should we support producing warnings in those cases to let the users know things might get broken?

clang/lib/Tooling/Refactoring/Transformer.cpp

76 ↗

(On Diff #209020)

Could we add unit tests for this particular function?

Interesting cases ([[ and ]] mark the start and end of a range):

#define FOO(a) a+a;
#define BAR 10+

// change part of a macro argument
int a = FOO([[10]] + 10);

// change the whole macro expansion
int b = [[FOO(10+10)]];

// Try to change 10 inside 'BAR', but not '+'.
// Should this fail? Should we give a warning?
int c = BAR 3; 

// Try changing the lhs (10) of a binary expr, but not rhs.
// Is that allowed? Should we give a warning?
int d = FOO(10);

99 ↗

(On Diff #209020)

naming NIT: use BeginInfo

clang/unittests/Tooling/TransformerTest.cpp

625 ↗

(On Diff #209020)

could we change to something other than 0 to make sure it's not the macro being expanded?

[ymandel]

In D64518#1585917, @ilya-biryukov wrote:

This clearly increases the utility of the library, but also seems to add corner cases that the library won't handle (see the comment about unittests for an example).
WDYT about those? Are they important, should we support producing warnings in those cases to let the users know things might get broken?

That's a really good question. The code explicitly chooses to treat these failures like "this didn't match" rather than "this matched and now there's an error". That reflects the split that some users will want to know while others will want the system to always skip such matches, just like it skips non-matching expressions.

This seems like a good candidate for configuration -- the user could then choose which mode to run in. But, I'm also open to just reporting these conditions as errors. It's already in a context that returns Expected, so its no trouble; it's just a matter of choosing what we think is "correct".

clang/lib/Tooling/Refactoring/Transformer.cpp

76 ↗

(On Diff #209020)

Sure. What do you think of exposing this function in clang/include/clang/Tooling/Refactoring/SourceCode.h and testing it from there?

[ymandel]

tweaks in response to comments.

[ilya-biryukov]

In D64518#1588092, @ymandel wrote:

This seems like a good candidate for configuration -- the user could then choose which mode to run in. But, I'm also open to just reporting these conditions as errors. It's already in a context that returns Expected, so its no trouble; it's just a matter of choosing what we think is "correct".

WRT to returning Expected vs Optional. Either seems fine and in the spirit of the library, depending on whether we want to produce more detailed errors. However, if we choose Optional let's stick to it, as practice shows switching from Optional to Expected correctly is almost impossible, as that requires a lot of attention to make sure all clients consume the errors (and given that it's an error case, tests often don't catch unconsumed errors).
I would personally go with Optional here (meaning the client code would have to say something generic like could not map from macro expansion to source code). But up to you, not a strong preference.

WRT to which cases we choose to handle, I'd start with a minimal number of supported examples (covering full macro expansion, or inside a single argument) and gradually add other cases as we find use-cases. What are your thoughts on that?

clang/lib/Tooling/Refactoring/Transformer.cpp

76 ↗

(On Diff #209020)

Sounds reasonable. Was thinking of a better name, maybe something like getRangeForEdit()?
Would also suggest to accept SourceManager and LangOptions instead of MatchResult to narrow down the requirements on the clients.

[ymandel]

In D64518#1589768, @ilya-biryukov wrote:

In D64518#1588092, @ymandel wrote:

This seems like a good candidate for configuration -- the user could then choose which mode to run in. But, I'm also open to just reporting these conditions as errors. It's already in a context that returns Expected, so its no trouble; it's just a matter of choosing what we think is "correct".

WRT to returning Expected vs Optional. Either seems fine and in the spirit of the library, depending on whether we want to produce more detailed errors. However, if we choose Optional let's stick to it, as practice shows switching from Optional to Expected correctly is almost impossible, as that requires a lot of attention to make sure all clients consume the errors (and given that it's an error case, tests often don't catch unconsumed errors).
I would personally go with Optional here (meaning the client code would have to say something generic like could not map from macro expansion to source code). But up to you, not a strong preference.

I think we might be talking about different things here. I meant that the *calling* function, translateEdits, returns Expected, so it would be easy to return an error when makeValidRange returns None. I agree that makeValidRange (or whatever we choose to call it) should stick with Optional for simplicity (with the generic interpretation of None being "could not map from macro expansion to source code").

WRT to which cases we choose to handle, I'd start with a minimal number of supported examples (covering full macro expansion, or inside a single argument) and gradually add other cases as we find use-cases. What are your thoughts on that?

I assume you mean which cases makeValidRange should handle (successfully)? If so, that sounds good. But, what do you think about how to handle failures of makeValidRange -- ignore them silently (which is what we're doing now) or treat them as errors?

[ilya-biryukov]

In D64518#1589857, @ymandel wrote:

In D64518#1589768, @ilya-biryukov wrote:

In D64518#1588092, @ymandel wrote:

This seems like a good candidate for configuration -- the user could then choose which mode to run in. But, I'm also open to just reporting these conditions as errors. It's already in a context that returns Expected, so its no trouble; it's just a matter of choosing what we think is "correct".

WRT to returning Expected vs Optional. Either seems fine and in the spirit of the library, depending on whether we want to produce more detailed errors. However, if we choose Optional let's stick to it, as practice shows switching from Optional to Expected correctly is almost impossible, as that requires a lot of attention to make sure all clients consume the errors (and given that it's an error case, tests often don't catch unconsumed errors).
I would personally go with Optional here (meaning the client code would have to say something generic like could not map from macro expansion to source code). But up to you, not a strong preference.

I think we might be talking about different things here. I meant that the *calling* function, translateEdits, returns Expected, so it would be easy to return an error when makeValidRange returns None. I agree that makeValidRange (or whatever we choose to call it) should stick with Optional for simplicity (with the generic interpretation of None being "could not map from macro expansion to source code").

Ah, great, we're on the same page then. LGTM!

WRT to which cases we choose to handle, I'd start with a minimal number of supported examples (covering full macro expansion, or inside a single argument) and gradually add other cases as we find use-cases. What are your thoughts on that?

I assume you mean which cases makeValidRange should handle (successfully)? If so, that sounds good.

Yes, exactly.

But, what do you think about how to handle failures of makeValidRange -- ignore them silently (which is what we're doing now) or treat them as errors?

I think it depends on the use-case, e.g. if we try to produce a clang-tidy fix for some warning and can't produce a fix because makeValidRange failed, then not showing the fix (i.e. failing silently) seems fine.
OTOH, if we're building a refactoring tool that should find an replace all occurrences of a matcher and apply the transformation, failing silently is probably not a good option, we should possibly list the locations where the transformation failed (so that users can do manual changes to complete the refactoring).

[ymandel]

Moved makeValidRange to its own revision and rebased onto that.

[ymandel]

remove unneeded include

[ymandel]

clang/lib/Tooling/Refactoring/Transformer.cpp

76 ↗

(On Diff #209020)

Went with passing the ASTContext rather than the MatchResult (in the new revision D64924)

[ilya-biryukov]

LGTM

cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp

 using ast_matchers::internal::DynTypedMatcher;
 using ast_type_traits::ASTNodeKind;
 using ast_type_traits::DynTypedNode;
 using llvm::Error;
 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) {
   SmallVector<tooling::detail::Transformation, 1> Transformations;
   for (const auto &Edit : Edits) {
     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));
   }
   return Transformations;
 }
 
 ASTEdit tooling::change(RangeSelector S, TextGenerator Replacement) {
   ASTEdit E;

cfe/trunk/unittests/Tooling/TransformerTest.cpp

private:
   FileContentMappings FileContents = {{"header.h", ""}};
 };
 
 class TransformerTest : public ClangRefactoringTestBase {
 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"));
   auto R = makeRule(
       callExpr(callee(functionDecl(hasName("strlen"))),
                hasArgument(0, cxxMemberCallExpr(
                                   on(expr(hasType(isOrPointsTo(StringType)))
                                          .bind(StringExpr)),

 }
 
 // Tests that no change is applied when a match is not expected.
 TEST_F(TransformerTest, NoMatch) {
   std::string Input = "int f(string s) { return s.size(); }";
   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";
   RewriteRule Rule = makeRule(
       cxxMemberCallExpr(on(expr(hasType(cxxRecordDecl(
                                     hasName("proto::ProtoCommandLineFlag"))))
                                .bind(Flag)),
                         unless(callee(cxxMethodDecl(hasName("GetProto"))))),
TEST_F(TransformerTest, ErrorOccurredMatchSkipped) {
   T.registerMatchers(&MatchFinder);
   // The rewrite process itself fails...
   EXPECT_FALSE(rewrite(Input));
   // ... and no changes or errors are produced in the process.
   EXPECT_THAT(Changes, IsEmpty());
   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 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);
+}
+
+// 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";
-  testRule(ruleStrlenSize(), Input, Input);
+    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);
 }
 
-// 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) {
+// 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