Skip to content

Commit

Permalink
[clang-tidy] Fix DanglingHandleCheck to work in C++17 and later mode
Browse files Browse the repository at this point in the history 
Due to guaranteed copy elision, not only do some nodes get removed from the AST,
but also some existing nodes change the source locations they correspond to.
Hence, the check works slightly differently in pre-C++17 and C++17-and-later modes
in terms of what gets highlighted.

Reviewed By: PiotrZSL

Differential Revision: https://reviews.llvm.org/D158371
  • Loading branch information
@loskutov @PiotrZSL
loskutov authored and PiotrZSL committed Sep 10, 2023
1 parent 01c1156 commit f2e5000
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 49 deletions.
74 changes: 32 additions & 42 deletions clang-tools-extra/clang-tidy/bugprone/DanglingHandleCheck.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@ ast_matchers::internal::Matcher<Stmt> handleFromTemporaryValue(
// If a ternary operator returns a temporary value, then both branches hold a
// temporary value. If one of them is not a temporary then it must be copied
// into one to satisfy the type of the operator.
const auto TemporaryTernary =
conditionalOperator(hasTrueExpression(cxxBindTemporaryExpr()),
hasFalseExpression(cxxBindTemporaryExpr()));
const auto TemporaryTernary = conditionalOperator(
hasTrueExpression(ignoringParenImpCasts(cxxBindTemporaryExpr())),
hasFalseExpression(ignoringParenImpCasts(cxxBindTemporaryExpr())));

return handleFrom(IsAHandle, anyOf(cxxBindTemporaryExpr(), TemporaryTernary));
}
Expand Down Expand Up @@ -103,26 +103,17 @@ void DanglingHandleCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
void DanglingHandleCheck::registerMatchersForVariables(MatchFinder *Finder) {
const auto ConvertedHandle = handleFromTemporaryValue(IsAHandle);

// Find 'Handle foo(ReturnsAValue());'
// Find 'Handle foo(ReturnsAValue());', 'Handle foo = ReturnsAValue();'
Finder->addMatcher(
varDecl(hasType(hasUnqualifiedDesugaredType(
recordType(hasDeclaration(cxxRecordDecl(IsAHandle))))),
unless(parmVarDecl()),
hasInitializer(
exprWithCleanups(has(ignoringParenImpCasts(ConvertedHandle)))
exprWithCleanups(ignoringElidableConstructorCall(has(
ignoringParenImpCasts(ConvertedHandle))))
.bind("bad_stmt"))),
this);

// Find 'Handle foo = ReturnsAValue();'
Finder->addMatcher(
traverse(TK_AsIs,
varDecl(hasType(hasUnqualifiedDesugaredType(recordType(
hasDeclaration(cxxRecordDecl(IsAHandle))))),
unless(parmVarDecl()),
hasInitializer(exprWithCleanups(
has(ignoringParenImpCasts(handleFrom(
IsAHandle, ConvertedHandle))))
.bind("bad_stmt")))),
this);
// Find 'foo = ReturnsAValue(); // foo is Handle'
Finder->addMatcher(
traverse(TK_AsIs,
Expand All @@ -141,36 +132,35 @@ void DanglingHandleCheck::registerMatchersForVariables(MatchFinder *Finder) {
void DanglingHandleCheck::registerMatchersForReturn(MatchFinder *Finder) {
// Return a local.
Finder->addMatcher(
traverse(
TK_AsIs,
returnStmt(
// The AST contains two constructor calls:
// 1. Value to Handle conversion.
// 2. Handle copy construction.
// We have to match both.
has(ignoringImplicit(handleFrom(
IsAHandle,
handleFrom(IsAHandle,
declRefExpr(to(varDecl(
// Is function scope ...
hasAutomaticStorageDuration(),
// ... and it is a local array or Value.
anyOf(hasType(arrayType()),
hasType(hasUnqualifiedDesugaredType(
recordType(hasDeclaration(recordDecl(
unless(IsAHandle)))))))))))))),
// Temporary fix for false positives inside lambdas.
unless(hasAncestor(lambdaExpr())))
.bind("bad_stmt")),
traverse(TK_AsIs,
returnStmt(
// The AST contains two constructor calls:
// 1. Value to Handle conversion.
// 2. Handle copy construction (elided in C++17+).
// We have to match both.
has(ignoringImplicit(ignoringElidableConstructorCall(
ignoringImplicit(handleFrom(
IsAHandle,
declRefExpr(to(varDecl(
// Is function scope ...
hasAutomaticStorageDuration(),
// ... and it is a local array or Value.
anyOf(hasType(arrayType()),
hasType(hasUnqualifiedDesugaredType(
recordType(hasDeclaration(recordDecl(
unless(IsAHandle))))))))))))))),
// Temporary fix for false positives inside lambdas.
unless(hasAncestor(lambdaExpr())))
.bind("bad_stmt")),
this);

// Return a temporary.
Finder->addMatcher(
traverse(
TK_AsIs,
returnStmt(has(exprWithCleanups(has(ignoringParenImpCasts(handleFrom(
IsAHandle, handleFromTemporaryValue(IsAHandle)))))))
.bind("bad_stmt")),
traverse(TK_AsIs,
returnStmt(has(exprWithCleanups(ignoringElidableConstructorCall(
has(ignoringParenImpCasts(
handleFromTemporaryValue(IsAHandle)))))))
.bind("bad_stmt")),
this);
}

Expand Down
23 changes: 16 additions & 7 deletions clang-tools-extra/test/clang-tidy/checkers/bugprone/dangling-handle.cpp
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
// RUN: %check_clang_tidy -std=c++11,c++14 %s bugprone-dangling-handle %t -- \
// RUN: %check_clang_tidy -std=c++11,c++14 -check-suffix=,CXX14 %s bugprone-dangling-handle %t -- \
// RUN: -config="{CheckOptions: \
// RUN: {bugprone-dangling-handle.HandleClasses: \
// RUN: 'std::basic_string_view; ::llvm::StringRef;'}}"

// RUN: %check_clang_tidy -std=c++17-or-later -check-suffix=,CXX17 %s bugprone-dangling-handle %t -- \
// RUN: -config="{CheckOptions: \
// RUN: {bugprone-dangling-handle.HandleClasses: \
// RUN: 'std::basic_string_view; ::llvm::StringRef;'}}"
// FIXME: Fix the checker to work in C++17 or later mode.

namespace std {

Expand Down Expand Up @@ -84,27 +88,32 @@ std::string ReturnsAString();

void Positives() {
std::string_view view1 = std::string();
// CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives its value [bugprone-dangling-handle]
// CHECK-MESSAGES-CXX14: [[@LINE-1]]:20: warning: std::basic_string_view outlives its value [bugprone-dangling-handle]
// CHECK-MESSAGES-CXX17: [[@LINE-2]]:28: warning: std::basic_string_view outlives its value [bugprone-dangling-handle]

std::string_view view_2 = ReturnsAString();
// CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives
// CHECK-MESSAGES-CXX14: [[@LINE-1]]:20: warning: std::basic_string_view outlives its value [bugprone-dangling-handle]
// CHECK-MESSAGES-CXX17: [[@LINE-2]]:29: warning: std::basic_string_view outlives its value [bugprone-dangling-handle]

view1 = std::string();
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: std::basic_string_view outlives

const std::string& str_ref = "";
std::string_view view3 = true ? "A" : str_ref;
// CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives
// CHECK-MESSAGES-CXX14: [[@LINE-1]]:20: warning: std::basic_string_view outlives
// CHECK-MESSAGES-CXX17: [[@LINE-2]]:28: warning: std::basic_string_view outlives
view3 = true ? "A" : str_ref;
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: std::basic_string_view outlives

std::string_view view4(ReturnsAString());
// CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives
// CHECK-MESSAGES-CXX14: [[@LINE-1]]:20: warning: std::basic_string_view outlives
// CHECK-MESSAGES-CXX17: [[@LINE-2]]:26: warning: std::basic_string_view outlives
}

void OtherTypes() {
llvm::StringRef ref = std::string();
// CHECK-MESSAGES: [[@LINE-1]]:19: warning: llvm::StringRef outlives its value
// CHECK-MESSAGES-CXX14: [[@LINE-1]]:19: warning: llvm::StringRef outlives its value
// CHECK-MESSAGES-CXX17: [[@LINE-2]]:25: warning: llvm::StringRef outlives its value
}

const char static_array[] = "A";
Expand Down

0 comments on commit f2e5000

Please sign in to comment.