[clang][Sema] Suggest static_cast in C++ code

[abrachet]

Summary

This patch changes the -Wformat diagnostic to suggest static_cast over
a C-style cast for {,Objective}C++ when recommending the argument be
casted rather than changing the format string.

Before:

clang/test/FixIt/format.mm:11:16: warning: format specifies type 'unichar' (aka 'unsigned short') but the argument has type 'wchar_t' [-Wformat]
   11 |   NSLog(@"%C", wchar_data);  // expected-warning{{format specifies type 'unichar' (aka 'unsigned short') but the argument has type 'wchar_t'}}
      |           ~~   ^~~~~~~~~~
      |                (unsigned short)

After:

clang/test/FixIt/format.mm:11:16: warning: format specifies type 'unichar' (aka 'unsigned short') but the argument has type 'wchar_t' [-Wformat]
   11 |   NSLog(@"%C", wchar_data);  // expected-warning{{format specifies type 'unichar' (aka 'unsigned short') but the argument has type 'wchar_t'}}
      |           ~~   ^~~~~~~~~~
      |                static_cast<unsigned short>( )

[tbaeder]

I'm generally fine with suggesting a static cast, but the only test case you're updating is a objective C one, isn't it? Isn't the fixit wrong there?

[abrachet]

In D153622#4452460, @tbaeder wrote:

I'm generally fine with suggesting a static cast, but the only test case you're updating is a objective C one, isn't it? Isn't the fixit wrong there?

That test is Objective C++, I don't know much about the language, but I've compiled this TU with the fixit applied and it compiles fine

I couldn't think of a case to test this fixit in C++, save for D153623 where that case is tested

[cjdb]

LGTM in general, but two things before approval:

1. please add a description of when this is applicable, along with an example (including the output suggested by Clang) to your git commit message
2. please update the release notes to have the same info

[aaron.ballman]

I'm not opposed to the changes but I also don't understand what's driving them either. Can you explain what problem you're solving, as this seems like a question of style (we traditionally don't concern fix-its with making style choices)?

Also, the changes need a release note and the patch needs more of a summary.

clang/lib/Sema/SemaChecking.cpp

11187–11189

It's a bit odd to me that we'd recommend a C++-style cast for an inherently C interface (format specifier strings), especially because that can be more of a choice of style than anything.

[abrachet]

In D153622#4452970, @aaron.ballman wrote:

I'm not opposed to the changes but I also don't understand what's driving them either. Can you explain what problem you're solving, as this seems like a question of style (we traditionally don't concern fix-its with making style choices)?

The main motivation here can be found in D153623, I've just split these patches into two to make them easier to review. The warning change in 3632e2f5179 caused a lot of new warnings for us so I made a made a fixit instead of needing to manually fixing them all.

Regarding clang being opinionated on the casting style, this is fairly common, I found [[ this | https://github.com/llvm/llvm-project/blob/d5ce68afdf65fd8906d9570c0a71d3557de70838/clang/lib/Sema/SemaExpr.cpp#L14622 ]] location that emits a static_cast suggestion in C++ mode and a C-style cast otherwise. There are also a couple of other fixits that unconditionally suggest static_cast but these are specific to C++ code. So I do think that fixits currently suggest static_cast over C-style casts. This changed was motivated by the fixit in D153623 which is only meaningful for C++

[aaron.ballman]

In D153622#4453475, @abrachet wrote:

In D153622#4452970, @aaron.ballman wrote:

I'm not opposed to the changes but I also don't understand what's driving them either. Can you explain what problem you're solving, as this seems like a question of style (we traditionally don't concern fix-its with making style choices)?

The main motivation here can be found in D153623, I've just split these patches into two to make them easier to review. The warning change in 3632e2f5179 caused a lot of new warnings for us so I made a made a fixit instead of needing to manually fixing them all.

Regarding clang being opinionated on the casting style, this is fairly common, I found [[ this | https://github.com/llvm/llvm-project/blob/d5ce68afdf65fd8906d9570c0a71d3557de70838/clang/lib/Sema/SemaExpr.cpp#L14622 ]] location that emits a static_cast suggestion in C++ mode and a C-style cast otherwise. There are also a couple of other fixits that unconditionally suggest static_cast but these are specific to C++ code. So I do think that fixits currently suggest static_cast over C-style casts. This changed was motivated by the fixit in D153623 which is only meaningful for C++

I brought this up because we support scoped enumerations in C as an extension, but C23 added scoped enumeration types officially (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3030.htm) and we've not yet determined how close or far from conformance we already are. However, a C-style cast in C and a C++-style cast in C++ is defensible. The part that is unfortunate is that I wouldn't apply that fix-it myself because I think a C++-style cast is too distracting for this particular use case. FWIW, I feel exactly the same way about the use of static_cast<void> for the -Wcomma case (added in https://reviews.llvm.org/D3976 without any mention of the choice being stylistic).

I won't block this patch, but I do think we should be very careful about making style choices like this with fix-its, as no matter which way we choose, some class of folks won't wish to apply the fix-it solely due to style, which lessens the utility of having the fix-it.

LGTM!

[cjdb]

With what @aaron.ballman has said in mind, should we be adding this to Clang, or should it be a clang-tidy thing?

[abrachet]

If we wanted to remove this patch I would be fine with that. There already exists clang-tidy modernize that can turn C-style casts to their C++ counterparts. So I think that is a fine compromise. Though a hesitation is that there is not git-clang-tidy (to my knowledge) that will only touch changed lines.

With what @aaron.ballman has said in mind, should we be adding this to Clang, or should it be a clang-tidy thing?

If this is in reference to moving D153623 out of clang and into clang-tidy, I wouldn't object to abandoning these two patches. Though to be frank, it's unlikely that I would contribute this to clang-tidy.

[cjdb]

In D153622#4485519, @abrachet wrote:

If we wanted to remove this patch I would be fine with that. There already exists clang-tidy modernize that can turn C-style casts to their C++ counterparts. So I think that is a fine compromise. Though a hesitation is that there is not git-clang-tidy (to my knowledge) that will only touch changed lines.

With what @aaron.ballman has said in mind, should we be adding this to Clang, or should it be a clang-tidy thing?

If this is in reference to moving D153623 out of clang and into clang-tidy, I wouldn't object to abandoning these two patches. Though to be frank, it's unlikely that I would contribute this to clang-tidy.

It's a reference to what Aaron has said in this patch:

as this seems like a question of style (we traditionally don't concern fix-its with making style choices)?

We typically put style stuff into clang-tidy.

[aaron.ballman]

In D153622#4474385, @cjdb wrote:

With what @aaron.ballman has said in mind, should we be adding this to Clang, or should it be a clang-tidy thing?

I (weakly) think we should keep it in Clang: I think we should have fix-its when possible and this is a case that can really save folks some head-scratching and avoid UB, but if we're going to have a fix-it, we need to pick *some* cast style to use, which means we're making a kind of style choice no matter what we do.

I think users who have strong preferences that a particular cast style is used consistently could then use clang-tidy to clean up the style part later. That's not entirely different from how we handle things like whitespace decisions with fix-its and clang-format's role in cleaning up those changes. We don't have such a check currently, but it seems plausible to add such a check should there be sufficient demand for it.

clang/docs/ReleaseNotes.rst

- Clang's notes about unconvertible types in overload resolution failure now covers
   .. 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.
 
 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 (IntendedTy == ExprTy && !ShouldNotPrintDirectly) {
       // actual type of the expression. (This occurs, for example, with Darwin's
       // 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 << "(";
+      CastFix << (S.LangOpts.CPlusPlus ? "static_cast<" : "(");
       IntendedTy.print(CastFix, S.Context.getPrintingPolicy());
-      CastFix << ")";
+      CastFix << (S.LangOpts.CPlusPlus ? ">" : ")");
 
       SmallVector<FixItHint,4> Hints;
       if (!AT.matchesType(S.Context, IntendedTy) || 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)) {
+      } else if (!requiresParensToAddCast(E) && !S.LangOpts.CPlusPlus) {
         // If the expression has high enough precedence,
         // just write the C-style cast.
         Hints.push_back(
             FixItHint::CreateInsertion(E->getBeginLoc(), CastFix.str()));
       } else {
         // Otherwise, add parens around the expression as well as the cast.
         CastFix << "(";
         Hints.push_back(

clang/test/FixIt/format.mm

 // RUN: %clang_cc1 -fsyntax-only -fblocks -verify %s
 // RUN: %clang_cc1 -fdiagnostics-parseable-fixits -fblocks %s 2>&1 | FileCheck %s
 
 extern "C" void NSLog(id, ...);
 
 void test_percent_C() {
   const unsigned short data = 'a';
   NSLog(@"%C", data);  // no-warning
 
   const wchar_t wchar_data = L'a';
   NSLog(@"%C", wchar_data);  // expected-warning{{format specifies type 'unichar' (aka 'unsigned short') but the argument has type 'wchar_t'}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:16-[[@LINE-1]]:16}:"(unsigned short)"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:16-[[@LINE-1]]:16}:"static_cast<unsigned short>(
 
   NSLog(@"%C", 0x260300);  // expected-warning{{format specifies type 'unichar' (aka 'unsigned short') but the argument has type 'int'}}
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%d"
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:"(unsigned short)"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:"static_cast<unsigned short>(
 
   typedef unsigned short unichar;
 
   NSLog(@"%C", wchar_data);  // expected-warning{{format specifies type 'unichar' (aka 'unsigned short') but the argument has type 'wchar_t'}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:16-[[@LINE-1]]:16}:"(unichar)"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:16-[[@LINE-1]]:16}:"static_cast<unichar>(
 
   NSLog(@"%C", 0x260300);  // expected-warning{{format specifies type 'unichar' (aka 'unsigned short') but the argument has type 'int'}}
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%d"
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:"(unichar)"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:"static_cast<unichar>(
 
   NSLog(@"%C", 0.0); // expected-warning{{format specifies type 'unichar' (aka 'unsigned short') but the argument has type 'double'}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%f"
-  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:"(unichar)"
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:"static_cast<unichar>(
 }