From 6529b0c48aab83bdada1d21a79da13b0971d1aec Mon Sep 17 00:00:00 2001
From: Sam McCall <sam.mccall@gmail.com>
Date: Fri, 17 Apr 2020 01:38:42 +0200
Subject: [PATCH] [clangd] Enable diagnostic fixes within macro argument
 expansions.

Summary: This seems like a pretty safe case, and common enough to be useful.

Reviewers: hokein

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, usaxena95, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D78338
---
 clang-tools-extra/clangd/Diagnostics.cpp           | 23 +++++++++++++++++-----
 .../clangd/unittests/DiagnosticsTests.cpp          |  6 ++++++
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp
index 32f6a9b..d72c2bd 100644
--- a/clang-tools-extra/clangd/Diagnostics.cpp
+++ b/clang-tools-extra/clangd/Diagnostics.cpp
@@ -556,10 +556,23 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
     if (!InsideMainFile)
       return false;
 
+    // Copy as we may modify the ranges.
+    auto FixIts = Info.getFixItHints().vec();
     llvm::SmallVector<TextEdit, 1> Edits;
-    for (auto &FixIt : Info.getFixItHints()) {
-      // Follow clang's behavior, don't apply FixIt to the code in macros,
-      // we are less certain it is the right fix.
+    for (auto &FixIt : FixIts) {
+      // Allow fixits within a single macro-arg expansion to be applied.
+      // This can be incorrect if the argument is expanded multiple times in
+      // different contexts. Hopefully this is rare!
+      if (FixIt.RemoveRange.getBegin().isMacroID() &&
+          FixIt.RemoveRange.getEnd().isMacroID() &&
+          SM.getFileID(FixIt.RemoveRange.getBegin()) ==
+              SM.getFileID(FixIt.RemoveRange.getEnd())) {
+        FixIt.RemoveRange = CharSourceRange(
+            {SM.getTopMacroCallerLoc(FixIt.RemoveRange.getBegin()),
+             SM.getTopMacroCallerLoc(FixIt.RemoveRange.getEnd())},
+            FixIt.RemoveRange.isTokenRange());
+      }
+      // Otherwise, follow clang's behavior: no fixits in macros.
       if (FixIt.RemoveRange.getBegin().isMacroID() ||
           FixIt.RemoveRange.getEnd().isMacroID())
         return false;
@@ -570,8 +583,8 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
 
     llvm::SmallString<64> Message;
     // If requested and possible, create a message like "change 'foo' to 'bar'".
-    if (SyntheticMessage && Info.getNumFixItHints() == 1) {
-      const auto &FixIt = Info.getFixItHint(0);
+    if (SyntheticMessage && FixIts.size() == 1) {
+      const auto &FixIt = FixIts.front();
       bool Invalid = false;
       llvm::StringRef Remove =
           Lexer::getSourceText(FixIt.RemoveRange, SM, *LangOpts, &Invalid);
diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
index f64f8e9..026e145 100644
--- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -104,6 +104,7 @@ TEST(DiagnosticsTest, DiagnosticRanges) {
   // Check we report correct ranges, including various edge-cases.
   Annotations Test(R"cpp(
     // error-ok
+    #define ID(X) X
     namespace test{};
     void $decl[[foo]]();
     class T{$explicit[[]]$constructor[[T]](int a);};
@@ -116,6 +117,7 @@ o]]();
       struct Foo { int x; }; Foo a;
       a.$nomember[[y]];
       test::$nomembernamespace[[test]];
+      $macro[[ID($macroarg[[fod]])]]();
     }
   )cpp");
   auto TU = TestTU::withCode(Test.code());
@@ -144,6 +146,10 @@ o]]();
           Diag(Test.range("nomember"), "no member named 'y' in 'Foo'"),
           Diag(Test.range("nomembernamespace"),
                "no member named 'test' in namespace 'test'"),
+          AllOf(Diag(Test.range("macro"),
+                     "use of undeclared identifier 'fod'; did you mean 'foo'?"),
+                WithFix(Fix(Test.range("macroarg"), "foo",
+                            "change 'fod' to 'foo'"))),
           // We make sure here that the entire token is highlighted
           AllOf(Diag(Test.range("constructor"),
                      "single-argument constructors must be marked explicit to "
-- 
2.7.4