[clang][Sema] Add fixit for scoped enum format error

[abrachet]

Summary

This helps transition code bases to handle the new warning added in 3632e2f5179
Before:

clang/test/FixIt/format.cpp:10:16: warning: format specifies type 'int' but the argument has type 'N::E' [-Wformat]
   10 |   printf("%d", N::E::One); // expected-warning{{format specifies type 'int' but the argument has type 'N::E'}}
      |           ~~   ^~~~~~~~~
      |           %d

After:

clang/test/FixIt/format.cpp:10:16: warning: format specifies type 'int' but the argument has type 'N::E' [-Wformat]
   10 |   printf("%d", N::E::One); // expected-warning{{format specifies type 'int' but the argument has type 'N::E'}}
      |           ~~   ^~~~~~~~~
      |                static_cast<int>( )

[tbaeder]

clang/test/FixIt/format.cpp

16

Does this CHECK line and the following one do anything without the : suffix?

[abrachet]

clang/test/FixIt/format.cpp

16

Thanks good catch

[aaron.ballman]

clang/test/FixIt/format.cpp

2

I'd like to see a C test for this functionality as well.

[cjdb]

Please add a release note for this.

[abrachet]

In D153623#4474379, @cjdb wrote:

Please add a release note for this.

Done

clang/test/FixIt/format.cpp

2

I don't think there is a way to get this warning to trigger for C code. The paper you linked in D153622 discusses mostly giving enum's type specifications (and clang has an extension for this). AFAICT, scoped enumerations through enum class are not possible in C even through extensions

[aaron.ballman]

LGTM!

clang/test/FixIt/format.cpp

2

Whoops, you're of course correct -- my brain was mixing up scoped enumeration and enumeration with a fixed underlying type. Sorry for that!

[cjdb]

Sorry, forgot to come back and LGTM, but it did indeed LGTM!

[smeenai]

Sorry for the delayed comment here.

The fix-it is convenient, but is it the best suggestion? It'll end up suggesting truncating the enum value instead of using the proper format specifier in https://godbolt.org/z/xdhrefG95, for example. More insidiously, the static_cast might work fine for all enum uses today but silently cause issues for future enum values.

Using std::to_underyling and matching the format specifier to the underlying type would be the best suggestion IMO, but that only works in C++23 and above :( I guess std::underlying_type_t isn't super ugly either for C++14 and above, but bare std::underlying_type is pretty rough to use in a cast. All of those require the addition of a header though, and I'm not sure if that's acceptable for a fix-it.

Would we at least consider suggesting a format specifier and cast based on the underlying type of the enum, instead of just casting to whatever type the format specifier was using? That won't guard against future changes to the enum, but it's better than the status quo IMO.

To be clear, I realize that applying the current fix-it won't make the code any worse than it was already. It'd be great if we could make it better instead though :)

clang/docs/ReleaseNotes.rst

.. code-block:: text
     source:2:15: error: no matching function for call to 'func'
     void test() { func(1, "two");  }
                   ^~~~
     source:1:6: note: candidate function not viable: no known conversion from 'const char[4]' to 'int' for 2nd argument
     void func(int aa, int bb);
          ^            ~~~~~~
 - ``-Wformat`` cast fix-its will now suggest ``static_cast`` instead of C-style casts
   for C++ code.
+- ``-Wformat`` will no longer suggest a no-op fix-it for fixing scoped enum format
+  warnings. Instead, it will suggest casting the enum object to the type specified
+  in the format string.
 
 Bug Fixes in This Version
 -------------------------
 
 - Fix segfault while running clang-rename on a non existing file.
   (`#36471 <https://github.com/llvm/llvm-project/issues/36471>`_)
 - Fix crash when diagnosing incorrect usage of ``_Nullable`` involving alias
   templates.

clang/lib/Sema/SemaChecking.cpp

if (Match == ArgType::MatchPromotion) {
     Match = ArgType::NoMatch;
   }
   if (ImplicitMatch == ArgType::NoMatchPedantic ||
       ImplicitMatch == ArgType::NoMatchTypeConfusion)
     Match = ImplicitMatch;
   assert(Match != ArgType::MatchPromotion);
   // Look through unscoped enums to their underlying type.
   bool IsEnum = false;
+  bool IsScopedEnum = false;
   if (auto EnumTy = ExprTy->getAs<EnumType>()) {
     if (EnumTy->isUnscopedEnumerationType()) {
       ExprTy = EnumTy->getDecl()->getIntegerType();
       // 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;
+    } else {
+      IsScopedEnum = true;
     }
   }
 
   // %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() &&
CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
   if (Success) {
     // Get the fix string from the fixed format specifier
     SmallString<16> buf;
     llvm::raw_svector_ostream os(buf);
     fixedFS.toString(os);
 
     CharSourceRange SpecRange = getSpecifierRange(StartSpecifier, SpecifierLen);
 
-    if (IntendedTy == ExprTy && !ShouldNotPrintDirectly) {
+    if (IntendedTy == ExprTy && !ShouldNotPrintDirectly && !IsScopedEnum) {
       unsigned Diag;
       switch (Match) {
       case ArgType::Match:
       case ArgType::MatchPromotion:
       case ArgType::NoMatchPromotionTypeConfusion:
         llvm_unreachable("expected non-matching");
       case ArgType::NoMatchPedantic:
         Diag = diag::warn_format_conversion_argument_type_mismatch_pedantic;
if (IntendedTy == ExprTy && !ShouldNotPrintDirectly && !IsScopedEnum) {
       // NSInteger on 32-bit platforms, where it is typedef'd as 'int', but
       // 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).
       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());
+      }
       CastFix << (S.LangOpts.CPlusPlus ? ">" : ")");
 
       SmallVector<FixItHint,4> Hints;
-      if (!AT.matchesType(S.Context, IntendedTy) || ShouldNotPrintDirectly)
+      if ((!AT.matchesType(S.Context, IntendedTy) && !IsScopedEnum) ||
+          ShouldNotPrintDirectly)
         Hints.push_back(FixItHint::CreateReplacement(SpecRange, os.str()));
 
       if (const CStyleCastExpr *CCast = dyn_cast<CStyleCastExpr>(E)) {
         // If there's already a cast present, just replace it.
         SourceRange CastRange(CCast->getLParenLoc(), CCast->getRParenLoc());
         Hints.push_back(FixItHint::CreateReplacement(CastRange, CastFix.str()));
 
       } else if (!requiresParensToAddCast(E) && !S.LangOpts.CPlusPlus) {

clang/test/FixIt/format.cpp

+// RUN: %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits -Wformat %s 2>&1 | FileCheck %s
+
+extern "C" int printf(const char *, ...);
+
+namespace N {
+  enum class E { One };
+}
+
+void a() {
+  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);
+  // CHECK: "static_cast<short>("
+
+  printf("%hu", N::E::One);
+  // CHECK: "static_cast<unsigned short>("
+}