Skip to content

[clang-tidy] Add C++ member function support to custom bugprone-unsafe-functions matches #117165

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Discookie merged 7 commits into from
Jan 30, 2025
Merged

[clang-tidy] Add C++ member function support to custom bugprone-unsafe-functions matches #117165

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e90ab99
[clang-tidy] Add C++ member function support to user-defined bugprone…
Discookie Nov 21, 2024
3bec1ac
Add option to show fully qualified names of functions
Discookie Dec 5, 2024
ade740c
Add note about matching C++ member functions
Discookie Dec 12, 2024
063fac5
Remove ShowFullyQualifiedNames to be added separately
Discookie Jan 6, 2025
44b8595
Add example for wildcard template maching in the docs
Discookie Jan 6, 2025
bcc3ccb
Remove leftover reference to FQ names
Discookie Jan 10, 2025
012a85a
Merge branch 'main' into unsafe-member-functions
Discookie Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 41 additions & 9 deletions clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ namespace clang::tidy::bugprone {

static constexpr llvm::StringLiteral OptionNameCustomFunctions =
"CustomFunctions";
static constexpr llvm::StringLiteral OptionNameShowFullyQualifiedNames =
"ShowFullyQualifiedNames";
static constexpr llvm::StringLiteral OptionNameReportDefaultFunctions =
"ReportDefaultFunctions";
static constexpr llvm::StringLiteral OptionNameReportMoreUnsafeFunctions =
Expand Down Expand Up @@ -185,6 +187,8 @@ UnsafeFunctionsCheck::UnsafeFunctionsCheck(StringRef Name,
: ClangTidyCheck(Name, Context),
CustomFunctions(parseCheckedFunctions(
Options.get(OptionNameCustomFunctions, ""), Context)),
ShowFullyQualifiedNames(
Options.get(OptionNameShowFullyQualifiedNames, false)),
ReportDefaultFunctions(
Options.get(OptionNameReportDefaultFunctions, true)),
ReportMoreUnsafeFunctions(
Expand All @@ -193,6 +197,8 @@ UnsafeFunctionsCheck::UnsafeFunctionsCheck(StringRef Name,
void UnsafeFunctionsCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, OptionNameCustomFunctions,
serializeCheckedFunctions(CustomFunctions));
Options.store(Opts, OptionNameShowFullyQualifiedNames,
ShowFullyQualifiedNames);
Options.store(Opts, OptionNameReportDefaultFunctions, ReportDefaultFunctions);
Options.store(Opts, OptionNameReportMoreUnsafeFunctions,
ReportMoreUnsafeFunctions);
Expand Down Expand Up @@ -256,13 +262,32 @@ void UnsafeFunctionsCheck::registerMatchers(MatchFinder *Finder) {
.bind(CustomFunctionNamesId)))
.bind(DeclRefId),
this);
// C++ member calls do not contain a DeclRefExpr to the function decl.
// Instead, they contain a MemberExpr that refers to the decl.
Finder->addMatcher(memberExpr(member(functionDecl(CustomFunctionsMatcher)
.bind(CustomFunctionNamesId)))
.bind(DeclRefId),
this);
}
}

void UnsafeFunctionsCheck::check(const MatchFinder::MatchResult &Result) {
const auto *DeclRef = Result.Nodes.getNodeAs<DeclRefExpr>(DeclRefId);
const auto *FuncDecl = cast<FunctionDecl>(DeclRef->getDecl());
assert(DeclRef && FuncDecl && "No valid matched node in check()");
const Expr *SourceExpr;
const FunctionDecl *FuncDecl;

if (const auto *DeclRef = Result.Nodes.getNodeAs<DeclRefExpr>(DeclRefId)) {
SourceExpr = DeclRef;
FuncDecl = cast<FunctionDecl>(DeclRef->getDecl());
} else if (const auto *Member =
Result.Nodes.getNodeAs<MemberExpr>(DeclRefId)) {
SourceExpr = Member;
FuncDecl = cast<FunctionDecl>(Member->getMemberDecl());
} else {
llvm_unreachable("No valid matched node in check()");
return;
}

assert(SourceExpr && FuncDecl && "No valid matched node in check()");

// Only one of these are matched at a time.
const auto *AnnexK = Result.Nodes.getNodeAs<FunctionDecl>(
Expand All @@ -286,14 +311,21 @@ void UnsafeFunctionsCheck::check(const MatchFinder::MatchResult &Result) {
Entry.Reason.empty() ? "is marked as unsafe" : Entry.Reason.c_str();

if (Entry.Replacement.empty()) {
diag(DeclRef->getExprLoc(), "function %0 %1; it should not be used")
diag(SourceExpr->getExprLoc(),
"function %0 %1; it should not be used")
<< FuncDecl << Reason << Entry.Replacement
<< DeclRef->getSourceRange();
<< SourceExpr->getSourceRange();
} else {
diag(DeclRef->getExprLoc(),
diag(SourceExpr->getExprLoc(),
"function %0 %1; '%2' should be used instead")
<< FuncDecl << Reason << Entry.Replacement
<< DeclRef->getSourceRange();
<< SourceExpr->getSourceRange();
}

if (ShowFullyQualifiedNames) {
diag(SourceExpr->getExprLoc(),
"fully qualified name of function is: '%0'", DiagnosticIDs::Note)
<< FuncDecl->getQualifiedNameAsString();
}

return;
Expand Down Expand Up @@ -323,9 +355,9 @@ void UnsafeFunctionsCheck::check(const MatchFinder::MatchResult &Result) {
if (!ReplacementFunctionName)
return;

diag(DeclRef->getExprLoc(), "function %0 %1; '%2' should be used instead")
diag(SourceExpr->getExprLoc(), "function %0 %1; '%2' should be used instead")
<< FuncDecl << getRationaleFor(FunctionName)
<< ReplacementFunctionName.value() << DeclRef->getSourceRange();
<< ReplacementFunctionName.value() << SourceExpr->getSourceRange();
}

void UnsafeFunctionsCheck::registerPPCallbacks(
Expand Down
5 changes: 4 additions & 1 deletion clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,10 @@ class UnsafeFunctionsCheck : public ClangTidyCheck {
private:
const std::vector<CheckedFunction> CustomFunctions;

// If true, the default set of functions are reported.
/// If true, the fully qualified name of custom functions will be shown in a
/// note tag.
const bool ShowFullyQualifiedNames;
Copy link
Contributor

@5chmidti 5chmidti Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is something that should be added. The only reason this exists, is to provide users the capability to debug false-positive matches on their regex. Or is there an additional reason to add this option?

Copy link
Contributor Author

@Discookie Discookie Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes it is pretty difficult to discern the precise patteren in the first place. eg. above when matching .open(), aliases are not followed (ifstream =/= basic_ifstream<char>), and there's also a template parameter on the class name, but not on the function.

These are existing behaviors, and I haven't found a way to strip template data off of the classname yet.
I'll add some examples for template arguments to the docs.

/// If true, the default set of functions are reported.
const bool ReportDefaultFunctions;
/// If true, additional functions from widely used API-s (such as POSIX) are
/// added to the list of reported functions.
Expand Down
2 changes: 1 addition & 1 deletion clang-tools-extra/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ Changes in existing checks

- Improved :doc:`bugprone-unsafe-functions
<clang-tidy/checks/bugprone/unsafe-functions>` check to allow specifying
additional functions to match.
additional functions to match, including C++ member functions.

- Improved :doc:`bugprone-use-after-move
<clang-tidy/checks/bugprone/use-after-move>` to avoid triggering on
Expand Down
16 changes: 16 additions & 0 deletions clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-functions.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,16 @@ qualified name (i.e. ``std::original``), otherwise the regex is matched against
If the regular expression starts with `::` (or `^::`), it is matched against the
fully qualified name (``::std::original``).

.. note::

Fully qualified names can contain template parameters on certain C++ classes, but not on C++ functions.
Type aliases are resolved before matching.
As an example, the member function ``open`` in the class ``std::ifstream``
has a fully qualified name of ``::std::basic_ifstream<char>::open``.

To aid with finding the fully qualified names of expressions, the option ``ShowFullyQualifiedNames`` can be used.
This option will show the fully qualified name of all functions matched by the custom function matchers.

Options
-------

Expand All @@ -139,6 +149,12 @@ Options
function, and an optional reason, separated by comma. For more information,
see :ref:`Custom functions<CustomFunctions>`.

.. option:: ShowFullyQualifiedNames

When `true`, the fully qualified name of all functions matched by the custom
function matchers will be shown.
Default is `false`.

Examples
--------

Expand Down
68 changes: 54 additions & 14 deletions clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions-custom-regex.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,19 @@
// RUN: %check_clang_tidy -check-suffix=NON-STRICT-REGEX %s bugprone-unsafe-functions %t --\
// RUN: -config="{CheckOptions: {bugprone-unsafe-functions.CustomFunctions: '::name_match,replacement,is a qualname match;^::prefix_match,,is matched on qualname prefix'}}"
// RUN: -config="{CheckOptions: {bugprone-unsafe-functions.CustomFunctions: '::name_match,replacement,is a qualname match;^::prefix_match,,is matched on qualname prefix;^::S::member_match_,,is matched on a C++ class member', bugprone-unsafe-functions.ShowFullyQualifiedNames: true}}"
// RUN: %check_clang_tidy -check-suffix=STRICT-REGEX %s bugprone-unsafe-functions %t --\
// RUN: -config="{CheckOptions: {bugprone-unsafe-functions.CustomFunctions: '^name_match$,replacement,is matched on function name only;^::prefix_match$,,is a full qualname match'}}"
// RUN: -config="{CheckOptions: {bugprone-unsafe-functions.CustomFunctions: '^name_match$,replacement,is matched on function name only;^::prefix_match$,,is a full qualname match;^::S::member_match_1$,,is matched on a C++ class member'}}"

void name_match();
void prefix_match();

struct S {
static void member_match_1() {}
void member_match_2() {}
};

void member_match_1() {}
void member_match_unmatched() {}

namespace regex_test {
void name_match();
void prefix_match();
Expand All @@ -16,29 +24,61 @@ void prefix_match_regex();

void f1() {
name_match();
// CHECK-MESSAGES-NON-STRICT-REGEX: :[[@LINE-1]]:3: warning: function 'name_match' is a qualname match; 'replacement' should be used instead
// CHECK-MESSAGES-STRICT-REGEX: :[[@LINE-2]]:3: warning: function 'name_match' is matched on function name only; 'replacement' should be used instead
// CHECK-NOTES-NON-STRICT-REGEX: :[[@LINE-1]]:3: warning: function 'name_match' is a qualname match; 'replacement' should be used instead
// CHECK-NOTES-NON-STRICT-REGEX: :[[@LINE-2]]:3: note: fully qualified name of function is: 'name_match'
// CHECK-NOTES-STRICT-REGEX: :[[@LINE-3]]:3: warning: function 'name_match' is matched on function name only; 'replacement' should be used instead
prefix_match();
// CHECK-MESSAGES-NON-STRICT-REGEX: :[[@LINE-1]]:3: warning: function 'prefix_match' is matched on qualname prefix; it should not be used
// CHECK-MESSAGES-STRICT-REGEX: :[[@LINE-2]]:3: warning: function 'prefix_match' is a full qualname match; it should not be used
// CHECK-NOTES-NON-STRICT-REGEX: :[[@LINE-1]]:3: warning: function 'prefix_match' is matched on qualname prefix; it should not be used
// CHECK-NOTES-NON-STRICT-REGEX: :[[@LINE-2]]:3: note: fully qualified name of function is: 'prefix_match'
// CHECK-NOTES-STRICT-REGEX: :[[@LINE-3]]:3: warning: function 'prefix_match' is a full qualname match; it should not be used

::name_match();
// CHECK-MESSAGES-NON-STRICT-REGEX: :[[@LINE-1]]:3: warning: function 'name_match' is a qualname match; 'replacement' should be used instead
// CHECK-MESSAGES-STRICT-REGEX: :[[@LINE-2]]:3: warning: function 'name_match' is matched on function name only; 'replacement' should be used instead
// CHECK-NOTES-NON-STRICT-REGEX: :[[@LINE-1]]:3: warning: function 'name_match' is a qualname match; 'replacement' should be used instead
// CHECK-NOTES-NON-STRICT-REGEX: :[[@LINE-2]]:3: note: fully qualified name of function is: 'name_match'
// CHECK-NOTES-STRICT-REGEX: :[[@LINE-3]]:3: warning: function 'name_match' is matched on function name only; 'replacement' should be used instead
regex_test::name_match();
// CHECK-MESSAGES-NON-STRICT-REGEX: :[[@LINE-1]]:3: warning: function 'name_match' is a qualname match; 'replacement' should be used instead
// CHECK-MESSAGES-STRICT-REGEX: :[[@LINE-2]]:3: warning: function 'name_match' is matched on function name only; 'replacement' should be used instead
// CHECK-NOTES-NON-STRICT-REGEX: :[[@LINE-1]]:3: warning: function 'name_match' is a qualname match; 'replacement' should be used instead
// CHECK-NOTES-NON-STRICT-REGEX: :[[@LINE-2]]:3: note: fully qualified name of function is: 'regex_test::name_match'
// CHECK-NOTES-STRICT-REGEX: :[[@LINE-3]]:3: warning: function 'name_match' is matched on function name only; 'replacement' should be used instead
name_match_regex();
// CHECK-MESSAGES-NON-STRICT-REGEX: :[[@LINE-1]]:3: warning: function 'name_match_regex' is a qualname match; 'replacement' should be used instead
// CHECK-NOTES-NON-STRICT-REGEX: :[[@LINE-1]]:3: warning: function 'name_match_regex' is a qualname match; 'replacement' should be used instead
// CHECK-NOTES-NON-STRICT-REGEX: :[[@LINE-2]]:3: note: fully qualified name of function is: 'name_match_regex'
// no-warning STRICT-REGEX

::prefix_match();
// CHECK-MESSAGES-NON-STRICT-REGEX: :[[@LINE-1]]:3: warning: function 'prefix_match' is matched on qualname prefix; it should not be used
// CHECK-MESSAGES-STRICT-REGEX: :[[@LINE-2]]:3: warning: function 'prefix_match' is a full qualname match; it should not be used
// CHECK-NOTES-NON-STRICT-REGEX: :[[@LINE-1]]:3: warning: function 'prefix_match' is matched on qualname prefix; it should not be used
// CHECK-NOTES-NON-STRICT-REGEX: :[[@LINE-2]]:3: note: fully qualified name of function is: 'prefix_match'
// CHECK-NOTES-STRICT-REGEX: :[[@LINE-3]]:3: warning: function 'prefix_match' is a full qualname match; it should not be used
regex_test::prefix_match();
// no-warning NON-STRICT-REGEX
// no-warning STRICT-REGEX
prefix_match_regex();
// CHECK-MESSAGES-NON-STRICT-REGEX: :[[@LINE-1]]:3: warning: function 'prefix_match_regex' is matched on qualname prefix; it should not be used
// CHECK-NOTES-NON-STRICT-REGEX: :[[@LINE-1]]:3: warning: function 'prefix_match_regex' is matched on qualname prefix; it should not be used
// CHECK-NOTES-NON-STRICT-REGEX: :[[@LINE-2]]:3: note: fully qualified name of function is: 'prefix_match_regex'
// no-warning STRICT-REGEX
}

void f2() {
S s;

S::member_match_1();
// CHECK-NOTES-NON-STRICT-REGEX: :[[@LINE-1]]:3: warning: function 'member_match_1' is matched on a C++ class member; it should not be used
// CHECK-NOTES-NON-STRICT-REGEX: :[[@LINE-2]]:3: note: fully qualified name of function is: 'S::member_match_1'
// CHECK-NOTES-STRICT-REGEX: :[[@LINE-3]]:3: warning: function 'member_match_1' is matched on a C++ class member; it should not be used

s.member_match_1();
// CHECK-NOTES-NON-STRICT-REGEX: :[[@LINE-1]]:5: warning: function 'member_match_1' is matched on a C++ class member; it should not be used
// CHECK-NOTES-NON-STRICT-REGEX: :[[@LINE-2]]:5: note: fully qualified name of function is: 'S::member_match_1'
// CHECK-NOTES-STRICT-REGEX: :[[@LINE-3]]:5: warning: function 'member_match_1' is matched on a C++ class member; it should not be used

s.member_match_2();
// CHECK-NOTES-NON-STRICT-REGEX: :[[@LINE-1]]:5: warning: function 'member_match_2' is matched on a C++ class member; it should not be used
// CHECK-NOTES-NON-STRICT-REGEX: :[[@LINE-2]]:5: note: fully qualified name of function is: 'S::member_match_2'
// no-warning STRICT-REGEX

member_match_1();
// no-warning

member_match_unmatched();
// no-warning
}
Loading