[clangd] Add fixes for clang "include <foo.h>" diagnostics

[sammccall]

Summary

Clang doesn't offer these fixes I guess for a couple of reasons:

• where to insert includes is a formatting concern, and clang shouldn't depend on clang-format
• the way clang prints diagnostics, we'd show a bunch of basically irrelevant context of "this is where we'd want to insert the include"

Maybe it's possible to hack around 1, but 2 is still a concern.
Meanwhile, bolting this onto include-fixer gets the job done.

Fixes https://github.com/clangd/clangd/issues/355
Fixes https://github.com/clangd/clangd/issues/937

[nridge]

Thanks, this is nice low-hanging fruit.

clang-tools-extra/clangd/Diagnostics.cpp

830

Do we maybe want to assert Info.getFixItHints().empty() here, so that if clang later introduces its own fix-it it gets on our radar and we can decide which one to prefer?

clang-tools-extra/clangd/IncludeFixer.cpp

52

nit: Index or Idx?

495

In the note_include_header_or_declare case, we could pull the second arg from the diagnostic (the name of the symbol) and use it to provide the slightly more detailed hint description that fixUnresolvedName() does. Up to you if you think that's worth doing.

clang-tools-extra/clangd/IncludeFixer.h

43

The added comment describes the behaviour of calling code, not the behaviour of this method. Perhaps it would make more sense in the calling code?

[sammccall]

clang-tools-extra/clangd/IncludeFixer.cpp

495

Good point. I shuffled around some code so these strings won't go out of sync.

And they were ("Add include X" vs "Include X") so being stubborn I changed all the existing ones to add match this one (since "include" isn't really a noun)

clang-tools-extra/clangd/IncludeFixer.h

43

Oops, the intent was to document the semantics of returned values.

Reworded and also added docs at contributeFixes.

clang-tools-extra/clangd/Diagnostics.cpp

if (!LastDiag) {
       return;
     }
 
     // If a diagnostic was suppressed due to the suppression filter,
     // also suppress notes associated with it.
     if (LastDiag->Severity == DiagnosticsEngine::Ignored)
       return;
 
+    // Give include-fixer a chance to replace a note with a fix.
+    if (Fixer) {
+      auto ReplacementFixes = Fixer(LastDiag->Severity, Info);
+      if (!ReplacementFixes.empty()) {
+        LastDiag->Fixes.insert(LastDiag->Fixes.end(), ReplacementFixes.begin(),
+                               ReplacementFixes.end());
+        return;
+      }
+    }
+
     if (!Info.getFixItHints().empty()) {
       // A clang note with fix-it is not a separate diagnostic in clangd. We
       // attach it as a Fix to the main diagnostic instead.
       if (!AddFix(false /* use the note as the message */))
         IgnoreDiagnostics::log(DiagLevel, Info);
     } else {
       // A clang note without fix-its corresponds to clangd::Note.
       Note N;

clang-tools-extra/clangd/IncludeFixer.h

 class IncludeFixer {
 public:
   IncludeFixer(llvm::StringRef File, std::shared_ptr<IncludeInserter> Inserter,
                const SymbolIndex &Index, unsigned IndexRequestLimit)
       : File(File), Inserter(std::move(Inserter)), Index(Index),
         IndexRequestLimit(IndexRequestLimit) {}
 
   /// Returns include insertions that can potentially recover the diagnostic.
+  /// If Info describes a note, it will be replaced by any returned fixes.
   std::vector<Fix> fix(DiagnosticsEngine::Level DiagLevel,
                        const clang::Diagnostic &Info) const;
 
   /// Returns an ExternalSemaSource that records failed name lookups in Sema.
   /// This allows IncludeFixer to suggest inserting headers that define those
   /// names.
   llvm::IntrusiveRefCntPtr<ExternalSemaSource> unresolvedNameRecorder();
 
 private:
   /// Attempts to recover diagnostic caused by an incomplete type \p T.
   std::vector<Fix> fixIncompleteType(const Type &T) const;
 
   /// Generates header insertion fixes for all symbols. Fixes are deduplicated.
   std::vector<Fix> fixesForSymbols(const SymbolSlab &Syms) const;
 
+  std::vector<Fix> fixMissingHeader(llvm::StringRef Name) const;
+
   struct UnresolvedName {
     std::string Name;   // E.g. "X" in foo::X.
     SourceLocation Loc; // Start location of the unresolved name.
     std::vector<std::string> Scopes; // Namespace scopes we should search in.
   };
 
   /// Records the last unresolved name seen by Sema.
   class UnresolvedNameRecorder;

clang-tools-extra/clangd/IncludeFixer.cpp

 #include "llvm/Support/FormatVariadic.h"
 #include <algorithm>
 #include <set>
 #include <string>
 #include <vector>
 
 namespace clang {
 namespace clangd {
+namespace {
+llvm::Optional<llvm::StringRef> getArgStr(const clang::Diagnostic &Info,
+                                          unsigned I) {
+  switch (Info.getArgKind(I)) {
+  case DiagnosticsEngine::ak_c_string:
+    return llvm::StringRef(Info.getArgCStr(I));
+  case DiagnosticsEngine::ak_std_string:
+    return llvm::StringRef(Info.getArgStdStr(I));
+  default:
+    return llvm::None;
+  }
+}
+} // namespace
 
 std::vector<Fix> IncludeFixer::fix(DiagnosticsEngine::Level DiagLevel,
                                    const clang::Diagnostic &Info) const {
   switch (Info.getID()) {
   case diag::err_incomplete_nested_name_spec:
   case diag::err_incomplete_base_class:
   case diag::err_incomplete_member_access:
   case diag::err_incomplete_type:
if (LastUnresolvedName) {
       //                      ~~~~~~~~~~~~~
       //                      UnresolvedName
       // We only attempt to recover a diagnostic if it has the same location as
       // the last seen unresolved name.
       if (DiagLevel >= DiagnosticsEngine::Error &&
           LastUnresolvedName->Loc == Info.getLocation())
         return fixUnresolvedName();
     }
+    break;
+  // Cases where clang explicitly knows which header to include.
+  // (There's no fix provided for boring formatting reasons).
+  case diag::err_implied_std_initializer_list_not_found:
+    return fixMissingHeader("<initializer_list>");
+  case diag::err_need_header_before_typeid:
+    return fixMissingHeader("<typeid>");
+  case diag::err_need_header_before_ms_uuidof:
+    return fixMissingHeader("<guiddef.h>");
+  case diag::err_need_header_before_placement_new:
+  case diag::err_implicit_coroutine_std_nothrow_type_not_found:
+    return fixMissingHeader("<new>");
+  case diag::err_omp_implied_type_not_found:
+  case diag::err_omp_interop_type_not_found:
+    return fixMissingHeader("<omp.h>");
+  case diag::err_implied_coroutine_type_not_found:
+    return fixMissingHeader("<coroutine>");
+  case diag::err_implied_comparison_category_type_not_found:
+    return fixMissingHeader("<compare>");
+  case diag::note_include_header_or_declare:
+    if (Info.getNumArgs() > 0)
+      if (auto Header = getArgStr(Info, 0))
+        return fixMissingHeader(("<" + *Header + ">").str());
+    break;
   }
+
   return {};
 }
 
 std::vector<Fix> IncludeFixer::fixIncompleteType(const Type &T) const {
   // Only handle incomplete TagDecl type.
   const TagDecl *TD = T.getAsTagDecl();
   if (!TD)
     return {};
if (!Syms.empty()) {
     if (!Matched.IncludeHeaders.empty() && Matched.Definition &&
         Matched.CanonicalDeclaration.FileURI == Matched.Definition.FileURI)
       Fixes = fixesForSymbols(Syms);
   }
   auto E = LookupCache.try_emplace(ID, std::move(Syms));
   return &E.first->second;
 }
 
+std::vector<Fix> IncludeFixer::fixMissingHeader(llvm::StringRef Name) const {
+  if (auto Edit = Inserter->insert(Name))
+    return {Fix{llvm::formatv("Include {0}", Name).str(), {std::move(*Edit)}}};
+  return {};
+}
+
 } // namespace clangd
 } // namespace clang

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

EXPECT_THAT(*TU.build().getDiagnostics(),
                   Diag(Test.range("unqualified"), "unknown type name 'X'"),
                   DiagName("unknown_typename"),
                   WithFix(Fix(Test.range("insert"), "#include \"a.h\"\n",
                               "Add include \"a.h\" for symbol na::X"),
                           Fix(Test.range("insert"), "#include \"b.h\"\n",
                               "Add include \"b.h\" for symbol na::nb::X")))));
 }
 
-TEST(IncludeFixerTest, NoCrashMemebrAccess) {
+TEST(IncludeFixerTest, NoCrashMemberAccess) {
   Annotations Test(R"cpp(// error-ok
     struct X { int  xyz; };
     void g() { X x; x.$[[xy]]; }
   )cpp");
   auto TU = TestTU::withCode(Test.code());
   auto Index = buildIndexWithSymbol(
       SymbolWithHeader{"na::X", "unittest:///a.h", "\"a.h\""});
   TU.ExternalIndex = Index.get();
TEST(IncludeFixerTest, NoCrashOnTemplateInstantiations) {
   auto Index = buildIndexWithSymbol({});
   TU.ExternalIndex = Index.get();
 
   EXPECT_THAT(
       *TU.build().getDiagnostics(),
       ElementsAre(Diag(Test.range(), "use of undeclared identifier 'a'")));
 }
 
+TEST(IncludeFixerTest, HeaderNamedInDiag) {
+  Annotations Test(R"cpp(
+    $insert[[]]int main() {
+      [[printf]]("");
+    }
+  )cpp");
+  auto TU = TestTU::withCode(Test.code());
+  TU.ExtraArgs = {"-xc"};
+  auto Index = buildIndexWithSymbol({});
+  TU.ExternalIndex = Index.get();
+
+  EXPECT_THAT(
+      *TU.build().getDiagnostics(),
+      ElementsAre(AllOf(
+          Diag(Test.range(), "implicitly declaring library function 'printf' "
+                             "with type 'int (const char *, ...)'"),
+          WithFix(Fix(Test.range("insert"), "#include <stdio.h>\n",
+                      "Include <stdio.h>")))));
+}
+
 TEST(DiagsInHeaders, DiagInsideHeader) {
   Annotations Main(R"cpp(
     #include [["a.h"]]
     void foo() {})cpp");
   Annotations Header("[[no_type_spec]]; // error-ok");
   TestTU TU = TestTU::withCode(Main.code());
   TU.AdditionalFiles = {{"a.h", std::string(Header.code())}};
   EXPECT_THAT(*TU.build().getDiagnostics(),