[clang][Tooling] Add support for generating #import edits

[dgoldman]

Summary

And make use of this from clangd's CodeComplete and IncludeFixer, although currently they are both restricted only to #include symbols.

[dgoldman]

Rebase

[dgoldman]

Run clang-format

[kadircet]

we should have tests in clang/unittests/Tooling/HeaderIncludesTest.cpp and the commit itself should be tagged as [clang][Tooling] rather than [clangd].

clang-tools-extra/clangd/Headers.h

250

again it's better to have an enum here.

clang-tools-extra/clangd/IncludeFixer.cpp

253

again can we rather pass the directive enum around?

clang/include/clang/Tooling/Inclusions/HeaderIncludes.h

74

rather than a boolean flag, can you introduce an enum IncludeDirective { Include, Import }; and default it to Include here?

86

we also need to preserve includedirective here now. as the behaviour of insert is to return none iff it's exactly the same spelling. so if there's a #include "foo.h" and we want to insert #import "foo.h" it shouldn't fail.

[dgoldman]

Fixes for review

[dgoldman]

Rebase

[kadircet]

thanks, mostly LG. some small changes.

clang-tools-extra/clangd/IncludeFixer.cpp

261

nit: llvm::StringLiteral DirectiveSpelling = Directive == tooling::IncludeDirective::Include ? "Include" : "Import"; and use that inside formatv calls

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

339

nit: ASSERT_TRUE, here and above.

clang/include/clang/Tooling/Inclusions/HeaderIncludes.h

47

this will leak enum values to the whole clang::tooling namespace. can you make this an enum class IncludeDirective instead (sorry for forgetting that in the initial comment)

75

i don't think we have many callers here. can we just update them instead of defaulting to Include here?

76

s/#includes/#includes and #imports/

clang/lib/Tooling/Inclusions/HeaderIncludes.cpp

300

i know this is not possible today, but just to be safe, can we actually check for import here and default to include ?

352

nit: can you put some braces around the outermost if, as the statements inside are getting long.

375

nit:

llvm::StringLiteral DirectiveSpelling = Directive == IncludeDirective::Include ? "include" : "import";
std::string NewInclude = llvm::formatv("#{0} {1}\n", DirectiveSpelling, QuotedName); // this already has an implicit conversion operator to std::string

clang/unittests/Tooling/HeaderIncludesTest.cpp

65 ↗

(On Diff #479665)

can you also add a removal test?

i know we didn't change the implementation, but today there's nothing checking the behaviour. so explicitly encoding the expectation here, especially for the case where we have both #include and #import with same spelling is useful. btw is it likely that #include "foo.h" and #import "foo.h" can refer to different physical headers (e.g. due to framework handling and whatnot)?

we're not changing the behaviour today, but if there's a difference, this is likely a bug (that already exists), so we should leave a fixme (or address it here if you got time, by introducing a IncludeDirective parameter into remove. updating users is likely harder in this case, as everyone needs to propagate/store the directive during their analysis).

[dgoldman]

Fixes for review + fix diffbase

clang/unittests/Tooling/HeaderIncludesTest.cpp

65 ↗

(On Diff #479665)

No, I don't think #import and #include can refer to different headers.

[kadircet]

thanks!

clang/unittests/Tooling/HeaderIncludesTest.cpp

77 ↗

(On Diff #480482)

nit: feel free to use raw string literals here and in other tests (or angled (<) includes), e.g:

llvm::StringRef Code = R"cpp(
  #include <abc.h>
  #import <abc.h>
  int x;)cpp";
EXPECT_EQ("int x;", remove(Code, "<abc.h>"));

81 ↗

(On Diff #480482)

nit: just inline "int x" to the LHS.

[dgoldman]

Use raw string literal

clang-tools-extra/clangd/CodeComplete.cpp

for (const auto &Inc : C.RankedIncludeHeaders) {
       // FIXME: We should support #import directives here.
       if ((Inc.Directive & clang::clangd::Symbol::Include) == 0)
         continue;
 
       if (auto ToInclude = Inserted(Inc.Header)) {
         CodeCompletion::IncludeCandidate Include;
         Include.Header = ToInclude->first;
         if (ToInclude->second && ShouldInsert)
-          Include.Insertion = Includes.insert(ToInclude->first);
+          Include.Insertion = Includes.insert(
+              ToInclude->first, Inc.Directive == Symbol::Import);
         Completion.Includes.push_back(std::move(Include));
       } else
         log("Failed to generate include insertion edits for adding header "
             "(FileURI='{0}', IncludeHeader='{1}') into {2}: {3}",
             C.IndexResult->CanonicalDeclaration.FileURI, Inc.Header, FileName,
             ToInclude.takeError());
     }
     // Prefer includes that do not need edits (i.e. already exist).

clang-tools-extra/clangd/Headers.h

public:
   /// \return A quoted "path" or <path> to be included, or None if it couldn't
   /// be shortened.
   llvm::Optional<std::string>
   calculateIncludePath(const HeaderFile &InsertedHeader,
                        llvm::StringRef IncludingFile) const;
 
   /// Calculates an edit that inserts \p VerbatimHeader into code. If the header
   /// is already included, this returns None.
-  llvm::Optional<TextEdit> insert(llvm::StringRef VerbatimHeader) const;
+  llvm::Optional<TextEdit> insert(llvm::StringRef VerbatimHeader,
+                                  bool ViaImport) const;
 
 private:
   StringRef FileName;
   StringRef Code;
   StringRef BuildDir;
   HeaderSearch *HeaderSearchInfo = nullptr;
   llvm::StringSet<> IncludedHeaders; // Both written and resolved.
   tooling::HeaderIncludes Inserter;  // Computers insertion replacement.

clang-tools-extra/clangd/Headers.cpp

IncludeInserter::calculateIncludePath(const HeaderFile &InsertedHeader,
   if (IsSystem)
     Suggested = "<" + Suggested + ">";
   else
     Suggested = "\"" + Suggested + "\"";
   return Suggested;
 }
 
 llvm::Optional<TextEdit>
-IncludeInserter::insert(llvm::StringRef VerbatimHeader) const {
+IncludeInserter::insert(llvm::StringRef VerbatimHeader, bool ViaImport) const {
   llvm::Optional<TextEdit> Edit;
   if (auto Insertion = Inserter.insert(VerbatimHeader.trim("\"<>"),
-                                       VerbatimHeader.startswith("<")))
+                                       VerbatimHeader.startswith("<"),
+                                       ViaImport))
     Edit = replacementToEdit(Code, *Insertion);
   return Edit;
 }
 
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Inclusion &Inc) {
   return OS << Inc.Written << " = "
             << (!Inc.Resolved.empty() ? Inc.Resolved : "[unresolved]")
             << " at line" << Inc.HashLine;

clang-tools-extra/clangd/IncludeFixer.h

 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;
 
   llvm::Optional<Fix> insertHeader(llvm::StringRef Name,
-                                   llvm::StringRef Symbol = "") const;
+                                   llvm::StringRef Symbol = "",
+                                   bool ViaImport = false) 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.

clang-tools-extra/clangd/IncludeFixer.cpp

if (Info.getNumArgs() > 0)
                                  getArgStr(Info, 1).value_or("")));
     break;
   }
 
   return {};
 }
 
 llvm::Optional<Fix> IncludeFixer::insertHeader(llvm::StringRef Spelled,
-                                               llvm::StringRef Symbol) const {
+                                               llvm::StringRef Symbol,
+                                               bool ViaImport) const {
   Fix F;
 
-  if (auto Edit = Inserter->insert(Spelled))
+  if (auto Edit = Inserter->insert(Spelled, ViaImport))
     F.Edits.push_back(std::move(*Edit));
   else
     return llvm::None;
 
   if (Symbol.empty())
-    F.Message = llvm::formatv("Include {0}", Spelled);
+    F.Message = llvm::formatv("{0} {1}", ViaImport ? "Import" : "Include",
+                              Spelled);
   else
-    F.Message = llvm::formatv("Include {0} for symbol {1}", Spelled, Symbol);
+    F.Message = llvm::formatv("{0} {1} for symbol {2}",
+        ViaImport ? "Import" : "Include", Spelled, Symbol);
 
   return F;
 }
 
 std::vector<Fix> IncludeFixer::fixIncompleteType(const Type &T) const {
   // Only handle incomplete TagDecl type.
   const TagDecl *TD = T.getAsTagDecl();
   if (!TD)
for (const auto &Inc : getRankedIncludes(Sym)) {
       // FIXME: We should support #import directives here.
       if ((Inc.Directive & clang::clangd::Symbol::Include) == 0)
         continue;
       if (auto ToInclude = Inserted(Sym, Inc.Header)) {
         if (ToInclude->second) {
           if (!InsertedHeaders.try_emplace(ToInclude->first).second)
             continue;
           if (auto Fix =
-                  insertHeader(ToInclude->first, (Sym.Scope + Sym.Name).str()))
+                  insertHeader(ToInclude->first, (Sym.Scope + Sym.Name).str(),
+                               Inc.Directive == Symbol::Import))
             Fixes.push_back(std::move(*Fix));
         }
       } else {
         vlog("Failed to calculate include insertion for {0} into {1}: {2}",
              Inc.Header, File, ToInclude.takeError());
       }
     }
   }

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

std::string calculate(PathRef Original, PathRef Preferred = "",
     auto Inserted = ToHeaderFile(Preferred);
     if (!Inserter.shouldInsertInclude(Original, Inserted))
       return "";
     auto Path = Inserter.calculateIncludePath(Inserted, MainFile);
     Action.EndSourceFile();
     return Path.value_or("");
   }
 
-  llvm::Optional<TextEdit> insert(llvm::StringRef VerbatimHeader) {
+  llvm::Optional<TextEdit> insert(llvm::StringRef VerbatimHeader,
+                                  bool ViaImport) {
     Clang = setupClang();
     PreprocessOnlyAction Action;
     EXPECT_TRUE(
         Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]));
 
     IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
                              CDB.getCompileCommand(MainFile)->Directory,
                              &Clang->getPreprocessor().getHeaderSearchInfo());
-    auto Edit = Inserter.insert(VerbatimHeader);
+    auto Edit = Inserter.insert(VerbatimHeader, ViaImport);
     Action.EndSourceFile();
     return Edit;
   }
 
   MockFS FS;
   MockCompilationDatabase CDB;
   std::string MainFile = testPath("main.cpp");
   std::string Subdir = testPath("sub");
TEST_F(HeadersTest, DontInsertDuplicateResolved) {
   Inc.Written = "fake-bar.h";
   Inc.Resolved = testPath("sub/bar.h");
   EXPECT_EQ(calculate(Inc.Resolved, "", {Inc}), "");
   // Do not insert preferred.
   EXPECT_EQ(calculate(Inc.Resolved, "\"BAR.h\"", {Inc}), "");
 }
 
 TEST_F(HeadersTest, PreferInserted) {
-  auto Edit = insert("<y>");
+  auto Edit = insert("<y>", /*ViaImport=*/false);
   EXPECT_TRUE(Edit);
-  EXPECT_TRUE(StringRef(Edit->newText).contains("<y>"));
+  EXPECT_EQ(Edit->newText, "#include <y>\n");
+
+  Edit = insert("\"header.h\"", /*ViaImport=*/true);
+  EXPECT_TRUE(Edit);
+  EXPECT_EQ(Edit->newText, "#import \"header.h\"\n");
 }
 
 TEST(Headers, NoHeaderSearchInfo) {
   std::string MainFile = testPath("main.cpp");
   IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
                            /*BuildDir=*/"", /*HeaderSearchInfo=*/nullptr);
 
   auto HeaderPath = testPath("sub/bar.h");

clang/include/clang/Tooling/Inclusions/HeaderIncludes.h

private:
   bool isMainHeader(StringRef IncludeName) const;
 
   const IncludeStyle Style;
   bool IsMainFile;
   std::string FileName;
   SmallVector<llvm::Regex, 4> CategoryRegexs;
 };
 
 /// Generates replacements for inserting or deleting #include directives in a
 /// file.
 class HeaderIncludes {
 public:
   HeaderIncludes(llvm::StringRef FileName, llvm::StringRef Code,
                  const IncludeStyle &Style);
 
-  /// Inserts an #include directive of \p Header into the code. If \p IsAngled
-  /// is true, \p Header will be quoted with <> in the directive; otherwise, it
-  /// will be quoted with "".
+  /// Inserts an #include or #import directive of \p Header into the code.
+  /// If \p IsAngled is true, \p Header will be quoted with <> in the directive;
+  /// otherwise, it will be quoted with "".
   ///
   /// When searching for points to insert new header, this ignores #include's
   /// after the #include block(s) in the beginning of a file to avoid inserting
   /// headers into code sections where new #include's should not be added by
   /// default. These code sections include:
   ///   - raw string literals (containing #include).
   ///   - #if blocks.
   ///   - Special #include's among declarations (e.g. functions).
   ///
   /// Returns a replacement that inserts the new header into a suitable #include
   /// block of the same category. This respects the order of the existing
   /// #includes in the block; if the existing #includes are not already sorted,
   /// this will simply insert the #include in front of the first #include of the
   /// same category in the code that should be sorted after \p IncludeName. If
   /// \p IncludeName already exists (with exactly the same spelling), this
   /// returns None.
-  llvm::Optional<tooling::Replacement> insert(llvm::StringRef Header,
-                                              bool IsAngled) const;
+  llvm::Optional<tooling::Replacement>
+  insert(llvm::StringRef Header, bool IsAngled, bool IsImport = false) const;
 
   /// Removes all existing #includes of \p Header quoted with <> if \p IsAngled
   /// is true or "" if \p IsAngled is false.
-  /// This doesn't resolve the header file path; it only deletes #includes with
-  /// exactly the same spelling.
+  /// This doesn't resolve the header file path; it only deletes #includes and
+  /// #imports with exactly the same spelling.
   tooling::Replacements remove(llvm::StringRef Header, bool IsAngled) const;
 
   // Matches a whole #include directive.
   static const llvm::Regex IncludeRegex;
 
 private:
   struct Include {
     Include(StringRef Name, tooling::Range R) : Name(Name), R(R) {}
 
     // An include header quoted with either <> or "".
     std::string Name;
     // The range of the whole line of include directive including any leading
     // whitespaces and trailing comment.
     tooling::Range R;
   };

clang/lib/Tooling/Inclusions/HeaderIncludes.cpp

for (auto Line : Lines) {
     NextLineOffset = std::min(Code.size(), Offset + Line.size() + 1);
     if (IncludeRegex.match(Line, &Matches)) {
       // If this is the last line without trailing newline, we need to make
       // sure we don't delete across the file boundary.
       addExistingInclude(
           Include(Matches[2],
                   tooling::Range(
                       Offset, std::min(Line.size() + 1, Code.size() - Offset))),
           NextLineOffset);
     }
     Offset = NextLineOffset;
   }
 
   // Populate CategoryEndOfssets:
   // - Ensure that CategoryEndOffset[Highest] is always populated.
   // - If CategoryEndOffset[Priority] isn't set, use the next higher value
   // that is set, up to CategoryEndOffset[Highest].
if (CurInclude.R.getOffset() <= MaxInsertOffset) {
     CategoryEndOffsets[Priority] = NextLineOffset;
     IncludesByPriority[Priority].push_back(&CurInclude);
     if (FirstIncludeOffset < 0)
       FirstIncludeOffset = CurInclude.R.getOffset();
   }
 }
 
 llvm::Optional<tooling::Replacement>
-HeaderIncludes::insert(llvm::StringRef IncludeName, bool IsAngled) const {
+HeaderIncludes::insert(llvm::StringRef IncludeName, bool IsAngled,
+                       bool IsImport) const {
   assert(IncludeName == trimInclude(IncludeName));
   // If a <header> ("header") already exists in code, "header" (<header>) with
   // different quotation will still be inserted.
   // FIXME: figure out if this is the best behavior.
   auto It = ExistingIncludes.find(IncludeName);
   if (It != ExistingIncludes.end())
     for (const auto &Inc : It->second)
       if ((IsAngled && StringRef(Inc.Name).startswith("<")) ||
           (!IsAngled && StringRef(Inc.Name).startswith("\"")))
         return llvm::None;
   std::string Quoted =
       std::string(llvm::formatv(IsAngled ? "<{0}>" : "\"{0}\"", IncludeName));
   StringRef QuotedName = Quoted;
   int Priority = Categories.getIncludePriority(
       QuotedName, /*CheckMainHeader=*/FirstIncludeOffset < 0);
   auto CatOffset = CategoryEndOffsets.find(Priority);
   assert(CatOffset != CategoryEndOffsets.end());
   unsigned InsertOffset = CatOffset->second; // Fall back offset
   auto Iter = IncludesByPriority.find(Priority);
   if (Iter != IncludesByPriority.end()) {
     for (const auto *Inc : Iter->second) {
       if (QuotedName < Inc->Name) {
         InsertOffset = Inc->R.getOffset();
         break;
       }
     }
   }
   assert(InsertOffset <= Code.size());
   std::string NewInclude =
-      std::string(llvm::formatv("#include {0}\n", QuotedName));
+      IsImport ? std::string(llvm::formatv("#import {0}\n", QuotedName))
+               : std::string(llvm::formatv("#include {0}\n", QuotedName));
   // When inserting headers at end of the code, also append '\n' to the code
   // if it does not end with '\n'.
   // FIXME: when inserting multiple #includes at the end of code, only one
   // newline should be added.
   if (InsertOffset == Code.size() && (!Code.empty() && Code.back() != '\n'))
     NewInclude = "\n" + NewInclude;
   return tooling::Replacement(FileName, InsertOffset, 0, NewInclude);
 }