[clangd] Enable diagnostic fixes within macro argument expansions.

[sammccall]

Summary

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

[hokein]

clang-tools-extra/clangd/Diagnostics.cpp

563

I feel a bit nervous about this (even it is for macro-arg expansion only), as macro is very tricky.

I think we may result in invalid code after applying the fixits in some cases:

1. if the fix is to remove an unused variable (interestingly, clang doesn't provide fixit to remove an unused variable, but for unused lambda capture, it does)

#define LA(arg1, arg2) [arg1, arg2] { return arg2;}
void test1(int x, int y) {
  LA(x, y); // after fixit, it would become LA(, y)? or LA(y)
}

2. if the fix is to add some characters to the macro argument, e.g. adding a dereference *, the semantic of the code after macro expansion maybe changed.

void f1(int &a);
void f2(int *a) {
   f1(a); // clang will emits a diagnostic with a fixit adding preceding a `*` to a.
}

maybe we should be more conservative? just whitelist some diagnostics? fixing typos seems pretty safe.

[sammccall]

clang-tools-extra/clangd/Diagnostics.cpp

563

your test1 example doesn't trigger this case because the fix has to delete a comma that's provided by the macro body - this patch doesn't change behavior.

To construct an example that follows this schema:

struct S { S(int *x); };
int *x;
S s1(*x); // fixit -> S s1(x);
#define CONCAT(a,b) a b
S s2(CONCAT(*, x)); // fixit -> S s2(CONCAT(, x));

The fixed code compiles fine and addresses the error in the expected way. It may not be *idiomatic*, but this is also a pathological case. I think it's at least as good to offer the fix in this case, and certainly it's not a good reason to drop support for others..

---

void f1(int &a);

I can't follow this example, there are no macros?
Why would the insertion change semantics?

---

maybe we should be more conservative? just whitelist some diagnostics? fixing typos seems pretty safe.

I think this leaves a lot of value on the table - we've been conservative so far.
The problem with whitelists is they're incomplete and outdated (e.g. we have a whitelist for include fixer that's very incomplete, and I haven't managed to get around to fixing it, and neither has anyone else).
So I think we should use this (or a blacklist) only if we can show this plausibly causes real problems.

(To put this another way: by being too aggressive we'll get more feedback, by being more conservative we'll continue to get none)

[hokein]

clang-tools-extra/clangd/Diagnostics.cpp

563

your test1 example doesn't trigger this case because the fix has to delete a comma that's provided by the macro body - this patch doesn't change behavior.

ah, you are right.

I can't follow this example, there are no macros?
Why would the insertion change semantics?

sorry, the example was incomplete, the case is like

int f1(int &a);
#define ABC(x) *x + f1(x);
void f2(int *a) {
  ABC(a); // fixit -> ABC(*a), incorrect for the `*x` in macro body.
}

if the macro argument is being used in multiple places of the macro body, it probably leads to problems. I suspect this is common in practice, we should not allow fixit in this case.

think this leaves a lot of value on the table - we've been conservative so far.

fair enough.

[sammccall]

clang-tools-extra/clangd/Diagnostics.cpp

563

if the macro argument is being used in multiple places of the macro body, it probably leads to problems. I suspect this is common in practice, we should not allow fixit in this case.

This is definitely true.

As discussed offline, this is mitigated by:

• stringified expansions are common, but don't really count for this purpose
• fix will often make sense for all occurrences (we can't detect this), in which case not offering it is worse
• remaining cases where fix makes sense for one case and not others probably aren't that numerous
• behaviour in this case is to apply the fix and diagnose the resulting error from the other expansion, which isn't that terrible (as far as macro diagnostic experience goes)

Agreed to do it anyway and wait for feedback, I'll document this though.

clang-tools-extra/clangd/Diagnostics.cpp

void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
   };
 
   auto AddFix = [&](bool SyntheticMessage) -> bool {
     assert(!Info.getFixItHints().empty() &&
            "diagnostic does not have attached fix-its");
     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;
       if (!isInsideMainFile(FixIt.RemoveRange.getBegin(), SM))
         return false;
       Edits.push_back(toTextEdit(FixIt, SM, *LangOpts));
     }
 
     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);
       llvm::StringRef Insert = FixIt.CodeToInsert;
       if (!Invalid) {
         llvm::raw_svector_ostream M(Message);
         if (!Remove.empty() && !Insert.empty()) {
           M << "change '";

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp

Position pos(int line, int character) {
   Res.character = character;
   return Res;
 }
 
 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);};
     int main() {
       $typo[[go\
 o]]();
       foo()$semicolon[[]]//with comments
       $unk[[unknown]]();
       double $type[[bar]] = "foo";
       struct Foo { int x; }; Foo a;
       a.$nomember[[y]];
       test::$nomembernamespace[[test]];
+      $macro[[ID($macroarg[[fod]])]]();
     }
   )cpp");
   auto TU = TestTU::withCode(Test.code());
   TU.ClangTidyChecks = "-*,google-explicit-constructor";
   EXPECT_THAT(
       TU.build().getDiagnostics(),
       ElementsAre(
           // This range spans lines.
EXPECT_THAT(
           // This range isn't provided by clang, we expand to the token.
           Diag(Test.range("unk"), "use of undeclared identifier 'unknown'"),
           Diag(Test.range("type"),
                "cannot initialize a variable of type 'double' with an lvalue "
                "of type 'const char [4]'"),
           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 "
                      "avoid unintentional implicit conversions"),
                 WithFix(Fix(Test.range("explicit"), "explicit ",
                             "insert 'explicit '")))));
 }