[Sema] Use underlying type of scoped enum for -Wformat diagnostics #67378
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
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.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang ChangesRight now, Full diff: https://github.com/llvm/llvm-project/pull/67378.diff 3 Files Affected:
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 &FS,
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<EnumType>()) {
+ 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 &FS,
// %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 &FS,
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 &FS,
// 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<FixItHint,4> 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 &FS,
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 000000000000000..5aa1a80d8614c20
--- /dev/null
+++ b/clang/test/FixIt/format-darwin-enum-class.cpp
@@ -0,0 +1,35 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -fsyntax-only -verify -Wformat %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -fsyntax-only -fdiagnostics-parseable-fixits -Wformat %s 2>&1 | FileCheck %s
+
+extern "C" int printf(const char * restrict, ...);
+
+#if __LP64__
+typedef long CFIndex;
+typedef long NSInteger;
+typedef unsigned long NSUInteger;
+#else
+typedef int CFIndex;
+typedef int NSInteger;
+typedef unsigned int NSUInteger;
+#endif
+
+enum class CFIndexEnum : CFIndex { One };
+enum class NSIntegerEnum : NSInteger { Two };
+enum class NSUIntegerEnum : NSUInteger { Three };
+
+void f() {
+ printf("%d", CFIndexEnum::One); // expected-warning{{format specifies type 'int' but the argument has type 'CFIndexEnum'}}
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%ld"
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:"static_cast<long>("
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:32-[[@LINE-3]]:32}:")"
+
+ printf("%d", NSIntegerEnum::Two); // expected-warning{{format specifies type 'int' but the argument has type 'NSIntegerEnum'}}
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%ld"
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:"static_cast<long>("
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:34-[[@LINE-3]]:34}:")"
+
+ printf("%d", NSUIntegerEnum::Three); // expected-warning{{format specifies type 'int' but the argument has type 'NSUIntegerEnum'}}
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%lu"
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:"static_cast<unsigned long>("
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:37-[[@LINE-3]]:37}:")"
+}
diff --git a/clang/test/FixIt/format.cpp b/clang/test/FixIt/format.cpp
index 5016ee587ed1c43..4e6573a4f9e54e3 100644
--- a/clang/test/FixIt/format.cpp
+++ b/clang/test/FixIt/format.cpp
@@ -12,21 +12,33 @@ struct S {
N::E Type;
};
+using uint32_t = unsigned;
+enum class FixedE : uint32_t { Two };
+
void a(N::E NEVal, S *SPtr, S &SRef) {
printf("%d", N::E::One); // expected-warning{{format specifies type 'int' but the argument has type 'N::E'}}
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:16-[[@LINE-1]]:16}:"static_cast<int>("
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:25-[[@LINE-2]]:25}:")"
printf("%hd", N::E::One); // expected-warning{{format specifies type 'short' but the argument has type 'N::E'}}
- // CHECK: "static_cast<short>("
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:14}:"%d"
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:17}:"static_cast<int>("
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:26-[[@LINE-3]]:26}:")"
printf("%hu", N::E::One); // expected-warning{{format specifies type 'unsigned short' but the argument has type 'N::E'}}
- // CHECK: "static_cast<unsigned short>("
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:14}:"%d"
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:17}:"static_cast<int>("
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:26-[[@LINE-3]]:26}:")"
LOG("%d", N::E::One); // expected-warning{{format specifies type 'int' but the argument has type 'N::E'}}
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:13}:"static_cast<int>("
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:22-[[@LINE-2]]:22}:")"
+ LOG("%s", N::E::One); // expected-warning{{format specifies type 'char *' but the argument has type 'N::E'}}
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:8-[[@LINE-1]]:10}:"%d"
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"static_cast<int>("
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:22-[[@LINE-3]]:22}:")"
+
printf("%d", NEVal); // expected-warning{{format specifies type 'int' but the argument has type 'N::E'}}
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:16-[[@LINE-1]]:16}:"static_cast<int>("
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:21-[[@LINE-2]]:21}:")"
@@ -58,4 +70,8 @@ void a(N::E NEVal, S *SPtr, S &SRef) {
SRef.Type);
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:"static_cast<int>("
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:")"
+
+ printf("%u", FixedE::Two); //expected-warning{{format specifies type 'unsigned int' but the argument has type 'FixedE'}}
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:16-[[@LINE-1]]:16}:"static_cast<uint32_t>("
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:27-[[@LINE-2]]:27}:")"
}
|
|
Ping. |
cjdb
reviewed
Oct 2, 2023
|
I updated the release notes. I was planning to ask for a pick into 17, but I'll just tweak the release notes for the pick and modify the Clang 18 release notes afterwards if it's accepted. |
cjdb
approved these changes
Oct 2, 2023
|
I have commit access. Thanks for the review! |
smeenai
added a commit
to smeenai/llvm-project
that referenced
this pull request
Oct 2, 2023
…lvm#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. (cherry picked from commit 0b07b06)
tru
pushed a commit
that referenced
this pull request
Oct 10, 2023
…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. (cherry picked from commit 0b07b06)
porglezomp
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Apr 24, 2024
…stics (llvm#67378)" This reverts commit 69c8c96.
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.
Right now,
-Wformatfor a scoped enum will suggest a cast based on theformat specifier being used. This can lead to incorrect results, e.g.
attempting to format a scoped enum with
%swould suggest casting tochar *instead of fixing the specifier. Change the logic to treat thescoped enum's underlying type as the intended type to be printed, and
suggest format specifier changes and casts based on that.