From 3e12b2e207cfa802937488a2c0b90d482eaf00a9 Mon Sep 17 00:00:00 2001 From: Mike Crowe Date: Tue, 27 Jun 2023 16:35:22 +0000 Subject: [PATCH] [clang-tidy] Fix modernize-use-std-print check when return value used The initial implementation of the modernize-use-std-print check was capable of converting calls to printf (etc.) which used the return value to calls to std::print which has no return value, thus breaking the code. Use code inspired by the implementation of bugprone-unused-return-value check to ignore cases where the return value is used. Add appropriate lit test cases and documentation. Reviewed By: PiotrZSL Differential Revision: https://reviews.llvm.org/D153860 --- .../clang-tidy/modernize/UseStdPrintCheck.cpp | 54 +++-- .../clang-tidy/checks/modernize/use-std-print.rst | 7 + .../checkers/modernize/use-std-print-absl.cpp | 16 ++ .../checkers/modernize/use-std-print-custom.cpp | 12 ++ .../checkers/modernize/use-std-print.cpp | 227 +++++++++++++++++++++ 5 files changed, 302 insertions(+), 14 deletions(-) diff --git a/clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp index 9b368f9..b1e1189 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp @@ -70,27 +70,53 @@ void UseStdPrintCheck::registerPPCallbacks(const SourceManager &SM, IncludeInserter.registerPreprocessor(PP); } +static clang::ast_matchers::StatementMatcher +unusedReturnValue(clang::ast_matchers::StatementMatcher MatchedCallExpr) { + auto UnusedInCompoundStmt = + compoundStmt(forEach(MatchedCallExpr), + // The checker can't currently differentiate between the + // return statement and other statements inside GNU statement + // expressions, so disable the checker inside them to avoid + // false positives. + unless(hasParent(stmtExpr()))); + auto UnusedInIfStmt = + ifStmt(eachOf(hasThen(MatchedCallExpr), hasElse(MatchedCallExpr))); + auto UnusedInWhileStmt = whileStmt(hasBody(MatchedCallExpr)); + auto UnusedInDoStmt = doStmt(hasBody(MatchedCallExpr)); + auto UnusedInForStmt = + forStmt(eachOf(hasLoopInit(MatchedCallExpr), + hasIncrement(MatchedCallExpr), hasBody(MatchedCallExpr))); + auto UnusedInRangeForStmt = cxxForRangeStmt(hasBody(MatchedCallExpr)); + auto UnusedInCaseStmt = switchCase(forEach(MatchedCallExpr)); + + return stmt(anyOf(UnusedInCompoundStmt, UnusedInIfStmt, UnusedInWhileStmt, + UnusedInDoStmt, UnusedInForStmt, UnusedInRangeForStmt, + UnusedInCaseStmt)); +} + void UseStdPrintCheck::registerMatchers(MatchFinder *Finder) { if (!PrintfLikeFunctions.empty()) Finder->addMatcher( - callExpr(argumentCountAtLeast(1), - hasArgument(0, stringLiteral(isOrdinary())), - callee(functionDecl( - unless(cxxMethodDecl()), - matchers::matchesAnyListedName(PrintfLikeFunctions)) - .bind("func_decl"))) - .bind("printf"), + unusedReturnValue( + callExpr(argumentCountAtLeast(1), + hasArgument(0, stringLiteral(isOrdinary())), + callee(functionDecl(unless(cxxMethodDecl()), + matchers::matchesAnyListedName( + PrintfLikeFunctions)) + .bind("func_decl"))) + .bind("printf")), this); if (!FprintfLikeFunctions.empty()) Finder->addMatcher( - callExpr(argumentCountAtLeast(2), - hasArgument(1, stringLiteral(isOrdinary())), - callee(functionDecl(unless(cxxMethodDecl()), - matchers::matchesAnyListedName( - FprintfLikeFunctions)) - .bind("func_decl"))) - .bind("fprintf"), + unusedReturnValue( + callExpr(argumentCountAtLeast(2), + hasArgument(1, stringLiteral(isOrdinary())), + callee(functionDecl(unless(cxxMethodDecl()), + matchers::matchesAnyListedName( + FprintfLikeFunctions)) + .bind("func_decl"))) + .bind("fprintf")), this); } diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst index 385ee35..8034238 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst @@ -49,6 +49,13 @@ It doesn't do a bad job, but it's not perfect. In particular: - The glibc extension ``%m``. +- ``printf`` and similar functions return the number of characters printed. + ``std::print`` does not. This means that any invocations that use the + return value will not be converted. Unfortunately this currently includes + explicitly-casting to ``void``. Deficiencies in this check mean that any + invocations inside ``GCC`` compound statements cannot be converted even + if the resulting value is not used. + If conversion would be incomplete or unsafe then the entire invocation will be left unchanged. diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print-absl.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print-absl.cpp index 5345380..e35d79d 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print-absl.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print-absl.cpp @@ -55,6 +55,14 @@ void printf_no_casts_in_strict_mode() { // CHECK-FIXES: std::println("Unsigned integer {} from short", s); } +int printf_uses_return_value(int i) { + using namespace absl; + + return PrintF("return value %d\n", i); + // CHECK-MESSAGES-NOT: [[@LINE-1]]:10: warning: use 'std::println' instead of 'PrintF' [modernize-use-std-print] + // CHECK-FIXES-NOT: std::println("return value {}", i); +} + void fprintf_simple(FILE *fp) { absl::FPrintF(fp, "Hello %s %d", "world", 42); // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'FPrintF' [modernize-use-std-print] @@ -85,3 +93,11 @@ void fprintf_no_casts_in_strict_mode(FILE *fp) { // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'FPrintF' [modernize-use-std-print] // CHECK-FIXES: std::println(fp, "Unsigned integer {} from short", s); } + +int fprintf_uses_return_value(FILE *fp, int i) { + using namespace absl; + + return FPrintF(fp, "return value %d\n", i); + // CHECK-MESSAGES-NOT: [[@LINE-1]]:10: warning: use 'std::println' instead of 'FPrintF' [modernize-use-std-print] + // CHECK-FIXES-NOT: std::println("return value {}", i); +} 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 0877983..a8dda40 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 @@ -48,6 +48,12 @@ void printf_newline() { printf("Hello %s %d\n", "world", 42); } +int printf_uses_return_value(int i) { + return myprintf("return value %d\n", i); + // CHECK-MESSAGES-NOT: [[@LINE-1]]:10: warning: use 'std::println' instead of 'myprintf' [modernize-use-std-print] + // CHECK-FIXES-NOT: std::println("return value {}", i); +} + void fprintf_simple(FILE *fp) { myfprintf(stderr, "Hello %s %d", "world", 42); @@ -73,3 +79,9 @@ void fprintf_newline(FILE *fp) // When using custom options leave fprintf alone fprintf(stderr, "Hello %s %d\n", "world", 42); } + +int fprintf_uses_return_value(int i) { + return myfprintf(stderr, "return value %d\n", i); + // CHECK-MESSAGES-NOT: [[@LINE-1]]:10: warning: use 'std::println' instead of 'myprintf' [modernize-use-std-print] + // CHECK-FIXES-NOT: std::println(stderr, "return value {}", i); +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp index a40eec9..e75c2d4 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp @@ -44,6 +44,233 @@ void printf_deceptive_newline() { // CHECK-FIXES: std::println("Hello"); } +// std::print returns nothing, so any callers that use the return +// value cannot be automatically translated. +int printf_uses_return_value(int choice) { + const int i = printf("Return value assigned to variable %d\n", 42); + + if (choice == 0) + printf("if body %d\n", i); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: use 'std::println' instead of 'printf' [modernize-use-std-print] + // CHECK-FIXES: std::println("if body {}", i); + else if (choice == 1) + printf("else if body %d\n", i); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: use 'std::println' instead of 'printf' [modernize-use-std-print] + // CHECK-FIXES: std::println("else if body {}", i); + else + printf("else body %d\n", i); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: use 'std::println' instead of 'printf' [modernize-use-std-print] + // CHECK-FIXES: std::println("else body {}", i); + + if (printf("Return value used as boolean in if statement")) + if (printf("Return value used in expression if statement") == 44) + if (const int j = printf("Return value used in assignment in if statement")) + if (const int k = printf("Return value used with initializer in if statement"); k == 44) + ; + + int d = 0; + while (printf("%d", d) < 2) + ++d; + + while (true) + printf("while body %d\n", i); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: use 'std::println' instead of 'printf' [modernize-use-std-print] + // CHECK-FIXES: std::println("while body {}", i); + + do + printf("do body %d\n", i); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: use 'std::println' instead of 'printf' [modernize-use-std-print] + // CHECK-FIXES: std::println("do body {}", i); + while (true); + + for (;;) + printf("for body %d\n", i); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: use 'std::println' instead of 'printf' [modernize-use-std-print] + // CHECK-FIXES: std::println("for body {}", i); + + for (printf("for init statement %d\n", i);;) + // CHECK-MESSAGES: [[@LINE-1]]:8: warning: use 'std::println' instead of 'printf' [modernize-use-std-print] + // CHECK-FIXES: std::println("for init statement {}", i); + ;; + + for (int j = printf("for init statement %d\n", i);;) + ;; + + for (; printf("for condition %d\n", i);) + ;; + + for (;; printf("for expression %d\n", i)) + // CHECK-MESSAGES: [[@LINE-1]]:11: warning: use 'std::println' instead of 'printf' [modernize-use-std-print] + // CHECK-FIXES: std::println("for expression {}", i) + ;; + + for (auto C : "foo") + printf("ranged-for body %d\n", i); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: use 'std::println' instead of 'printf' [modernize-use-std-print] + // CHECK-FIXES: std::println("ranged-for body {}", i); + + switch (1) { + case 1: + printf("switch case body %d\n", i); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: use 'std::println' instead of 'printf' [modernize-use-std-print] + // CHECK-FIXES: std::println("switch case body {}", i); + break; + default: + printf("switch default body %d\n", i); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: use 'std::println' instead of 'printf' [modernize-use-std-print] + // CHECK-FIXES: std::println("switch default body {}", i); + break; + } + + try { + printf("try body %d\n", i); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: use 'std::println' instead of 'printf' [modernize-use-std-print] + // CHECK-FIXES: std::println("try body {}", i); + } catch (int) { + printf("catch body %d\n", i); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: use 'std::println' instead of 'printf' [modernize-use-std-print] + // CHECK-FIXES: std::println("catch body {}", i); + } + + (printf("Parenthesised expression %d\n", i)); + // CHECK-MESSAGES: [[@LINE-1]]:4: warning: use 'std::println' instead of 'printf' [modernize-use-std-print] + // CHECK-FIXES: (std::println("Parenthesised expression {}", i)); + + // Ideally we would convert these two, but the current check doesn't cope with + // that. + (void)printf("cast to void %d\n", i); + // CHECK-MESSAGES-NOT: [[@LINE-1]]:9: warning: use 'std::println' instead of 'printf' [modernize-use-std-print] + // CHECK-FIXES-NOT: std::println("cast to void {}", i); + + static_cast(printf("static_cast to void %d\n", i)); + // CHECK-MESSAGES-NOT: [[@LINE-1]]:9: warning: use 'std::println' instead of 'printf' [modernize-use-std-print] + // CHECK-FIXES-NOT: std::println("static cast to void {}", i); + + const int x = ({ printf("GCC statement expression using return value immediately %d\n", i); }); + const int y = ({ const int y = printf("GCC statement expression using return value immediately %d\n", i); y; }); + + // Ideally we would convert this one, but the current check doesn't cope with + // that. + ({ printf("GCC statement expression with unused result %d\n", i); }); + // CHECK-MESSAGES-NOT: [[@LINE-1]]:6: warning: use 'std::println' instead of 'printf' [modernize-use-std-print] + // CHECK-FIXES-NOT: std::println("GCC statement expression with unused result {}", i); + + return printf("Return value used in return\n"); +} + +int fprintf_uses_return_value(int choice) { + const int i = fprintf(stderr, "Return value assigned to variable %d\n", 42); + + if (choice == 0) + fprintf(stderr, "if body %d\n", i); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print] + // CHECK-FIXES: std::println(stderr, "if body {}", i); + else if (choice == 1) + fprintf(stderr, "else if body %d\n", i); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print] + // CHECK-FIXES: std::println(stderr, "else if body {}", i); + else + fprintf(stderr, "else body %d\n", i); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print] + // CHECK-FIXES: std::println(stderr, "else body {}", i); + + if (fprintf(stderr, "Return value used as boolean in if statement")) + if (fprintf(stderr, "Return value used in expression if statement") == 44) + if (const int j = fprintf(stderr, "Return value used in assignment in if statement")) + if (const int k = fprintf(stderr, "Return value used with initializer in if statement"); k == 44) + ; + + int d = 0; + while (fprintf(stderr, "%d", d) < 2) + ++d; + + while (true) + fprintf(stderr, "while body %d\n", i); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print] + // CHECK-FIXES: std::println(stderr, "while body {}", i); + + do + fprintf(stderr, "do body %d\n", i); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print] + // CHECK-FIXES: std::println(stderr, "do body {}", i); + while (true); + + for (;;) + fprintf(stderr, "for body %d\n", i); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print] + // CHECK-FIXES: std::println(stderr, "for body {}", i); + + for (fprintf(stderr, "for init statement %d\n", i);;) + // CHECK-MESSAGES: [[@LINE-1]]:8: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print] + // CHECK-FIXES: std::println(stderr, "for init statement {}", i); + ;; + + for (int j = fprintf(stderr, "for init statement %d\n", i);;) + ;; + + for (; fprintf(stderr, "for condition %d\n", i);) + ;; + + for (;; fprintf(stderr, "for expression %d\n", i)) + // CHECK-MESSAGES: [[@LINE-1]]:11: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print] + // CHECK-FIXES: std::println(stderr, "for expression {}", i) + ;; + + for (auto C : "foo") + fprintf(stderr, "ranged-for body %d\n", i); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print] + // CHECK-FIXES: std::println(stderr, "ranged-for body {}", i); + + switch (1) { + case 1: + fprintf(stderr, "switch case body %d\n", i); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print] + // CHECK-FIXES: std::println(stderr, "switch case body {}", i); + break; + default: + fprintf(stderr, "switch default body %d\n", i); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print] + // CHECK-FIXES: std::println(stderr, "switch default body {}", i); + break; + } + + try { + fprintf(stderr, "try body %d\n", i); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print] + // CHECK-FIXES: std::println(stderr, "try body {}", i); + } catch (int) { + fprintf(stderr, "catch body %d\n", i); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print] + // CHECK-FIXES: std::println(stderr, "catch body {}", i); + } + + + (printf("Parenthesised expression %d\n", i)); + // CHECK-MESSAGES: [[@LINE-1]]:4: warning: use 'std::println' instead of 'printf' [modernize-use-std-print] + // CHECK-FIXES: (std::println("Parenthesised expression {}", i)); + + // Ideally we would convert these two, but the current check doesn't cope with + // that. + (void)fprintf(stderr, "cast to void %d\n", i); + // CHECK-MESSAGES-NOT: [[@LINE-1]]:9: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print] + // CHECK-FIXES-NOT: std::println(stderr, "cast to void {}", i); + + static_cast(fprintf(stderr, "static_cast to void %d\n", i)); + // CHECK-MESSAGES-NOT: [[@LINE-1]]:9: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print] + // CHECK-FIXES-NOT: std::println(stderr, "static cast to void {}", i); + + const int x = ({ fprintf(stderr, "GCC statement expression using return value immediately %d\n", i); }); + const int y = ({ const int y = fprintf(stderr, "GCC statement expression using return value immediately %d\n", i); y; }); + + // Ideally we would convert this one, but the current check doesn't cope with + // that. + ({ fprintf(stderr, "GCC statement expression with unused result %d\n", i); }); + // CHECK-MESSAGES-NOT: [[@LINE-1]]:6: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print] + // CHECK-FIXES-NOT: std::println("GCC statement expression with unused result {}", i); + + return fprintf(stderr, "Return value used in return\n"); +} + void fprintf_simple() { fprintf(stderr, "Hello"); // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'fprintf' [modernize-use-std-print] -- 2.7.4