[clang-tidy] Fix assert in modernize-use-std-format/print #94104
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-clang-tidy Author: Mike Crowe (mikecrowe) ChangesEnsure 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. Full diff: https://github.com/llvm/llvm-project/pull/94104.diff 5 Files Affected:
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");
+}
|
|
@llvm/pr-subscribers-clang-tools-extra Author: Mike Crowe (mikecrowe) ChangesEnsure 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. Full diff: https://github.com/llvm/llvm-project/pull/94104.diff 5 Files Affected:
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");
+}
|
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 llvm#92896
mikecrowe
changed the title
PiotrZSL
approved these changes
Jun 1, 2024
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
PiotrZSL
reviewed
Jun 3, 2024
Don't mention modernize-use-std-format check since it is new in this version anyway.
Thanks for the review. Please can you squash and land it for me? I don't have permission to do so. |
|
Thank you @PiotrZSL! |
mikecrowe
added a commit
to mikecrowe/clang-tidy-fmt
that referenced
this pull request
Jul 16, 2024
When fixing llvm#92896 in 0e62d5c (llvm#94104) I failed to spot that I'd broken converting from fmt::printf, fmt::fprintf and fmt::sprintf in these checks since the format parameter of those functions is not a simple character pointer. The first part of the previous fix to avoid the assert and instead produce an error message was sufficient. It was only the second part that required the format parameter of the called function to be a simple character pointer that was problematic. Let's remove that second part and add the now-expected error messages to the lit tests along with fixing the prototype for the fmt functions to more accurately reflect the ones used by the fmt library so they are actually useful. Fixes llvm#92896
PiotrZSL
pushed a commit
that referenced
this pull request
Jul 17, 2024
When fixing #92896 in 0e62d5c (#94104) I failed to spot that I'd broken converting from fmt::printf, fmt::fprintf and fmt::sprintf in these checks since the format parameter of those functions is not a simple character pointer. The first part of the previous fix to avoid the assert and instead produce an error message was sufficient. It was only the second part that required the format parameter of the called function to be a simple character pointer that was problematic. Let's remove that second part and add the now-expected error messages to the lit tests along with fixing the prototype for the fmt functions to more accurately reflect the ones used by the fmt library so they are actually useful. Fixes #92896
yuxuanchen1997
pushed a commit
that referenced
this pull request
Jul 25, 2024
Summary: When fixing #92896 in 0e62d5c (#94104) I failed to spot that I'd broken converting from fmt::printf, fmt::fprintf and fmt::sprintf in these checks since the format parameter of those functions is not a simple character pointer. The first part of the previous fix to avoid the assert and instead produce an error message was sufficient. It was only the second part that required the format parameter of the called function to be a simple character pointer that was problematic. Let's remove that second part and add the now-expected error messages to the lit tests along with fixing the prototype for the fmt functions to more accurately reflect the ones used by the fmt library so they are actually useful. Fixes #92896 Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250858
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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