[PATCH] D78338: [clangd] Enable diagnostic fixes within macro argument expansions.

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: hokein.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous, MaskRay, ilya-biryukov.
Herald added a project: clang.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78338

Files:
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp


Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -103,6 +103,7 @@
   // 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);};
@@ -115,6 +116,7 @@
       struct Foo { int x; }; Foo a;
       a.$nomember[[y]];
       test::$nomembernamespace[[test]];
+      $macro[[ID($macroarg[[fod]])]]();
     }
   )cpp");
   auto TU = TestTU::withCode(Test.code());
@@ -143,6 +145,10 @@
           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 "
Index: clang-tools-extra/clangd/Diagnostics.cpp
===================================================================
--- clang-tools-extra/clangd/Diagnostics.cpp
+++ clang-tools-extra/clangd/Diagnostics.cpp
@@ -556,10 +556,21 @@
     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.
+      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 +581,8 @@
 
     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);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D78338.258202.patch
Type: text/x-patch
Size: 3214 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200416/37650551/attachment.bin>