From 3f1ab737e2199415c072fd6624a728949eb30342 Mon Sep 17 00:00:00 2001 From: Yitzhak Mandelbaum Date: Thu, 18 Jul 2019 17:44:54 +0000 Subject: [PATCH] [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 | 40 ++------- clang/unittests/Tooling/TransformerTest.cpp | 124 +++++++++++++++++++++----- 2 files changed, 108 insertions(+), 56 deletions(-) diff --git a/clang/lib/Tooling/Refactoring/Transformer.cpp b/clang/lib/Tooling/Refactoring/Transformer.cpp index 8e6fe6c..7cf8a5d 100644 --- a/clang/lib/Tooling/Refactoring/Transformer.cpp +++ b/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> tooling::detail::translateEdits(const MatchResult &Result, llvm::ArrayRef Edits) { @@ -75,14 +44,17 @@ tooling::detail::translateEdits(const MatchResult &Result, Expected Range = Edit.TargetRange(Result); if (!Range) return Range.takeError(); - if (Range->isInvalid() || - isOriginMacroBody(*Result.SourceManager, Range->getBegin())) + llvm::Optional 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(); 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)); } diff --git a/clang/unittests/Tooling/TransformerTest.cpp b/clang/unittests/Tooling/TransformerTest.cpp index 64f511b..dcf77fe 100644 --- a/clang/unittests/Tooling/TransformerTest.cpp +++ b/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 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); +} + +// 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); } -// 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, 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 -- 2.7.4