[clang] [Sema] Use underlying type of scoped enum for -Wformat diagnostics (PR #67378)
https://github.com/smeenai closed https://github.com/llvm/llvm-project/pull/67378 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Sema] Use underlying type of scoped enum for -Wformat diagnostics (PR #67378)
https://github.com/cjdb commented: Thanks for working on this, it seems like an important fix. Would you mind updating the release notes please? I'll approve afterwards. https://github.com/llvm/llvm-project/pull/67378 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Sema] Use underlying type of scoped enum for -Wformat diagnostics (PR #67378)
smeenai wrote: Ping. https://github.com/llvm/llvm-project/pull/67378 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Sema] Use underlying type of scoped enum for -Wformat diagnostics (PR #67378)
llvmbot wrote:
@llvm/pr-subscribers-clang
Changes
Right now, `-Wformat` for a scoped enum will suggest a cast based on the
format specifier being used. This can lead to incorrect results, e.g.
attempting to format a scoped enum with `%s` would suggest casting to
`char *` instead of fixing the specifier. Change the logic to treat the
scoped enum's underlying type as the intended type to be printed, and
suggest format specifier changes and casts based on that.
---
Full diff: https://github.com/llvm/llvm-project/pull/67378.diff
3 Files Affected:
- (modified) clang/lib/Sema/SemaChecking.cpp (+12-13)
- (added) clang/test/FixIt/format-darwin-enum-class.cpp (+35)
- (modified) clang/test/FixIt/format.cpp (+18-2)
``diff
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 1d56c495e899722..5af52ea5564248d 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -11345,12 +11345,15 @@ CheckPrintfHandler::checkFormatExpr(const
analyze_printf::PrintfSpecifier ,
ImplicitMatch == ArgType::NoMatchTypeConfusion)
Match = ImplicitMatch;
assert(Match != ArgType::MatchPromotion);
+
// Look through unscoped enums to their underlying type.
bool IsEnum = false;
bool IsScopedEnum = false;
+ QualType IntendedTy = ExprTy;
if (auto EnumTy = ExprTy->getAs()) {
+IntendedTy = EnumTy->getDecl()->getIntegerType();
if (EnumTy->isUnscopedEnumerationType()) {
- ExprTy = EnumTy->getDecl()->getIntegerType();
+ ExprTy = IntendedTy;
// This controls whether we're talking about the underlying type or not,
// which we only want to do when it's an unscoped enum.
IsEnum = true;
@@ -11362,7 +11365,6 @@ CheckPrintfHandler::checkFormatExpr(const
analyze_printf::PrintfSpecifier ,
// %C in an Objective-C context prints a unichar, not a wchar_t.
// If the argument is an integer of some kind, believe the %C and suggest
// a cast instead of changing the conversion specifier.
- QualType IntendedTy = ExprTy;
if (isObjCContext() &&
FS.getConversionSpecifier().getKind() == ConversionSpecifier::CArg) {
if (ExprTy->isIntegralOrUnscopedEnumerationType() &&
@@ -11398,8 +11400,10 @@ CheckPrintfHandler::checkFormatExpr(const
analyze_printf::PrintfSpecifier ,
std::tie(CastTy, CastTyName) = shouldNotPrintDirectly(S.Context,
IntendedTy, E);
if (!CastTy.isNull()) {
// %zi/%zu and %td/%tu are OK to use for NSInteger/NSUInteger of type int
- // (long in ASTContext). Only complain to pedants.
- if ((CastTyName == "NSInteger" || CastTyName == "NSUInteger") &&
+ // (long in ASTContext). Only complain to pedants or when they're the
+ // underlying type of a scoped enum (which always needs a cast).
+ if (!IsScopedEnum &&
+ (CastTyName == "NSInteger" || CastTyName == "NSUInteger") &&
(AT.isSizeT() || AT.isPtrdiffT()) &&
AT.matchesType(S.Context, CastTy))
Match = ArgType::NoMatchPedantic;
@@ -11454,20 +11458,15 @@ CheckPrintfHandler::checkFormatExpr(const
analyze_printf::PrintfSpecifier ,
// should be printed as 'long' for 64-bit compatibility.)
// Rather than emitting a normal format/argument mismatch, we want to
// add a cast to the recommended type (and correct the format string
- // if necessary).
+ // if necessary). We should also do so for scoped enumerations.
SmallString<16> CastBuf;
llvm::raw_svector_ostream CastFix(CastBuf);
CastFix << (S.LangOpts.CPlusPlus ? "static_cast<" : "(");
- if (IsScopedEnum) {
-CastFix << AT.getRepresentativeType(S.Context).getAsString(
-S.Context.getPrintingPolicy());
- } else {
-IntendedTy.print(CastFix, S.Context.getPrintingPolicy());
- }
+ IntendedTy.print(CastFix, S.Context.getPrintingPolicy());
CastFix << (S.LangOpts.CPlusPlus ? ">" : ")");
SmallVector Hints;
- if ((!AT.matchesType(S.Context, IntendedTy) && !IsScopedEnum) ||
+ if (AT.matchesType(S.Context, IntendedTy) != ArgType::Match ||
ShouldNotPrintDirectly)
Hints.push_back(FixItHint::CreateReplacement(SpecRange, os.str()));
@@ -11495,7 +11494,7 @@ CheckPrintfHandler::checkFormatExpr(const
analyze_printf::PrintfSpecifier ,
Hints.push_back(FixItHint::CreateInsertion(After, ")"));
}
- if (ShouldNotPrintDirectly) {
+ if (ShouldNotPrintDirectly && !IsScopedEnum) {
// The expression has a type that should not be printed directly.
// We extract the name from the typedef because we don't want to show
// the underlying type in the diagnostic.
diff --git a/clang/test/FixIt/format-darwin-enum-class.cpp
b/clang/test/FixIt/format-darwin-enum-class.cpp
new file mode 100644
index 000..5aa1a80d8614c20
--- /dev/null
+++ b/clang/test/FixIt/format-darwin-enum-class.cpp
@@ -0,0 +1,35 @@
+// RUN: %clang_cc1 -triple
[clang] [Sema] Use underlying type of scoped enum for -Wformat diagnostics (PR #67378)
https://github.com/smeenai created
https://github.com/llvm/llvm-project/pull/67378
Right now, `-Wformat` for a scoped enum will suggest a cast based on the
format specifier being used. This can lead to incorrect results, e.g.
attempting to format a scoped enum with `%s` would suggest casting to
`char *` instead of fixing the specifier. Change the logic to treat the
scoped enum's underlying type as the intended type to be printed, and
suggest format specifier changes and casts based on that.
>From a1c307ddcc309b6b915feaad6d09f74ecc3f6e79 Mon Sep 17 00:00:00 2001
From: Shoaib Meenai
Date: Mon, 25 Sep 2023 13:48:10 -0700
Subject: [PATCH] [Sema] Use underlying type of scoped enum for -Wformat
diagnostics
Right now, `-Wformat` for a scoped enum will suggest a cast based on the
format specifier being used. This can lead to incorrect results, e.g.
attempting to format a scoped enum with `%s` would suggest casting to
`char *` instead of fixing the specifier. Change the logic to treat the
scoped enum's underlying type as the intended type to be printed, and
suggest format specifier changes and casts based on that.
---
clang/lib/Sema/SemaChecking.cpp | 25 +++--
clang/test/FixIt/format-darwin-enum-class.cpp | 35 +++
clang/test/FixIt/format.cpp | 20 +--
3 files changed, 65 insertions(+), 15 deletions(-)
create mode 100644 clang/test/FixIt/format-darwin-enum-class.cpp
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 1d56c495e899722..5af52ea5564248d 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -11345,12 +11345,15 @@ CheckPrintfHandler::checkFormatExpr(const
analyze_printf::PrintfSpecifier ,
ImplicitMatch == ArgType::NoMatchTypeConfusion)
Match = ImplicitMatch;
assert(Match != ArgType::MatchPromotion);
+
// Look through unscoped enums to their underlying type.
bool IsEnum = false;
bool IsScopedEnum = false;
+ QualType IntendedTy = ExprTy;
if (auto EnumTy = ExprTy->getAs()) {
+IntendedTy = EnumTy->getDecl()->getIntegerType();
if (EnumTy->isUnscopedEnumerationType()) {
- ExprTy = EnumTy->getDecl()->getIntegerType();
+ ExprTy = IntendedTy;
// This controls whether we're talking about the underlying type or not,
// which we only want to do when it's an unscoped enum.
IsEnum = true;
@@ -11362,7 +11365,6 @@ CheckPrintfHandler::checkFormatExpr(const
analyze_printf::PrintfSpecifier ,
// %C in an Objective-C context prints a unichar, not a wchar_t.
// If the argument is an integer of some kind, believe the %C and suggest
// a cast instead of changing the conversion specifier.
- QualType IntendedTy = ExprTy;
if (isObjCContext() &&
FS.getConversionSpecifier().getKind() == ConversionSpecifier::CArg) {
if (ExprTy->isIntegralOrUnscopedEnumerationType() &&
@@ -11398,8 +11400,10 @@ CheckPrintfHandler::checkFormatExpr(const
analyze_printf::PrintfSpecifier ,
std::tie(CastTy, CastTyName) = shouldNotPrintDirectly(S.Context,
IntendedTy, E);
if (!CastTy.isNull()) {
// %zi/%zu and %td/%tu are OK to use for NSInteger/NSUInteger of type int
- // (long in ASTContext). Only complain to pedants.
- if ((CastTyName == "NSInteger" || CastTyName == "NSUInteger") &&
+ // (long in ASTContext). Only complain to pedants or when they're the
+ // underlying type of a scoped enum (which always needs a cast).
+ if (!IsScopedEnum &&
+ (CastTyName == "NSInteger" || CastTyName == "NSUInteger") &&
(AT.isSizeT() || AT.isPtrdiffT()) &&
AT.matchesType(S.Context, CastTy))
Match = ArgType::NoMatchPedantic;
@@ -11454,20 +11458,15 @@ CheckPrintfHandler::checkFormatExpr(const
analyze_printf::PrintfSpecifier ,
// should be printed as 'long' for 64-bit compatibility.)
// Rather than emitting a normal format/argument mismatch, we want to
// add a cast to the recommended type (and correct the format string
- // if necessary).
+ // if necessary). We should also do so for scoped enumerations.
SmallString<16> CastBuf;
llvm::raw_svector_ostream CastFix(CastBuf);
CastFix << (S.LangOpts.CPlusPlus ? "static_cast<" : "(");
- if (IsScopedEnum) {
-CastFix << AT.getRepresentativeType(S.Context).getAsString(
-S.Context.getPrintingPolicy());
- } else {
-IntendedTy.print(CastFix, S.Context.getPrintingPolicy());
- }
+ IntendedTy.print(CastFix, S.Context.getPrintingPolicy());
CastFix << (S.LangOpts.CPlusPlus ? ">" : ")");
SmallVector Hints;
- if ((!AT.matchesType(S.Context, IntendedTy) && !IsScopedEnum) ||
+ if (AT.matchesType(S.Context, IntendedTy) != ArgType::Match ||
ShouldNotPrintDirectly)
Hints.push_back(FixItHint::CreateReplacement(SpecRange, os.str()));
@@ -11495,7 +11494,7 @@
