[clangd] Check file path of declaring header when deciding whether to insert include.

[ioeric]

Summary

Previously, we would use include spelling of the declaring header to check
whether the inserted header is the same as the main file. This doesn't help because
we only have file path of the main file.

[sammccall]

Thanks!

IIUC, this already fixes the cases we'd seen of include-insertion of a header into itself.
Is it feasible to add a test case for that?

clangd/Headers.h

140 ↗

(On Diff #195136)

is the path

[ioeric]

unittests/clangd/HeadersTests.cpp

208 ↗

(On Diff #195136)

IIUC, this already fixes the cases we'd seen of include-insertion of a header into itself.
Is it feasible to add a test case for that?

Here is a test case for it. However, to trigger the old behavior caused by include spelling, we would need to implement a custom URI scheme that performs include spelling customization. Let me know if you think we should add one.

[sammccall]

unittests/clangd/HeadersTests.cpp

208 ↗

(On Diff #195136)

Ah right, this probably isn't worth it :-(

clang-tools-extra/trunk/clangd/CodeComplete.cpp

if (C.IdentifierResult) {
       Completion.Origin |= SymbolOrigin::Identifier;
       Completion.Kind = CompletionItemKind::Text;
       Completion.Name = C.IdentifierResult->Name;
     }
 
     // Turn absolute path into a literal string that can be #included.
     auto Inserted = [&](llvm::StringRef Header)
         -> llvm::Expected<std::pair<std::string, bool>> {
-      auto ResolvedDeclaring =
-          toHeaderFile(C.IndexResult->CanonicalDeclaration.FileURI, FileName);
+      auto DeclaringURI =
+          URI::parse(C.IndexResult->CanonicalDeclaration.FileURI);
+      if (!DeclaringURI)
+        return DeclaringURI.takeError();
+      auto ResolvedDeclaring = URI::resolve(*DeclaringURI, FileName);
       if (!ResolvedDeclaring)
         return ResolvedDeclaring.takeError();
       auto ResolvedInserted = toHeaderFile(Header, FileName);
       if (!ResolvedInserted)
         return ResolvedInserted.takeError();
       return std::make_pair(
-          Includes.calculateIncludePath(*ResolvedDeclaring, *ResolvedInserted),
+          Includes.calculateIncludePath(*ResolvedInserted),
           Includes.shouldInsertInclude(*ResolvedDeclaring, *ResolvedInserted));
     };
     bool ShouldInsert = C.headerToInsertIfAllowed(Opts).hasValue();
     // Calculate include paths and edits for all possible headers.
     for (const auto &Inc : C.RankedIncludeHeaders) {
       if (auto ToInclude = Inserted(Inc)) {
         CodeCompletion::IncludeCandidate Include;
         Include.Header = ToInclude->first;

clang-tools-extra/trunk/clangd/Headers.h

public:
   void addExisting(const Inclusion &Inc);
 
   /// Checks whether to add an #include of the header into \p File.
   /// An #include will not be added if:
   ///   - Either \p DeclaringHeader or \p InsertedHeader is already (directly)
   ///   in \p Inclusions (including those included via different paths).
   ///   - \p DeclaringHeader or \p InsertedHeader is the same as \p File.
   ///
-  /// \param DeclaringHeader is the original header corresponding to \p
+  /// \param DeclaringHeader is path of the original header corresponding to \p
   /// InsertedHeader e.g. the header that declares a symbol.
   /// \param InsertedHeader The preferred header to be inserted. This could be
   /// the same as DeclaringHeader but must be provided.
-  bool shouldInsertInclude(const HeaderFile &DeclaringHeader,
+  bool shouldInsertInclude(PathRef DeclaringHeader,
                            const HeaderFile &InsertedHeader) const;
 
   /// Determines the preferred way to #include a file, taking into account the
   /// search path. Usually this will prefer a shorter representation like
   /// 'Foo/Bar.h' over a longer one like 'Baz/include/Foo/Bar.h'.
   ///
-  /// \param DeclaringHeader is the original header corresponding to \p
-  /// InsertedHeader e.g. the header that declares a symbol.
   /// \param InsertedHeader The preferred header to be inserted. This could be
   /// the same as DeclaringHeader but must be provided.
   ///
   /// \return A quoted "path" or <path> to be included.
-  std::string calculateIncludePath(const HeaderFile &DeclaringHeader,
-                                   const HeaderFile &InsertedHeader) const;
+  std::string calculateIncludePath(const HeaderFile &InsertedHeader) 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;
 
 private:
   StringRef FileName;
   StringRef Code;

clang-tools-extra/trunk/clangd/Headers.cpp

void IncludeInserter::addExisting(const Inclusion &Inc) {
   IncludedHeaders.insert(Inc.Written);
   if (!Inc.Resolved.empty())
     IncludedHeaders.insert(Inc.Resolved);
 }
 
 /// FIXME(ioeric): we might not want to insert an absolute include path if the
 /// path is not shortened.
 bool IncludeInserter::shouldInsertInclude(
-    const HeaderFile &DeclaringHeader, const HeaderFile &InsertedHeader) const {
-  assert(DeclaringHeader.valid() && InsertedHeader.valid());
+    PathRef DeclaringHeader, const HeaderFile &InsertedHeader) const {
+  assert(InsertedHeader.valid());
   if (!HeaderSearchInfo && !InsertedHeader.Verbatim)
     return false;
-  if (FileName == DeclaringHeader.File || FileName == InsertedHeader.File)
+  if (FileName == DeclaringHeader || FileName == InsertedHeader.File)
     return false;
   auto Included = [&](llvm::StringRef Header) {
     return IncludedHeaders.find(Header) != IncludedHeaders.end();
   };
-  return !Included(DeclaringHeader.File) && !Included(InsertedHeader.File);
+  return !Included(DeclaringHeader) && !Included(InsertedHeader.File);
 }
 
 std::string
-IncludeInserter::calculateIncludePath(const HeaderFile &DeclaringHeader,
-                                      const HeaderFile &InsertedHeader) const {
-  assert(DeclaringHeader.valid() && InsertedHeader.valid());
+IncludeInserter::calculateIncludePath(const HeaderFile &InsertedHeader) const {
+  assert(InsertedHeader.valid());
   if (InsertedHeader.Verbatim)
     return InsertedHeader.File;
   bool IsSystem = false;
   if (!HeaderSearchInfo)
     return "\"" + InsertedHeader.File + "\"";
   std::string Suggested = HeaderSearchInfo->suggestPathToFileForDiagnostics(
       InsertedHeader.File, BuildDir, &IsSystem);
   if (IsSystem)

clang-tools-extra/trunk/clangd/IncludeFixer.cpp

if (!Matched.IncludeHeaders.empty() && Matched.Definition &&
       Fixes = fixesForSymbols(Syms);
   }
   return Fixes;
 }
 
 std::vector<Fix> IncludeFixer::fixesForSymbols(const SymbolSlab &Syms) const {
   auto Inserted = [&](const Symbol &Sym, llvm::StringRef Header)
       -> llvm::Expected<std::pair<std::string, bool>> {
-    auto ResolvedDeclaring =
-        toHeaderFile(Sym.CanonicalDeclaration.FileURI, File);
+    auto DeclaringURI = URI::parse(Sym.CanonicalDeclaration.FileURI);
+    if (!DeclaringURI)
+      return DeclaringURI.takeError();
+    auto ResolvedDeclaring = URI::resolve(*DeclaringURI, File);
     if (!ResolvedDeclaring)
       return ResolvedDeclaring.takeError();
     auto ResolvedInserted = toHeaderFile(Header, File);
     if (!ResolvedInserted)
       return ResolvedInserted.takeError();
     return std::make_pair(
-        Inserter->calculateIncludePath(*ResolvedDeclaring, *ResolvedInserted),
+        Inserter->calculateIncludePath(*ResolvedInserted),
         Inserter->shouldInsertInclude(*ResolvedDeclaring, *ResolvedInserted));
   };
 
   std::vector<Fix> Fixes;
   // Deduplicate fixes by include headers. This doesn't distiguish symbols in
   // different scopes from the same header, but this case should be rare and is
   // thus ignored.
   llvm::StringSet<> InsertedHeaders;
   for (const auto &Sym : Syms) {
     for (const auto &Inc : getRankedIncludes(Sym)) {
       if (auto ToInclude = Inserted(Sym, Inc)) {
         if (ToInclude->second) {
           auto I = InsertedHeaders.try_emplace(ToInclude->first);
           if (!I.second)
             continue;
           if (auto Edit = Inserter->insert(ToInclude->first))
             Fixes.push_back(
                 Fix{llvm::formatv("Add include {0} for symbol {1}{2}",
                                   ToInclude->first, Sym.Scope, Sym.Name),
                     {std::move(*Edit)}});
         }
       } else {
-        vlog("Failed to calculate include insertion for {0} into {1}: {2}",
-             File, Inc, ToInclude.takeError());
+        vlog("Failed to calculate include insertion for {0} into {1}: {2}", Inc,
+             File, ToInclude.takeError());
       }
     }
   }
   return Fixes;
 }
 
 // Returns the identifiers qualified by an unresolved name. \p Loc is the
 // start location of the unresolved name. For the example below, this returns

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

auto ToHeaderFile = [](llvm::StringRef Header) {
                         /*Verbatim=*/!llvm::sys::path::is_absolute(Header)};
     };
 
     IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
                              CDB.getCompileCommand(MainFile)->Directory,
                              &Clang->getPreprocessor().getHeaderSearchInfo());
     for (const auto &Inc : Inclusions)
       Inserter.addExisting(Inc);
-    auto Declaring = ToHeaderFile(Original);
     auto Inserted = ToHeaderFile(Preferred);
-    if (!Inserter.shouldInsertInclude(Declaring, Inserted))
+    if (!Inserter.shouldInsertInclude(Original, Inserted))
       return "";
-    std::string Path = Inserter.calculateIncludePath(Declaring, Inserted);
+    std::string Path = Inserter.calculateIncludePath(Inserted);
     Action.EndSourceFile();
     return Path;
   }
 
   llvm::Optional<TextEdit> insert(llvm::StringRef VerbatimHeader) {
     auto Clang = setupClang();
     PreprocessOnlyAction Action;
     EXPECT_TRUE(

 }
 
 TEST(Headers, NoHeaderSearchInfo) {
   std::string MainFile = testPath("main.cpp");
   IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
                            /*BuildDir=*/"", /*HeaderSearchInfo=*/nullptr);
 
   auto HeaderPath = testPath("sub/bar.h");
-  auto Declaring = HeaderFile{HeaderPath, /*Verbatim=*/false};
   auto Inserting = HeaderFile{HeaderPath, /*Verbatim=*/false};
   auto Verbatim = HeaderFile{"<x>", /*Verbatim=*/true};
 
-  EXPECT_EQ(Inserter.calculateIncludePath(Declaring, Inserting),
+  EXPECT_EQ(Inserter.calculateIncludePath(Inserting),
             "\"" + HeaderPath + "\"");
-  EXPECT_EQ(Inserter.shouldInsertInclude(Declaring, Inserting), false);
+  EXPECT_EQ(Inserter.shouldInsertInclude(HeaderPath, Inserting), false);
 
-  EXPECT_EQ(Inserter.calculateIncludePath(Declaring, Verbatim), "<x>");
-  EXPECT_EQ(Inserter.shouldInsertInclude(Declaring, Verbatim), true);
+  EXPECT_EQ(Inserter.calculateIncludePath(Verbatim), "<x>");
+  EXPECT_EQ(Inserter.shouldInsertInclude(HeaderPath, Verbatim), true);
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang