Add FixItHint for missing #include (err_module_unimported_use_header)

[sammccall]

Summary

The line is inserted using clang-format's logic with LLVM style.
Users must run clang-format afterwards if they want some other style.

Alternatives:

respect .clang-format: this would be surprising in the compiler
insert at offset 0: this is bad people who don't use clang-format
apply $simple_heuristic: clang-format handles lots of special cases for us

[sammccall]

(First patch, so please spell out mistakes!)

I think the big question is whether it's okay to depend on Format.

[sammccall]

Ping - let me know if there's a more appropriate reviewer!

[bruno]

Thanks for looking into this. It's a nice FixIt to have.

I don't see any dep cycle; clangFormat depends on clangToolingCore, which also depends clangRewrite, which means there are going to be 3 new dependencies for libSema in the end: clangToolingCore, clangRewrite and clangFormat.

I don't know the history behind the desired dependencies, I'll let others comment whether this is OK, but my guess it that it depends on the tradeoff, it's hard to justify 3 new deps for a change that is supposed to be simple. How hard is to implement this without using libFormat?

lib/Sema/SemaLookup.cpp

39

You also want clangToolingCore to be listed as a explicit dep since you're including from it.

[sammccall]

I don't know the history behind the desired dependencies, I'll let others comment whether this is OK, but my guess it that it depends on the tradeoff, it's hard to justify 3 new deps for a change that is supposed to be simple. How hard is to implement this without using libFormat?

What libFormat does is fairly complex:

• it parses header guards/file comments to avoid violating them
• it locates existing includes to insert nearby
• it is aware of "main header", and system vs user headers
• it avoids generating a fixit if the header is already included somewhere
• it preserves sort order within include blocks (I forgot to include this!)

This is fixCppIncludeInsertions in Format.cpp plus its helpers, and sortCppIncludes.

Altogether this looks like being around 500LOC that would be directly duplicated, plus some replacements for policy handled by LLVMStyle, plus tests (a new impl would need to be more thoroughly tested).
I also don't know what the costs of the dependency are.

ioeric: is my estimate about right?

lib/Sema/SemaLookup.cpp

4997

Hmm, I probably want to formatReplacements here for sorting...

[bkramer]

In D25311#574806, @bruno wrote:

I don't know the history behind the desired dependencies, I'll let others comment whether this is OK, but my guess it that it depends on the tradeoff, it's hard to justify 3 new deps for a change that is supposed to be simple. How hard is to implement this without using libFormat?

I think clang already depends on libclangRewrite. libclangToolingCore is tiny, it's basically just the Replacement class and a couple of utilities. While it would be possible to hoist just the include handling and its dependencies out of libclangFormat into a separate library I'm not convinced that it's worth it. libclangFormat is tiny, the .a file is less than a megabyte and the impact on the clang binary is 400kB in a Release+Asserts build, a 0.5% increase.

[rsmith]

In D25311#576382, @bkramer wrote:

I think clang already depends on libclangRewrite. libclangToolingCore is tiny, it's basically just the Replacement class and a couple of utilities. While it would be possible to hoist just the include handling and its dependencies out of libclangFormat into a separate library I'm not convinced that it's worth it. libclangFormat is tiny, the .a file is less than a megabyte and the impact on the clang binary is 400kB in a Release+Asserts build, a 0.5% increase.

Nonetheless, from a layering perspective, it doesn't make much sense for libSema to depend on libFormat. The functionality in question sounds like it belongs in Lex.

lib/Sema/CMakeLists.txt

add_clang_library(clangSema
   TypeLocBuilder.cpp
 
   LINK_LIBS
   clangAST
   clangAnalysis
   clangBasic
   clangEdit
   clangLex
+  clangFormat
   )

lib/Sema/SemaLookup.cpp

 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclLookups.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/Basic/Builtins.h"
 #include "clang/Basic/LangOptions.h"
+#include "clang/Format/Format.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/ModuleLoader.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Sema/DeclSpec.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/Overload.h"
 #include "clang/Sema/Scope.h"
 #include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/Sema.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/TemplateDeduction.h"
 #include "clang/Sema/TypoCorrection.h"
+#include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/TinyPtrVector.h"
 #include "llvm/ADT/edit_distance.h"
 #include "llvm/Support/ErrorHandling.h"
 #include <algorithm>
 #include <iterator>
 #include <list>

 static std::string getIncludeStringForHeader(Preprocessor &PP,
                                              const FileEntry *E) {
   bool IsSystem;
   auto Path =
       PP.getHeaderSearchInfo().suggestPathToFileForDiagnostics(E, &IsSystem);
   return (IsSystem ? '<' : '"') + Path + (IsSystem ? '>' : '"');
 }
 
+/// Hint where to insert a missing #include into the file.
+/// We reuse clang-format's header-insertion logic, always with LLVM-style.
+static FixItHint AddInclude(FileID File, SourceManager &SourceMgr,
+                            std::string &IncludeTarget) {
+  StringRef Code = SourceMgr.getBufferData(File);
+  auto StartOfFile = SourceMgr.getLocForStartOfFile(File);
+  tooling::Replacement Unplaced(SourceMgr.getFilename(StartOfFile), UINT_MAX, 0,
+                                "#include " + IncludeTarget);
+  auto Reps = cleanupAroundReplacements(Code, tooling::Replacements(Unplaced),
+                                        format::getLLVMStyle());
+  if (Reps)
+    for (const auto &Placed : *Reps) {
+      auto Begin = StartOfFile.getLocWithOffset(Placed.getOffset());
+      auto End = Begin.getLocWithOffset(Placed.getLength());
+      return FixItHint::CreateReplacement(SourceRange(Begin, End),
+                                          Placed.getReplacementText());
+    }
+  return FixItHint();
+}
+
 void Sema::diagnoseMissingImport(SourceLocation UseLoc, NamedDecl *Decl,
                                  SourceLocation DeclLoc,
                                  ArrayRef<Module *> Modules,
                                  MissingImportKind MIK, bool Recover) {
   assert(!Modules.empty());
 
   if (Modules.size() > 1) {
     std::string ModuleList;
     unsigned N = 0;
     for (Module *M : Modules) {
       ModuleList += "\n        ";
       if (++N == 5 && N != Modules.size()) {
         ModuleList += "[...]";
         break;
       }
       ModuleList += M->getFullModuleName();
     }
 
     Diag(UseLoc, diag::err_module_unimported_use_multiple)
       << (int)MIK << Decl << ModuleList;
   } else if (const FileEntry *E =
                  PP.getModuleHeaderToIncludeForDiagnostics(UseLoc, DeclLoc)) {
     // The right way to make the declaration visible is to include a header;
     // suggest doing so.
-    //
-    // FIXME: Find a smart place to suggest inserting a #include, and add
-    // a FixItHint there.
+    std::string Include = getIncludeStringForHeader(PP, E);
     Diag(UseLoc, diag::err_module_unimported_use_header)
-      << (int)MIK << Decl << Modules[0]->getFullModuleName()
-      << getIncludeStringForHeader(PP, E);
+        << (int)MIK << Decl << Modules[0]->getFullModuleName() << Include
+        << AddInclude(SourceMgr.getFileID(UseLoc), SourceMgr, Include);
   } else {
     // FIXME: Add a FixItHint that imports the corresponding module.
     Diag(UseLoc, diag::err_module_unimported_use)
       << (int)MIK << Decl << Modules[0]->getFullModuleName();
   }
 
   unsigned DiagID;
   switch (MIK) {

test/Modules/suggest-include.cpp

 // RUN: rm -rf %t
 // RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -fimplicit-module-maps -I%S/Inputs/suggest-include %s -verify
+// RUN: not %clang_cc1 -fmodules -fmodules-cache-path=%t -fimplicit-module-maps -I%S/Inputs/suggest-include %s -fdiagnostics-parseable-fixits 2>&1 | FileCheck --match-full-lines --implicit-check-not "fix-it:{{.*}}" %s
 
 #include "empty.h" // import the module file
+// CHECK: fix-it:"{{.*}}":{[[@LINE]]:1-[[@LINE]]:84}:"#include \"usetextual1.h\"\n"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:1-[[@LINE-1]]:84}:"#include \"usetextual2.h\"\n"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:1-[[@LINE-2]]:84}:"#include \"usetextual3.h\"\n"
 
 // expected-note@usetextual1.h:2 {{previous}}
 // expected-note@textual2.h:1 {{previous}}
 // expected-note@textual3.h:1 {{previous}}
 // expected-note@textual4.h:1 {{previous}}
 // expected-note@textual5.h:1 {{previous}}
 // expected-note@private1.h:1 {{previous}}
 // expected-note@private2.h:1 {{previous}}