[clang] 61c5ad8 - [Sema] Fix fixit cast printing inside macros (#66853)

[via cfe-commits]

Author: Shoaib Meenai
Date: 2023-09-20T17:32:35-07:00
New Revision: 61c5ad8857a71510e4393680a1e42740da4ba6fa

URL: https://github.com/llvm/llvm-project/commit/61c5ad8857a71510e4393680a1e42740da4ba6fa
DIFF: https://github.com/llvm/llvm-project/commit/61c5ad8857a71510e4393680a1e42740da4ba6fa.diff

LOG: [Sema] Fix fixit cast printing inside macros (#66853)

`Lexer::getLocForEndOfToken` is documented as returning an invalid
source location when the end of the token is inside a macro expansion.
We don't want that for this particular application, so just calculate
the end location directly instead.

Before this, format fix-its would omit the closing parenthesis (thus
producing invalid code) for macros, e.g.:

```
$ cat format.cpp
extern "C" int printf(const char *, ...);
enum class Foo { Bar };
#define LOG(...) printf(__VA_ARGS__)
void f(Foo foo) { LOG("%d\n", foo); }

$ clang -fsyntax-only format.cpp
format.cpp:4:29: warning: format specifies type 'int' but the argument has type 'Foo' [-Wformat]
    4 | void f(Foo f) { LOG("%d\n", f); }
      |                      ~~     ^
      |                             static_cast<int>(
format.cpp:3:25: note: expanded from macro 'LOG'
    3 | #define LOG(...) printf(__VA_ARGS__)
      |                         ^~~~~~~~~~~
1 warning generated.
```

We now emit a valid fix-it:

```
$ clang -fsyntax-only format.cpp
format.cpp:4:31: warning: format specifies type 'int' but the argument has type 'Foo' [-Wformat]
    4 | void f(Foo foo) { LOG("%d\n", foo); }
      |                        ~~     ^~~
      |                               static_cast<int>( )
format.cpp:3:25: note: expanded from macro 'LOG'
    3 | #define LOG(...) printf(__VA_ARGS__)
      |                         ^~~~~~~~~~~
1 warning generated.
```

Fixes https://github.com/llvm/llvm-project/issues/63462

Added: 
    

Modified: 
    clang/lib/Sema/SemaChecking.cpp
    clang/test/FixIt/format.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index fad70223362eddd..b867d55c174e68b 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -11470,7 +11470,11 @@ CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
         Hints.push_back(
             FixItHint::CreateInsertion(E->getBeginLoc(), CastFix.str()));
 
-        SourceLocation After = S.getLocForEndOfToken(E->getEndLoc());
+        // We don't use getLocForEndOfToken because it returns invalid source
+        // locations for macro expansions (by design).
+        SourceLocation EndLoc = S.SourceMgr.getSpellingLoc(E->getEndLoc());
+        SourceLocation After = EndLoc.getLocWithOffset(
+            Lexer::MeasureTokenLength(EndLoc, S.SourceMgr, S.LangOpts));
         Hints.push_back(FixItHint::CreateInsertion(After, ")"));
       }
 

diff  --git a/clang/test/FixIt/format.cpp b/clang/test/FixIt/format.cpp
index 9cc4c2600eb6670..5016ee587ed1c43 100644
--- a/clang/test/FixIt/format.cpp
+++ b/clang/test/FixIt/format.cpp
@@ -1,19 +1,61 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wformat %s
 // RUN: %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits -Wformat %s 2>&1 | FileCheck %s
 
 extern "C" int printf(const char *, ...);
+#define LOG(...) printf(__VA_ARGS__)
 
 namespace N {
   enum class E { One };
 }
 
-void a() {
+struct S {
+  N::E Type;
+};
+
+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);
+  printf("%hd", N::E::One); // expected-warning{{format specifies type 'short' but the argument has type 'N::E'}}
   // CHECK: "static_cast<short>("
 
-  printf("%hu", N::E::One);
+  printf("%hu", N::E::One); // expected-warning{{format specifies type 'unsigned short' but the argument has type 'N::E'}}
   // CHECK: "static_cast<unsigned short>("
+
+  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}:")"
+
+  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}:")"
+
+  LOG("%d", NEVal); // 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]]:18-[[@LINE-2]]:18}:")"
+
+  printf(
+      "%d",
+      SPtr->Type // expected-warning{{format specifies type 'int' but the argument has type 'N::E'}}
+  );
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:7}:"static_cast<int>("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:17-[[@LINE-3]]:17}:")"
+
+  LOG( // expected-warning{{format specifies type 'int' but the argument has type 'N::E'}}
+      "%d",
+      SPtr->Type
+  );
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:7}:"static_cast<int>("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:17-[[@LINE-3]]:17}:")"
+
+  printf("%d",
+      SRef.Type); // expected-warning{{format specifies type 'int' but the argument has type 'N::E'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:"static_cast<int>("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:")"
+
+  LOG("%d", // expected-warning{{format specifies type 'int' but the argument has type 'N::E'}}
+      SRef.Type);
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:"static_cast<int>("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:")"
 }