[clang] b9d2bf3 - [libTooling] Fix bug in Stencil handling of macro ranges
Yitzhak Mandelbaum via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 17 09:11:46 PST 2020
Author: Yitzhak Mandelbaum
Date: 2020-01-17T12:11:25-05:00
New Revision: b9d2bf38e86e6dd8a2f188d9a24f546aa67de8af
URL: https://github.com/llvm/llvm-project/commit/b9d2bf38e86e6dd8a2f188d9a24f546aa67de8af
DIFF: https://github.com/llvm/llvm-project/commit/b9d2bf38e86e6dd8a2f188d9a24f546aa67de8af.diff
LOG: [libTooling] Fix bug in Stencil handling of macro ranges
Summary: Currently, an attempt to rewrite source code inside a macro expansion succeeds, but results in empty text, rather than failing with an error. This patch restructures to the code to explicitly validate ranges before attempting to edit them.
Reviewers: gribozavr
Subscribers: cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D72274
Added:
Modified:
clang/include/clang/Tooling/Transformer/SourceCode.h
clang/lib/Tooling/Transformer/SourceCode.cpp
clang/lib/Tooling/Transformer/Stencil.cpp
clang/unittests/Tooling/SourceCodeTest.cpp
clang/unittests/Tooling/StencilTest.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Tooling/Transformer/SourceCode.h b/clang/include/clang/Tooling/Transformer/SourceCode.h
index bc9cc3d2a258..1b92a117f44c 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<CharSourceRange>
getRangeForEdit(const CharSourceRange &EditRange, const SourceManager &SM,
const LangOptions &LangOpts);
-
inline llvm::Optional<CharSourceRange>
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 836401d1e605..5c1f8b46fe42 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<StringError>(errc::invalid_argument,
+ "Invalid range");
+
+ if (Range.getBegin().isMacroID() || Range.getEnd().isMacroID())
+ return llvm::make_error<StringError>(
+ errc::invalid_argument, "Range starts or ends in a macro expansion");
+
+ if (SM.isInSystemHeader(Range.getBegin()) ||
+ SM.isInSystemHeader(Range.getEnd()))
+ return llvm::make_error<StringError>(errc::invalid_argument,
+ "Range is in system header");
+
+ std::pair<FileID, unsigned> BeginInfo = SM.getDecomposedLoc(Range.getBegin());
+ std::pair<FileID, unsigned> EndInfo = SM.getDecomposedLoc(Range.getEnd());
+ if (BeginInfo.first != EndInfo.first)
+ return llvm::make_error<StringError>(
+ errc::invalid_argument, "Range begins and ends in
diff erent files");
+
+ if (BeginInfo.second > EndInfo.second)
+ return llvm::make_error<StringError>(
+ errc::invalid_argument, "Range's begin is past its end");
+
+ return llvm::Error::success();
+}
+
llvm::Optional<CharSourceRange>
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<FileID, unsigned> BeginInfo = SM.getDecomposedLoc(Range.getBegin());
- std::pair<FileID, unsigned> 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 8710e3cdf60f..00d8be71f797 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 eb28d7cf27d6..9e0d2e2c4274 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 <gmock/gmock.h>
#include <gtest/gtest.h>
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 84a33e9cedee..b248a5a69de9 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<StringError>());
+}
+
TEST(StencilToStringTest, RawTextOp) {
auto S = cat("foo bar baz");
StringRef Expected = R"("foo bar baz")";
More information about the cfe-commits
mailing list