[Clang] fix missing source location for errors in macro-expanded

[a-tarasyuk]

Fixes #143216

---

This patch fixes diagnostic locations for tokens from macro expansions.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Oleksandr T. (a-tarasyuk)

Changes

Fixes #143216

---

This patch addresses the issue where diagnostics for case statements originating from macro expansions lacked source location information when the colon : was missing.

---

Full diff: https://github.com/llvm/llvm-project/pull/143460.diff

4 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+1)
• (modified) clang/include/clang/Basic/DiagnosticCommonKinds.td (+1)
• (modified) clang/lib/Parse/ParseStmt.cpp (+14)
• (modified) clang/test/Parser/switch-recovery.cpp (+13)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 322686fce0b04..0ecbb4864050c 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -689,6 +689,7 @@ Bug Fixes in This Version
 - Fixed type mismatch error when 'builtin-elementwise-math' arguments have different qualifiers, this should be well-formed. (#GH141397)
 - Constant evaluation now correctly runs the destructor of a variable declared in
   the second clause of a C-style ``for`` loop. (#GH139818)
+- Fixed incorrect diagnostic location for missing ``:`` in case statements expanded from macros. (#GH143216)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Basic/DiagnosticCommonKinds.td b/clang/include/clang/Basic/DiagnosticCommonKinds.td
index 0bd8a423c393e..ee8fc66c1822c 100644
--- a/clang/include/clang/Basic/DiagnosticCommonKinds.td
+++ b/clang/include/clang/Basic/DiagnosticCommonKinds.td
@@ -83,6 +83,7 @@ let CategoryName = "Parse Issue" in {
 def err_expected : Error<"expected %0">;
 def err_expected_either : Error<"expected %0 or %1">;
 def err_expected_after : Error<"expected %1 after %0">;
+def note_macro_expansion : Note<"expanded from macro '%0'">;
 
 def err_param_redefinition : Error<"redefinition of parameter %0">;
 def warn_method_param_redefinition : Warning<"redefinition of method parameter %0">;
diff --git a/clang/lib/Parse/ParseStmt.cpp b/clang/lib/Parse/ParseStmt.cpp
index c788723023c8b..5db6dd36f840b 100644
--- a/clang/lib/Parse/ParseStmt.cpp
+++ b/clang/lib/Parse/ParseStmt.cpp
@@ -833,9 +833,23 @@ StmtResult Parser::ParseCaseStatement(ParsedStmtContext StmtCtx,
           << FixItHint::CreateReplacement(ColonLoc, ":");
     } else {
       SourceLocation ExpectedLoc = PP.getLocForEndOfToken(PrevTokLocation);
+      SourceLocation ExprLoc =
+          LHS.get() ? LHS.get()->getExprLoc() : SourceLocation();
+
+      if (ExpectedLoc.isInvalid() && ExprLoc.isMacroID()) {
+        ExpectedLoc = PP.getSourceManager().getSpellingLoc(ExprLoc);
+      }
+
       Diag(ExpectedLoc, diag::err_expected_after)
           << "'case'" << tok::colon
           << FixItHint::CreateInsertion(ExpectedLoc, ":");
+
+      if (ExprLoc.isMacroID()) {
+        Diag(ExprLoc, diag::note_macro_expansion)
+            << Lexer::getImmediateMacroNameForDiagnostics(
+                   ExprLoc, PP.getSourceManager(), getLangOpts());
+      }
+
       ColonLoc = ExpectedLoc;
     }
 
diff --git a/clang/test/Parser/switch-recovery.cpp b/clang/test/Parser/switch-recovery.cpp
index baf703cd03aed..5966b04b3f636 100644
--- a/clang/test/Parser/switch-recovery.cpp
+++ b/clang/test/Parser/switch-recovery.cpp
@@ -229,3 +229,16 @@ void fn1() {
     }
 } // expected-error{{expected statement}}
 }
+
+namespace GH143216 {
+#define FOO 1 case 3: // expected-error {{expected ':' after 'case'}}
+
+int f(int x) {
+  switch (x) {
+  case FOO // expected-note {{expanded from macro 'FOO'}}
+    return 0;
+  default:
+    return 1;
+  }
+}
+}

[efriedma-quic]

This seems like a more general issue... some examples:

#define D foo bar
enum { D };

#define D foo bar
void f() { int a[2]; auto [D] = a; }

#define D <int!
template <class T> class X;
X D;

#define D C::{
class C { D }};

Can we come up with some kind of generalized solution?

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[a-tarasyuk]

Can we come up with some kind of generalized solution?

@efriedma-quic Introducing a general method like GetExpectedLocation to resolve the location makes sense. However, if we're talking about handling the diagnostics in one place, it could get messy — each case uses a different diagnostic with its own set of arguments and fix-its. WDYT?

[efriedma-quic]

I see two possible ways to approach this, in general:

• Come up with a method that actually figures out the end of the token inside the macro, instead of bailing on macros. I'm not sure we can provide a good location in edge cases involving token pasting, but we can probably do better that just immediately bailing on any macro.
• Add a variant of getLocForEndOfToken that lets you pass in a fallback location. The method would return that instead of an invalid token. Here, you can probably just use Tok.getLocation().

I think we normally suppress fixits in macros anyway, so you don't need to worry about places that pass the result of getLocForEndOfToken to a fixit.

[efriedma-quic]

LGTM

(It might be worth trying to audit other uses of getLocForEndOfToken; it looks like there are other dubious uses. But we don't need to do everything in one patch.)