[clangd] IncludeCleaner: Attach "insert prgama keep" fix-it to diagnostic

[kbobyrev]

Summary

Now that IWYU pragma: keep supresses the warning for unused header, it should
be possible to suggest the user both removing the header and adding a pragma to
keep it instead.

[sammccall]

Nice!

clang-tools-extra/clangd/IncludeCleaner.cpp

212

I think we should be a bit more direct about the *content* that we're matching here, rather than the coordinates.

e.g. the current logic is broken in various ways if the directive is split over physical lines, and dealing with coordinates makes it both hard to fix and detect.

And it seems like you're going to offer a fix erasing any existing trailing comment.

Possibly there are other unexpected patterns where the code might get mangled too.

Maybe more like:

StringRef RestOfLine = Code.substr(HashOffset).take_until('\n' || '\r');
if (!RestOfLine.consume_front("#"))
 return fail;
RestOfLine = RestOfLine.ltrim();
// and consume the rest of the stuff we expect on the line
if (RestOfLine.ltrim().empty()) {
  return {
    offsetToPosition(Code, RestOfLine.begin() - Code.data(),
    offsetToPosition(Code, RestOfLine.end() - Code.data(),
  };
}

223

this is incorrect for

#include \
  <foo.h>

(Happy if it works or rejects this case, but it shouldn't silently do the wrong thing.)

227

I'd expect error cases to be handled somehow, the fact that this function always succeeds is suspicious

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

1616

nit: insert->pragma_keep?

[kbobyrev]

Address review comments.

clang-tools-extra/clangd/IncludeCleaner.cpp

clangd::Range getDiagnosticRange(llvm::StringRef Code, unsigned HashOffset) {
   // Span the warning until the EOL or EOF.
   Result.end.character +=
       lspLength(Code.drop_front(HashOffset).take_until([](char C) {
         return C == '\n' || C == '\r';
       }));
   return Result;
 }
 
+// Returns the range starting right after the included header name and ending at
+// EOL.
+clangd::Range getIWYUPragmaRange(llvm::StringRef Code, unsigned HashOffset) {
+  clangd::Range Result;
+  Result.end = Result.start = offsetToPosition(Code, HashOffset);
+
+  unsigned QuotesCount = 0;
+  Result.start.character +=
+      lspLength(Code.drop_front(HashOffset).take_until([&QuotesCount](char C) {
+        if (C == '"')
+          if (++QuotesCount == 2)
+            return true;
+        return C == '>';
+      })) +
+      1;
+  Result.end.character +=
+      lspLength(Code.drop_front(HashOffset).take_until([](char C) {
+        return C == '\n' || C == '\r';
+      }));
+  return Result;
+}
+
 // Finds locations of macros referenced from within the main file. That includes
 // references that were not yet expanded, e.g `BAR` in `#define FOO BAR`.
 void findReferencedMacros(ParsedAST &AST, ReferencedLocations &Result) {
   trace::Span Tracer("IncludeCleaner::findReferencedMacros");
   auto &SM = AST.getSourceManager();
   auto &PP = AST.getPreprocessor();
   // FIXME(kirillbobyrev): The macros from the main file are collected in
   // ParsedAST's MainFileMacros. However, we can't use it here because it
for (const auto *Inc : computeUnusedIncludes(AST)) {
     D.Source = Diag::DiagSource::Clangd;
     D.File = FileName;
     D.Severity = DiagnosticsEngine::Warning;
     D.Tags.push_back(Unnecessary);
     D.Range = getDiagnosticRange(Code, Inc->HashOffset);
     // FIXME(kirillbobyrev): Removing inclusion might break the code if the
     // used headers are only reachable transitively through this one. Suggest
     // including them directly instead.
-    // FIXME(kirillbobyrev): Add fix suggestion for adding IWYU pragmas
-    // (keep/export) remove the warning once we support IWYU pragmas.
     D.Fixes.emplace_back();
     D.Fixes.back().Message = "remove #include directive";
     D.Fixes.back().Edits.emplace_back();
     D.Fixes.back().Edits.back().range.start.line = Inc->HashLine;
     D.Fixes.back().Edits.back().range.end.line = Inc->HashLine + 1;
     D.InsideMainFile = true;
+    // Allow suppressing the warning through adding keep pragma.
+    // NOTE: This would also erase the comment after the header inclusion if
+    // there is one.
+    D.Fixes.emplace_back();
+    D.Fixes.back().Message = "insert IWYU pragma: keep";
+    D.Fixes.back().Edits.emplace_back();
+    D.Fixes.back().Edits.back().range =
+        getIWYUPragmaRange(Code, Inc->HashOffset);
+    D.InsideMainFile = true;
+    D.Fixes.back().Edits.back().newText = " // IWYU pragma: keep";
+    D.InsideMainFile = true;
     Result.push_back(std::move(D));
   }
   return Result;
 }
 
 } // namespace clangd
 } // namespace clang

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

EXPECT_THAT(*TU.build().getDiagnostics(),
                   AllOf(Diag(Test.range("unused"), "unused variable 'x'"),
                         WithTag(DiagnosticTag::Unnecessary)),
                   AllOf(Diag(Test.range("deprecated"), "'bar' is deprecated"),
                         WithTag(DiagnosticTag::Deprecated))));
 }
 
 TEST(DiagnosticsTest, IncludeCleaner) {
   Annotations Test(R"cpp(
-$fix[[  $diag[[#include "unused.h"]]
+$remove[[  $diag[[#include "unused.h"]]$insert[[]]
 ]]
   #include "used.h"
 
   #include <system_header.h>
 
   void foo() {
     used();
   }
]]
   Config Cfg;
   Cfg.Diagnostics.UnusedIncludes = Config::UnusedIncludesPolicy::Strict;
   WithContextValue WithCfg(Config::Key, std::move(Cfg));
   EXPECT_THAT(
       *TU.build().getDiagnostics(),
       UnorderedElementsAre(AllOf(
           Diag(Test.range("diag"), "included header unused.h is not used"),
           WithTag(DiagnosticTag::Unnecessary), DiagSource(Diag::Clangd),
-          WithFix(Fix(Test.range("fix"), "", "remove #include directive")))));
+          WithFix(Fix(Test.range("remove"), "", "remove #include directive"),
+                  Fix(Test.range("insert"), " // IWYU pragma: keep",
+                      "insert IWYU pragma: keep")))));
   Cfg.Diagnostics.SuppressAll = true;
   WithContextValue SuppressAllWithCfg(Config::Key, std::move(Cfg));
   EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
   Cfg.Diagnostics.SuppressAll = false;
   Cfg.Diagnostics.Suppress = {"unused-includes"};
   WithContextValue SuppressFilterWithCfg(Config::Key, std::move(Cfg));
   EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang