diff --git a/clang/include/clang/Tooling/Transformer/SourceCode.h b/clang/include/clang/Tooling/Transformer/SourceCode.h index bc9cc3d2a25809..1b92a117f44c00 100644 --- a/clang/include/clang/Tooling/Transformer/SourceCode.h +++ b/clang/include/clang/Tooling/Transformer/SourceCode.h @@ -73,13 +73,18 @@ StringRef getExtendedText(const T &Node, tok::TokenKind Next, 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 getRangeForEdit(const CharSourceRange &EditRange, const SourceManager &SM, const LangOptions &LangOpts); - inline llvm::Optional getRangeForEdit(const CharSourceRange &EditRange, const ASTContext &Context) { return getRangeForEdit(EditRange, Context.getSourceManager(), diff --git a/clang/lib/Tooling/Transformer/SourceCode.cpp b/clang/lib/Tooling/Transformer/SourceCode.cpp index 836401d1e605c2..5c1f8b46fe423a 100644 --- a/clang/lib/Tooling/Transformer/SourceCode.cpp +++ b/clang/lib/Tooling/Transformer/SourceCode.cpp @@ -11,9 +11,13 @@ //===----------------------------------------------------------------------===// #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(), @@ -30,6 +34,34 @@ CharSourceRange clang::tooling::maybeExtendRange(CharSourceRange 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(errc::invalid_argument, + "Invalid range"); + + if (Range.getBegin().isMacroID() || Range.getEnd().isMacroID()) + return llvm::make_error( + errc::invalid_argument, "Range starts or ends in a macro expansion"); + + if (SM.isInSystemHeader(Range.getBegin()) || + SM.isInSystemHeader(Range.getEnd())) + return llvm::make_error(errc::invalid_argument, + "Range is in system header"); + + std::pair BeginInfo = SM.getDecomposedLoc(Range.getBegin()); + std::pair EndInfo = SM.getDecomposedLoc(Range.getEnd()); + if (BeginInfo.first != EndInfo.first) + return llvm::make_error( + errc::invalid_argument, "Range begins and ends in different files"); + + if (BeginInfo.second > EndInfo.second) + return llvm::make_error( + errc::invalid_argument, "Range's begin is past its end"); + + return llvm::Error::success(); +} + llvm::Optional clang::tooling::getRangeForEdit(const CharSourceRange &EditRange, const SourceManager &SM, @@ -46,20 +78,9 @@ clang::tooling::getRangeForEdit(const CharSourceRange &EditRange, // 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 BeginInfo = SM.getDecomposedLoc(Range.getBegin()); - std::pair 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; + } diff --git a/clang/lib/Tooling/Transformer/Stencil.cpp b/clang/lib/Tooling/Transformer/Stencil.cpp index 8710e3cdf60f32..00d8be71f79721 100644 --- a/clang/lib/Tooling/Transformer/Stencil.cpp +++ b/clang/lib/Tooling/Transformer/Stencil.cpp @@ -230,6 +230,8 @@ Error evalData(const SelectorData &Data, const MatchFinder::MatchResult &Match, 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(); } diff --git a/clang/unittests/Tooling/SourceCodeTest.cpp b/clang/unittests/Tooling/SourceCodeTest.cpp index eb28d7cf27d607..9e0d2e2c4274ea 100644 --- a/clang/unittests/Tooling/SourceCodeTest.cpp +++ b/clang/unittests/Tooling/SourceCodeTest.cpp @@ -10,16 +10,20 @@ #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 #include 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 { @@ -200,4 +204,116 @@ int a = FOO($r[[10]] + 10.0); 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 diff --git a/clang/unittests/Tooling/StencilTest.cpp b/clang/unittests/Tooling/StencilTest.cpp index 84a33e9cedeedc..b248a5a69de9f9 100644 --- a/clang/unittests/Tooling/StencilTest.cpp +++ b/clang/unittests/Tooling/StencilTest.cpp @@ -368,6 +368,21 @@ TEST_F(StencilTest, RunOp) { 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()); +} + TEST(StencilToStringTest, RawTextOp) { auto S = cat("foo bar baz"); StringRef Expected = R"("foo bar baz")";