[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–1618

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.
+llvm::Optional<clangd::Range> getIWYUPragmaRange(llvm::StringRef Code,
+                                                 unsigned HashOffset) {
+  // Attempt to parse the string: it should be a simple one-line inclusion
+  // without a trailing comment. Other formats are not supported.
+  StringRef RestOfLine = Code.substr(HashOffset).take_until([](char C) {
+    return C == '\n' || C == '\r';
+  });
+  if (!RestOfLine.consume_front("#"))
+    return None;
+  RestOfLine = RestOfLine.ltrim();
+  if (!RestOfLine.consume_front("include"))
+    return None;
+  RestOfLine = RestOfLine.ltrim();
+  if (!RestOfLine.consume_front("<") && !RestOfLine.consume_front("\""))
+    return None;
+  RestOfLine =
+      RestOfLine.drop_until([](char C) { return C == '>' || C == '"'; });
+  if (RestOfLine.startswith("\"") || RestOfLine.startswith(">"))
+    RestOfLine = RestOfLine.drop_front();
+  if (!RestOfLine.ltrim().empty())
+    return None;
+  return clangd::Range{
+      offsetToPosition(Code, RestOfLine.begin() - Code.data()),
+      offsetToPosition(Code, RestOfLine.begin() - Code.data())};
+}
+
 // 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. The FixIt will
+    // be offered for lines with no extra comment at the end of the line that
+    // have the full name of an included header.
+    if (auto PragmaRange = getIWYUPragmaRange(Code, Inc->HashOffset)) {
+      D.Fixes.emplace_back();
+      D.Fixes.back().Message = "insert IWYU pragma: keep";
+      D.Fixes.back().Edits.emplace_back();
+      D.Fixes.back().Edits.back().range = *PragmaRange;
+      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"]]$pragma_keep[[]]
+]]
+$remove_no_fixit[[  $diag_no_fixit[[#include "no_fixit.h" // Has comment: don't offer IWYU pragma keep FixIt!]]
 ]]
   #include "used.h"
 
   #include <system_header.h>
 
   void foo() {
     used();
   }
   )cpp");
   TestTU TU;
   TU.Code = Test.code().str();
   TU.AdditionalFiles["unused.h"] = R"cpp(
     #pragma once
     void unused() {}
   )cpp";
+  TU.AdditionalFiles["no_fixit.h"] = R"cpp(
+    #pragma once
+    void no_fixit() {}
+  )cpp";
   TU.AdditionalFiles["used.h"] = R"cpp(
     #pragma once
     void used() {}
   )cpp";
   TU.AdditionalFiles["system/system_header.h"] = "";
   TU.ExtraArgs = {"-isystem" + testPath("system")};
   // Off by default.
   EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
   Config Cfg;
   Cfg.Diagnostics.UnusedIncludes = Config::UnusedIncludesPolicy::Strict;
   WithContextValue WithCfg(Config::Key, std::move(Cfg));
   EXPECT_THAT(
       *TU.build().getDiagnostics(),
-      UnorderedElementsAre(AllOf(
+      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("pragma_keep"), " // IWYU pragma: keep",
+                      "insert IWYU pragma: keep"))),
+          AllOf(Diag(Test.range("diag_no_fixit"),
+                     "included header no_fixit.h is not used"),
+                WithTag(DiagnosticTag::Unnecessary), DiagSource(Diag::Clangd),
+                WithFix(Fix(Test.range("remove_no_fixit"), "",
+                            "remove #include directive")))));
   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