[clang-tools-extra] [clang-tidy] Fix assert in modernize-use-std-format/print (PR #94104)
Mike Crowe via cfe-commits
cfe-commits at lists.llvm.org
Mon Jun 3 05:38:47 PDT 2024
https://github.com/mikecrowe updated https://github.com/llvm/llvm-project/pull/94104
>From 2972062997ca582100b5797cd548c4dc2f80c69a Mon Sep 17 00:00:00 2001
From: Mike Crowe <mac at mcrowe.com>
Date: Fri, 31 May 2024 21:27:03 +0100
Subject: [PATCH 1/3] [clang-tidy] Fix assert in modernize-use-std-format/print
Ensure that FormatStringConverter's constructor fails with a sensible
error message rather than asserting if the format string is not a narrow
string literal.
Also, ensure that we don't even get that far in modernize-use-std-print
and modernize-use-std-format by checking that the format string
parameter is a char pointer.
Fixes #92896
---
.../modernize/UseStdFormatCheck.cpp | 18 ++++++++-----
.../clang-tidy/modernize/UseStdPrintCheck.cpp | 8 ++++++
.../utils/FormatStringConverter.cpp | 8 +++---
.../modernize/use-std-format-custom.cpp | 18 +++++++++++--
.../modernize/use-std-print-custom.cpp | 26 +++++++++++++++++--
5 files changed, 65 insertions(+), 13 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/modernize/UseStdFormatCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseStdFormatCheck.cpp
index 6cef21f1318a2..5c72f8f22dec7 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseStdFormatCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseStdFormatCheck.cpp
@@ -20,6 +20,11 @@ namespace clang::tidy::modernize {
namespace {
AST_MATCHER(StringLiteral, isOrdinary) { return Node.isOrdinary(); }
+AST_MATCHER(QualType, isSimpleChar) {
+ const auto ActualType = Node.getTypePtr();
+ return ActualType->isSpecificBuiltinType(BuiltinType::Char_S) ||
+ ActualType->isSpecificBuiltinType(BuiltinType::Char_U);
+}
} // namespace
UseStdFormatCheck::UseStdFormatCheck(StringRef Name, ClangTidyContext *Context)
@@ -47,13 +52,14 @@ void UseStdFormatCheck::registerPPCallbacks(const SourceManager &SM,
}
void UseStdFormatCheck::registerMatchers(MatchFinder *Finder) {
+ auto CharPointerType = hasType(pointerType(pointee(isSimpleChar())));
Finder->addMatcher(
- callExpr(argumentCountAtLeast(1),
- hasArgument(0, stringLiteral(isOrdinary())),
- callee(functionDecl(unless(cxxMethodDecl()),
- matchers::matchesAnyListedName(
- StrFormatLikeFunctions))
- .bind("func_decl")))
+ callExpr(
+ argumentCountAtLeast(1), hasArgument(0, stringLiteral(isOrdinary())),
+ callee(functionDecl(
+ unless(cxxMethodDecl()), hasParameter(0, CharPointerType),
+ matchers::matchesAnyListedName(StrFormatLikeFunctions))
+ .bind("func_decl")))
.bind("strformat"),
this);
}
diff --git a/clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp
index ff990feadc0c1..6a4497eaf7f60 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp
@@ -20,6 +20,11 @@ namespace clang::tidy::modernize {
namespace {
AST_MATCHER(StringLiteral, isOrdinary) { return Node.isOrdinary(); }
+AST_MATCHER(QualType, isSimpleChar) {
+ const auto ActualType = Node.getTypePtr();
+ return ActualType->isSpecificBuiltinType(BuiltinType::Char_S) ||
+ ActualType->isSpecificBuiltinType(BuiltinType::Char_U);
+}
} // namespace
UseStdPrintCheck::UseStdPrintCheck(StringRef Name, ClangTidyContext *Context)
@@ -95,12 +100,14 @@ unusedReturnValue(clang::ast_matchers::StatementMatcher MatchedCallExpr) {
}
void UseStdPrintCheck::registerMatchers(MatchFinder *Finder) {
+ auto CharPointerType = hasType(pointerType(pointee(isSimpleChar())));
if (!PrintfLikeFunctions.empty())
Finder->addMatcher(
unusedReturnValue(
callExpr(argumentCountAtLeast(1),
hasArgument(0, stringLiteral(isOrdinary())),
callee(functionDecl(unless(cxxMethodDecl()),
+ hasParameter(0, CharPointerType),
matchers::matchesAnyListedName(
PrintfLikeFunctions))
.bind("func_decl")))
@@ -113,6 +120,7 @@ void UseStdPrintCheck::registerMatchers(MatchFinder *Finder) {
callExpr(argumentCountAtLeast(2),
hasArgument(1, stringLiteral(isOrdinary())),
callee(functionDecl(unless(cxxMethodDecl()),
+ hasParameter(1, CharPointerType),
matchers::matchesAnyListedName(
FprintfLikeFunctions))
.bind("func_decl")))
diff --git a/clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp b/clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp
index 845e71c5003b8..33f3ea47df1e3 100644
--- a/clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp
+++ b/clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp
@@ -208,9 +208,11 @@ FormatStringConverter::FormatStringConverter(ASTContext *ContextIn,
assert(ArgsOffset <= NumArgs);
FormatExpr = llvm::dyn_cast<StringLiteral>(
Args[FormatArgOffset]->IgnoreImplicitAsWritten());
- assert(FormatExpr);
- if (!FormatExpr->isOrdinary())
- return; // No wide string support yet
+ if (!FormatExpr || !FormatExpr->isOrdinary()) {
+ // Function must have a narrow string literal as its first argument.
+ conversionNotPossible("first argument is not a narrow string literal");
+ return;
+ }
PrintfFormatString = FormatExpr->getString();
// Assume that the output will be approximately the same size as the input,
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-format-custom.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-format-custom.cpp
index 815e22b291551..c025113055cce 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-format-custom.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-format-custom.cpp
@@ -2,7 +2,7 @@
// RUN: -std=c++20 %s modernize-use-std-format %t -- \
// RUN: -config="{CheckOptions: { \
// RUN: modernize-use-std-format.StrictMode: true, \
-// RUN: modernize-use-std-format.StrFormatLikeFunctions: '::strprintf; mynamespace::strprintf2', \
+// RUN: modernize-use-std-format.StrFormatLikeFunctions: '::strprintf; mynamespace::strprintf2; bad_format_type_strprintf', \
// RUN: modernize-use-std-format.ReplacementFormatFunction: 'fmt::format', \
// RUN: modernize-use-std-format.FormatHeader: '<fmt/core.h>' \
// RUN: }}" \
@@ -10,7 +10,7 @@
// RUN: %check_clang_tidy -check-suffixes=,NOTSTRICT \
// RUN: -std=c++20 %s modernize-use-std-format %t -- \
// RUN: -config="{CheckOptions: { \
-// RUN: modernize-use-std-format.StrFormatLikeFunctions: '::strprintf; mynamespace::strprintf2', \
+// RUN: modernize-use-std-format.StrFormatLikeFunctions: '::strprintf; mynamespace::strprintf2; bad_format_type_strprintf', \
// RUN: modernize-use-std-format.ReplacementFormatFunction: 'fmt::format', \
// RUN: modernize-use-std-format.FormatHeader: '<fmt/core.h>' \
// RUN: }}" \
@@ -50,3 +50,17 @@ std::string A(const std::string &in)
{
return "_" + in;
}
+
+// Issue #92896: Ensure that the check doesn't assert if the argument is
+// promoted to something that isn't a string.
+struct S {
+ S(...);
+};
+std::string bad_format_type_strprintf(const S &, ...);
+
+std::string unsupported_format_parameter_type()
+{
+ // No fixes here because the format parameter of the function called is not a
+ // string.
+ return bad_format_type_strprintf("");
+}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print-custom.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print-custom.cpp
index 8466217b765a8..09720001ab837 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print-custom.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print-custom.cpp
@@ -1,8 +1,8 @@
// RUN: %check_clang_tidy -std=c++23 %s modernize-use-std-print %t -- \
// RUN: -config="{CheckOptions: \
// RUN: { \
-// RUN: modernize-use-std-print.PrintfLikeFunctions: 'unqualified_printf;::myprintf; mynamespace::myprintf2', \
-// RUN: modernize-use-std-print.FprintfLikeFunctions: '::myfprintf; mynamespace::myfprintf2' \
+// RUN: modernize-use-std-print.PrintfLikeFunctions: 'unqualified_printf;::myprintf; mynamespace::myprintf2; bad_format_type_printf', \
+// RUN: modernize-use-std-print.FprintfLikeFunctions: '::myfprintf; mynamespace::myfprintf2; bad_format_type_fprintf' \
// RUN: } \
// RUN: }" \
// RUN: -- -isystem %clang_tidy_headers
@@ -86,3 +86,25 @@ void no_name(const std::string &in)
{
"A" + in;
}
+
+int myprintf(const wchar_t *, ...);
+
+void wide_string_not_supported() {
+ myprintf(L"wide string %s", L"string");
+}
+
+// Issue #92896: Ensure that the check doesn't assert if the argument is
+// promoted to something that isn't a string.
+struct S {
+ S(...) {}
+};
+int bad_format_type_printf(const S &, ...);
+int bad_format_type_fprintf(FILE *, const S &, ...);
+
+void unsupported_format_parameter_type()
+{
+ // No fixes here because the format parameter of the function called is not a
+ // string.
+ bad_format_type_printf("Hello %s", "world");
+ bad_format_type_fprintf(stderr, "Hello %s", "world");
+}
>From 6d384a8191454e3932faa16a9159785a07f1d82e Mon Sep 17 00:00:00 2001
From: Mike Crowe <mac at mcrowe.com>
Date: Mon, 3 Jun 2024 13:22:31 +0100
Subject: [PATCH 2/3] fixup! Improve isSimpleChar and move it into
utils/Matchers.h
---
.../clang-tidy/modernize/UseStdFormatCheck.cpp | 8 ++------
.../clang-tidy/modernize/UseStdPrintCheck.cpp | 8 ++------
clang-tools-extra/clang-tidy/utils/Matchers.h | 8 ++++++++
3 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/modernize/UseStdFormatCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseStdFormatCheck.cpp
index 5c72f8f22dec7..d082faa786b37 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseStdFormatCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseStdFormatCheck.cpp
@@ -20,11 +20,6 @@ namespace clang::tidy::modernize {
namespace {
AST_MATCHER(StringLiteral, isOrdinary) { return Node.isOrdinary(); }
-AST_MATCHER(QualType, isSimpleChar) {
- const auto ActualType = Node.getTypePtr();
- return ActualType->isSpecificBuiltinType(BuiltinType::Char_S) ||
- ActualType->isSpecificBuiltinType(BuiltinType::Char_U);
-}
} // namespace
UseStdFormatCheck::UseStdFormatCheck(StringRef Name, ClangTidyContext *Context)
@@ -52,7 +47,8 @@ void UseStdFormatCheck::registerPPCallbacks(const SourceManager &SM,
}
void UseStdFormatCheck::registerMatchers(MatchFinder *Finder) {
- auto CharPointerType = hasType(pointerType(pointee(isSimpleChar())));
+ auto CharPointerType =
+ hasType(pointerType(pointee(matchers::isSimpleChar())));
Finder->addMatcher(
callExpr(
argumentCountAtLeast(1), hasArgument(0, stringLiteral(isOrdinary())),
diff --git a/clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp
index 6a4497eaf7f60..1ea170c3cd310 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp
@@ -20,11 +20,6 @@ namespace clang::tidy::modernize {
namespace {
AST_MATCHER(StringLiteral, isOrdinary) { return Node.isOrdinary(); }
-AST_MATCHER(QualType, isSimpleChar) {
- const auto ActualType = Node.getTypePtr();
- return ActualType->isSpecificBuiltinType(BuiltinType::Char_S) ||
- ActualType->isSpecificBuiltinType(BuiltinType::Char_U);
-}
} // namespace
UseStdPrintCheck::UseStdPrintCheck(StringRef Name, ClangTidyContext *Context)
@@ -100,7 +95,8 @@ unusedReturnValue(clang::ast_matchers::StatementMatcher MatchedCallExpr) {
}
void UseStdPrintCheck::registerMatchers(MatchFinder *Finder) {
- auto CharPointerType = hasType(pointerType(pointee(isSimpleChar())));
+ auto CharPointerType =
+ hasType(pointerType(pointee(matchers::isSimpleChar())));
if (!PrintfLikeFunctions.empty())
Finder->addMatcher(
unusedReturnValue(
diff --git a/clang-tools-extra/clang-tidy/utils/Matchers.h b/clang-tools-extra/clang-tidy/utils/Matchers.h
index 045e3ffbb6a8b..5fd98db967870 100644
--- a/clang-tools-extra/clang-tidy/utils/Matchers.h
+++ b/clang-tools-extra/clang-tidy/utils/Matchers.h
@@ -49,6 +49,14 @@ AST_MATCHER_FUNCTION(ast_matchers::TypeMatcher, isPointerToConst) {
return pointerType(pointee(qualType(isConstQualified())));
}
+// Returns QualType matcher for target char type only.
+AST_MATCHER(QualType, isSimpleChar) {
+ const auto ActualType = Node.getTypePtr();
+ return ActualType &&
+ (ActualType->isSpecificBuiltinType(BuiltinType::Char_S) ||
+ ActualType->isSpecificBuiltinType(BuiltinType::Char_U));
+}
+
AST_MATCHER(Expr, hasUnevaluatedContext) {
if (isa<CXXNoexceptExpr>(Node) || isa<RequiresExpr>(Node))
return true;
>From b06d790e3ae47c6a34c9b107bbe56f8221c5e44d Mon Sep 17 00:00:00 2001
From: Mike Crowe <mac at mcrowe.com>
Date: Mon, 3 Jun 2024 13:32:52 +0100
Subject: [PATCH 3/3] fixup! Mention fix in release notes
---
clang-tools-extra/docs/ReleaseNotes.rst | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 4f674d1a4d556..8a92f0eef4b08 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -351,6 +351,10 @@ Changes in existing checks
<clang-tidy/checks/modernize/use-starts-ends-with>` check to also handle
calls to ``compare`` method.
+- Improved :doc:`modernize-use-std-format` and
+ :doc:`modernize-use-std-print` checks to not crash if the format string
+ parameter of the function to be replaced is not of the expected type.
+
- Improved :doc:`modernize-use-using <clang-tidy/checks/modernize/use-using>`
check by adding support for detection of typedefs declared on function level.
More information about the cfe-commits
mailing list