5de443a4d3
This change enhances the `bugprone-unhandled-self-assignment` checker by adding an additional matcher that generalizes the copy-and-swap idiom pattern detection. # What Changed Added a new matcher that checks for: - An instance of the current class being created in operator= (regardless of constructor arguments) - That instance being passed to a `swap` function call # Problem Solved This fix reduces false positives in PMR-like scenarios where "extended" constructors are used (typically taking an additional allocator argument). The checker now properly recognizes copy-and-swap implementations that use extended copy/move constructors instead of flagging them as unhandled self-assignment cases. Fixes #146324 --------- Co-authored-by: Baranov Victor <bar.victor.2002@gmail.com>
133 lines
5.9 KiB
C++
133 lines
5.9 KiB
C++
//===--- UnhandledSelfAssignmentCheck.cpp - clang-tidy --------------------===//
|
|
//
|
|
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
|
|
// See https://llvm.org/LICENSE.txt for license information.
|
|
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
|
|
//
|
|
//===----------------------------------------------------------------------===//
|
|
|
|
#include "UnhandledSelfAssignmentCheck.h"
|
|
#include "clang/AST/ASTContext.h"
|
|
#include "clang/ASTMatchers/ASTMatchFinder.h"
|
|
|
|
using namespace clang::ast_matchers;
|
|
|
|
namespace clang::tidy::bugprone {
|
|
|
|
UnhandledSelfAssignmentCheck::UnhandledSelfAssignmentCheck(
|
|
StringRef Name, ClangTidyContext *Context)
|
|
: ClangTidyCheck(Name, Context),
|
|
WarnOnlyIfThisHasSuspiciousField(
|
|
Options.get("WarnOnlyIfThisHasSuspiciousField", true)) {}
|
|
|
|
void UnhandledSelfAssignmentCheck::storeOptions(
|
|
ClangTidyOptions::OptionMap &Opts) {
|
|
Options.store(Opts, "WarnOnlyIfThisHasSuspiciousField",
|
|
WarnOnlyIfThisHasSuspiciousField);
|
|
}
|
|
|
|
void UnhandledSelfAssignmentCheck::registerMatchers(MatchFinder *Finder) {
|
|
// We don't care about deleted, default or implicit operator implementations.
|
|
const auto IsUserDefined = cxxMethodDecl(
|
|
isDefinition(), unless(anyOf(isDeleted(), isImplicit(), isDefaulted())));
|
|
|
|
// We don't need to worry when a copy assignment operator gets the other
|
|
// object by value.
|
|
const auto HasReferenceParam =
|
|
cxxMethodDecl(hasParameter(0, parmVarDecl(hasType(referenceType()))));
|
|
|
|
// Self-check: Code compares something with 'this' pointer. We don't check
|
|
// whether it is actually the parameter what we compare.
|
|
const auto HasNoSelfCheck = cxxMethodDecl(unless(hasDescendant(
|
|
binaryOperation(hasAnyOperatorName("==", "!="),
|
|
hasEitherOperand(ignoringParenCasts(cxxThisExpr()))))));
|
|
|
|
// Both copy-and-swap and copy-and-move method creates a copy first and
|
|
// assign it to 'this' with swap or move.
|
|
// In the non-template case, we can search for the copy constructor call.
|
|
const auto HasNonTemplateSelfCopy = cxxMethodDecl(
|
|
ofClass(cxxRecordDecl(unless(hasAncestor(classTemplateDecl())))),
|
|
traverse(TK_AsIs,
|
|
hasDescendant(cxxConstructExpr(hasDeclaration(cxxConstructorDecl(
|
|
isCopyConstructor(), ofClass(equalsBoundNode("class"))))))));
|
|
|
|
// In the template case, we need to handle two separate cases: 1) a local
|
|
// variable is created with the copy, 2) copy is created only as a temporary
|
|
// object.
|
|
const auto HasTemplateSelfCopy = cxxMethodDecl(
|
|
ofClass(cxxRecordDecl(hasAncestor(classTemplateDecl()))),
|
|
anyOf(hasDescendant(
|
|
varDecl(hasType(cxxRecordDecl(equalsBoundNode("class"))),
|
|
hasDescendant(parenListExpr()))),
|
|
hasDescendant(cxxUnresolvedConstructExpr(hasDescendant(declRefExpr(
|
|
hasType(cxxRecordDecl(equalsBoundNode("class")))))))));
|
|
|
|
// If inside the copy assignment operator another assignment operator is
|
|
// called on 'this' we assume that self-check might be handled inside
|
|
// this nested operator.
|
|
const auto HasNoNestedSelfAssign =
|
|
cxxMethodDecl(unless(hasDescendant(cxxMemberCallExpr(callee(cxxMethodDecl(
|
|
hasName("operator="), ofClass(equalsBoundNode("class"))))))));
|
|
|
|
// Checking that some kind of constructor is called and followed by a `swap`:
|
|
// T& operator=(const T& other) {
|
|
// T tmp{this->internal_data(), some, other, args};
|
|
// swap(tmp);
|
|
// return *this;
|
|
// }
|
|
const auto HasCopyAndSwap = cxxMethodDecl(
|
|
ofClass(cxxRecordDecl()),
|
|
hasBody(compoundStmt(
|
|
hasDescendant(
|
|
varDecl(hasType(cxxRecordDecl(equalsBoundNode("class"))))
|
|
.bind("tmp_var")),
|
|
hasDescendant(stmt(anyOf(
|
|
cxxMemberCallExpr(hasArgument(
|
|
0, declRefExpr(to(varDecl(equalsBoundNode("tmp_var")))))),
|
|
callExpr(
|
|
callee(functionDecl(hasName("swap"))), argumentCountIs(2),
|
|
hasAnyArgument(
|
|
declRefExpr(to(varDecl(equalsBoundNode("tmp_var"))))),
|
|
hasAnyArgument(unaryOperator(has(cxxThisExpr()),
|
|
hasOperatorName("*"))))))))));
|
|
|
|
DeclarationMatcher AdditionalMatcher = cxxMethodDecl();
|
|
if (WarnOnlyIfThisHasSuspiciousField) {
|
|
// Matcher for standard smart pointers.
|
|
const auto SmartPointerType = qualType(hasUnqualifiedDesugaredType(
|
|
recordType(hasDeclaration(classTemplateSpecializationDecl(
|
|
anyOf(allOf(hasAnyName("::std::shared_ptr", "::std::weak_ptr",
|
|
"::std::auto_ptr"),
|
|
templateArgumentCountIs(1)),
|
|
allOf(hasName("::std::unique_ptr"),
|
|
templateArgumentCountIs(2))))))));
|
|
|
|
// We will warn only if the class has a pointer or a C array field which
|
|
// probably causes a problem during self-assignment (e.g. first resetting
|
|
// the pointer member, then trying to access the object pointed by the
|
|
// pointer, or memcpy overlapping arrays).
|
|
AdditionalMatcher = cxxMethodDecl(ofClass(cxxRecordDecl(
|
|
has(fieldDecl(anyOf(hasType(pointerType()), hasType(SmartPointerType),
|
|
hasType(arrayType())))))));
|
|
}
|
|
|
|
Finder->addMatcher(
|
|
cxxMethodDecl(
|
|
ofClass(cxxRecordDecl().bind("class")), isCopyAssignmentOperator(),
|
|
IsUserDefined, HasReferenceParam, HasNoSelfCheck,
|
|
unless(HasNonTemplateSelfCopy), unless(HasTemplateSelfCopy),
|
|
unless(HasCopyAndSwap), HasNoNestedSelfAssign, AdditionalMatcher)
|
|
.bind("copyAssignmentOperator"),
|
|
this);
|
|
}
|
|
|
|
void UnhandledSelfAssignmentCheck::check(
|
|
const MatchFinder::MatchResult &Result) {
|
|
const auto *MatchedDecl =
|
|
Result.Nodes.getNodeAs<CXXMethodDecl>("copyAssignmentOperator");
|
|
diag(MatchedDecl->getLocation(),
|
|
"operator=() does not handle self-assignment properly");
|
|
}
|
|
|
|
} // namespace clang::tidy::bugprone
|