Index: clangd/ClangdLSPServer.cpp =================================================================== --- clangd/ClangdLSPServer.cpp +++ clangd/ClangdLSPServer.cpp @@ -189,12 +189,20 @@ ExecuteCommandParams::CLANGD_INSERT_HEADER_INCLUDE + " called on non-added file " + FileURI.file()) .str()); - auto Replaces = Server.insertInclude(FileURI.file(), *Code, - Params.includeInsertion->header); + llvm::StringRef DeclaringHeader = Params.includeInsertion->declaringHeader; + if (DeclaringHeader.empty()) + return replyError( + ErrorCode::InvalidParams, + "declaringHeader must be provided for include insertion."); + llvm::StringRef PreferredHeader = Params.includeInsertion->preferredHeader; + auto Replaces = Server.insertInclude( + FileURI.file(), *Code, DeclaringHeader, + PreferredHeader.empty() ? DeclaringHeader : PreferredHeader); if (!Replaces) { std::string ErrMsg = ("Failed to generate include insertion edits for adding " + - Params.includeInsertion->header + " into " + FileURI.file()) + DeclaringHeader + " (" + PreferredHeader + ") into " + + FileURI.file()) .str(); log(ErrMsg + ":" + llvm::toString(Replaces.takeError())); replyError(ErrorCode::InternalError, ErrMsg); @@ -204,7 +212,8 @@ WorkspaceEdit WE; WE.changes = {{FileURI.uri(), Edits}}; - reply("Inserted header " + Params.includeInsertion->header); + reply(("Inserted header " + DeclaringHeader + " (" + PreferredHeader + ")") + .str()); ApplyEdit(std::move(WE)); } else { // We should not get here because ExecuteCommandParams would not have Index: clangd/ClangdServer.h =================================================================== --- clangd/ClangdServer.h +++ clangd/ClangdServer.h @@ -235,12 +235,21 @@ UniqueFunction>)> Callback); - /// Inserts a new #include of \p Header into \p File, if it's not present. - /// \p Header is either an URI that can be resolved to an #include path that - /// is suitable to be inserted or a literal string quoted with <> or "" that - /// can be #included directly. + /// Inserts a new #include into \p File, if it's not present in \p Code. + /// + /// \p DeclaringHeader The original header corresponding to this insertion + /// e.g. the header that declared a symbol. This can be either a URI or a + /// literal string quoted with <> or "" that can be #included directly. + /// \p InsertedHeader The preferred header to be inserted. This may be + /// different from \p DeclaringHeader as a header file can have a different + /// canonical include. This can be either a URI or a literal string quoted + /// with <> or "" that can be #included directly. + /// + /// Both OriginalHeader and InsertedHeader will be considered to determine + /// whether an include needs to be added. Expected insertInclude(PathRef File, StringRef Code, - StringRef Header); + StringRef DeclaringHeader, + StringRef InsertedHeader); /// Gets current document contents for \p File. Returns None if \p File is not /// currently tracked. Index: clangd/ClangdServer.cpp =================================================================== --- clangd/ClangdServer.cpp +++ clangd/ClangdServer.cpp @@ -313,31 +313,42 @@ BindWithForward(Action, File.str(), NewName.str(), std::move(Callback))); } +/// Creates a `HeaderFile` from \p Header which can be either a URI or a literal +/// include. +static llvm::Expected toHeaderFile(StringRef Header, + llvm::StringRef HintPath) { + if (isLiteralInclude(Header)) + return HeaderFile{Header.str(), /*Verbatim=*/true}; + auto U = URI::parse(Header); + if (!U) + return U.takeError(); + auto Resolved = URI::resolve(*U, HintPath); + if (!Resolved) + return Resolved.takeError(); + return HeaderFile{std::move(*Resolved), /*Verbatim=*/false}; +}; + Expected ClangdServer::insertInclude(PathRef File, StringRef Code, - llvm::StringRef Header) { + StringRef DeclaringHeader, + StringRef InsertedHeader) { + assert(!DeclaringHeader.empty() && !InsertedHeader.empty()); std::string ToInclude; - if (Header.startswith("<") || Header.startswith("\"")) { - ToInclude = Header; - } else { - auto U = URI::parse(Header); - if (!U) - return U.takeError(); - auto Resolved = URI::resolve(*U, /*HintPath=*/File); - if (!Resolved) - return Resolved.takeError(); - - tooling::CompileCommand CompileCommand = - CompileArgs.getCompileCommand(File); - auto Include = - calculateIncludePath(File, Code, *Resolved, CompileCommand, - FSProvider.getTaggedFileSystem(File).Value); - if (!Include) - return Include.takeError(); - if (Include->empty()) - return tooling::Replacements(); - ToInclude = std::move(*Include); - } + auto ResolvedOrginal = toHeaderFile(DeclaringHeader, File); + if (!ResolvedOrginal) + return ResolvedOrginal.takeError(); + auto ResolvedPreferred = toHeaderFile(InsertedHeader, File); + if (!ResolvedPreferred) + return ResolvedPreferred.takeError(); + tooling::CompileCommand CompileCommand = CompileArgs.getCompileCommand(File); + auto Include = calculateIncludePath( + File, Code, *ResolvedOrginal, *ResolvedPreferred, CompileCommand, + FSProvider.getTaggedFileSystem(File).Value); + if (!Include) + return Include.takeError(); + if (Include->empty()) + return tooling::Replacements(); + ToInclude = std::move(*Include); auto Style = format::getStyle("file", File, "llvm"); if (!Style) { Index: clangd/CodeComplete.cpp =================================================================== --- clangd/CodeComplete.cpp +++ clangd/CodeComplete.cpp @@ -286,13 +286,12 @@ I.documentation = D->Documentation; if (I.detail.empty()) I.detail = D->CompletionDetail; - // We only insert #include for items with details, since we can't tell - // whether the file URI of the canonical declaration would be the + // We only insert #include for items with both declarations and detail - + // we can't tell whether the file URI of the declaration would be the // canonical #include without checking IncludeHeader in the detail. // FIXME: delay creating include insertion command to // "completionItem/resolve", when it is supported - if (!D->IncludeHeader.empty() || - !IndexResult->CanonicalDeclaration.FileURI.empty()) { + if (!IndexResult->CanonicalDeclaration.FileURI.empty()) { // LSP favors additionalTextEdits over command. But we are still using // command here because it would be expensive to calculate #include // insertion edits for all candidates, and the include insertion edit @@ -301,9 +300,8 @@ // Command title is not added since this is not a user-facing command. Cmd.command = ExecuteCommandParams::CLANGD_INSERT_HEADER_INCLUDE; IncludeInsertion Insertion; - Insertion.header = D->IncludeHeader.empty() - ? IndexResult->CanonicalDeclaration.FileURI - : D->IncludeHeader; + Insertion.declaringHeader = IndexResult->CanonicalDeclaration.FileURI; + Insertion.preferredHeader = D->IncludeHeader; Insertion.textDocument.uri = URIForFile(FileName); Cmd.includeInsertion = std::move(Insertion); I.command = std::move(Cmd); Index: clangd/Headers.h =================================================================== --- clangd/Headers.h +++ clangd/Headers.h @@ -19,18 +19,36 @@ namespace clang { namespace clangd { +/// Returns true if \p Include is literal include like "path" or . +bool isLiteralInclude(llvm::StringRef Include); + +/// Represents a header file to be #include'd. +struct HeaderFile { + std::string File; + /// If this is true, `File` is a literal string quoted with <> or "" that + /// can be #included directly; otherwise, `File` is an absolute file path. + bool Verbatim; + + bool valid() 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 File is an absolute file path. -/// \param Header is an absolute file path. -/// \return A quoted "path" or . This returns an empty string if: -/// - \p Header is already (directly) included in the file (including those -/// included via different paths). -/// - \p Header is the same as \p File. +/// \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 . This returns an empty string if: +/// - Either \p DeclaringHeader or \p InsertedHeader is already (directly) +/// included in the file (including those included via different paths). +/// - \p DeclaringHeader or \p InsertedHeader is the same as \p File. llvm::Expected -calculateIncludePath(PathRef File, llvm::StringRef Code, llvm::StringRef Header, +calculateIncludePath(PathRef File, llvm::StringRef Code, + const HeaderFile &DeclaringHeader, + const HeaderFile &InsertedHeader, const tooling::CompileCommand &CompileCommand, IntrusiveRefCntPtr FS); Index: clangd/Headers.cpp =================================================================== --- clangd/Headers.cpp +++ clangd/Headers.cpp @@ -24,36 +24,50 @@ class RecordHeaders : public PPCallbacks { public: - RecordHeaders(std::set &Headers) : Headers(Headers) {} + RecordHeaders(std::set &WrittenHeaders, + std::set &ResolvedHeaders) + : WrittenHeaders(WrittenHeaders), ResolvedHeaders(ResolvedHeaders) {} void InclusionDirective(SourceLocation /*HashLoc*/, const Token & /*IncludeTok*/, - llvm::StringRef /*FileName*/, bool /*IsAngled*/, + llvm::StringRef FileName, bool IsAngled, CharSourceRange /*FilenameRange*/, const FileEntry *File, llvm::StringRef /*SearchPath*/, llvm::StringRef /*RelativePath*/, const Module * /*Imported*/) override { + WrittenHeaders.insert( + (IsAngled ? "<" + FileName + ">" : "\"" + FileName + "\"").str()); if (File != nullptr && !File->tryGetRealPathName().empty()) - Headers.insert(File->tryGetRealPathName()); + ResolvedHeaders.insert(File->tryGetRealPathName()); } private: - std::set &Headers; + std::set &WrittenHeaders; + std::set &ResolvedHeaders; }; } // namespace +bool isLiteralInclude(llvm::StringRef Include) { + return Include.startswith("<") || Include.startswith("\""); +} + +bool HeaderFile::valid() const { + return (Verbatim && isLiteralInclude(File)) || + (!Verbatim && llvm::sys::path::is_absolute(File)); +} + /// FIXME(ioeric): we might not want to insert an absolute include path if the /// path is not shortened. llvm::Expected calculateIncludePath(llvm::StringRef File, llvm::StringRef Code, - llvm::StringRef Header, + const HeaderFile &DeclaringHeader, + const HeaderFile &InsertedHeader, const tooling::CompileCommand &CompileCommand, IntrusiveRefCntPtr FS) { - assert(llvm::sys::path::is_absolute(File) && - llvm::sys::path::is_absolute(Header)); - - if (File == Header) + assert(llvm::sys::path::is_absolute(File)); + assert(DeclaringHeader.valid() && InsertedHeader.valid()); + if (File == DeclaringHeader.File || File == InsertedHeader.File) return ""; FS->setCurrentWorkingDirectory(CompileCommand.Directory); @@ -95,29 +109,35 @@ return llvm::make_error( "Failed to begin preprocessor only action for file " + File, llvm::inconvertibleErrorCode()); - std::set ExistingHeaders; + std::set WrittenHeaders; + std::set ResolvedHeaders; Clang->getPreprocessor().addPPCallbacks( - llvm::make_unique(ExistingHeaders)); + llvm::make_unique(WrittenHeaders, ResolvedHeaders)); if (!Action.Execute()) return llvm::make_error( "Failed to execute preprocessor only action for file " + File, llvm::inconvertibleErrorCode()); - if (ExistingHeaders.find(Header) != ExistingHeaders.end()) { - return llvm::make_error( - Header + " is already included in " + File, - llvm::inconvertibleErrorCode()); - } + auto Included = [&](llvm::StringRef Header) { + return WrittenHeaders.find(Header) != WrittenHeaders.end() || + ResolvedHeaders.find(Header) != ResolvedHeaders.end(); + }; + if (Included(DeclaringHeader.File) || Included(InsertedHeader.File)) + return ""; auto &HeaderSearchInfo = Clang->getPreprocessor().getHeaderSearchInfo(); bool IsSystem = false; + + if (InsertedHeader.Verbatim) + return InsertedHeader.File; + std::string Suggested = HeaderSearchInfo.suggestPathToFileForDiagnostics( - Header, CompileCommand.Directory, &IsSystem); + InsertedHeader.File, CompileCommand.Directory, &IsSystem); if (IsSystem) Suggested = "<" + Suggested + ">"; else Suggested = "\"" + Suggested + "\""; - log("Suggested #include for " + Header + " is: " + Suggested); + log("Suggested #include for " + InsertedHeader.File + " is: " + Suggested); return Suggested; } Index: clangd/Protocol.h =================================================================== --- clangd/Protocol.h +++ clangd/Protocol.h @@ -427,11 +427,20 @@ struct IncludeInsertion { /// The document in which the command was invoked. + /// If either originalHeader or preferredHeader has been (directly) included + /// in the current file, no new include will be inserted. TextDocumentIdentifier textDocument; - /// The header to be inserted. This could be either a URI ir a literal string - /// quoted with <> or "" that can be #included directly. - std::string header; + /// The declaring header corresponding to this insertion e.g. the header that + /// declares a symbol. This could be either a URI or a literal string quoted + /// with <> or "" that can be #included directly. + std::string declaringHeader; + /// The preferred header to be inserted. This may be different from + /// originalHeader as a header file can have a different canonical include. + /// This could be either a URI or a literal string quoted with <> or "" that + /// can be #included directly. If empty, declaringHeader is used to calculate + /// the #include path. + std::string preferredHeader; }; bool fromJSON(const json::Expr &, IncludeInsertion &); json::Expr toJSON(const IncludeInsertion &II); Index: clangd/Protocol.cpp =================================================================== --- clangd/Protocol.cpp +++ clangd/Protocol.cpp @@ -370,10 +370,13 @@ bool fromJSON(const json::Expr &II, IncludeInsertion &R) { json::ObjectMapper O(II); return O && O.map("textDocument", R.textDocument) && - O.map("header", R.header); + O.map("declaringHeader", R.declaringHeader) && + O.map("preferredHeader", R.preferredHeader); } json::Expr toJSON(const IncludeInsertion &II) { - return json::obj{{"textDocument", II.textDocument}, {"header", II.header}}; + return json::obj{{"textDocument", II.textDocument}, + {"declaringHeader", II.declaringHeader}, + {"preferredHeader", II.preferredHeader}}; } json::Expr toJSON(const ApplyWorkspaceEditParams &Params) { Index: test/clangd/insert-include.test =================================================================== --- test/clangd/insert-include.test +++ test/clangd/insert-include.test @@ -4,10 +4,10 @@ --- {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"void f() {}"}}} --- -{"jsonrpc":"2.0","id":3,"method":"workspace/executeCommand","params":{"arguments":[{"header":"","textDocument":{"uri":"test:///main.cpp"}}],"command":"clangd.insertInclude"}} +{"jsonrpc":"2.0","id":3,"method":"workspace/executeCommand","params":{"arguments":[{"declaringHeader":"\"/usr/include/bits/vector\"", "preferredHeader":"","textDocument":{"uri":"test:///main.cpp"}}],"command":"clangd.insertInclude"}} # CHECK: "id": 3, # CHECK-NEXT: "jsonrpc": "2.0", -# CHECK-NEXT: "result": "Inserted header " +# CHECK-NEXT: "result": "Inserted header \"/usr/include/bits/vector\" ()" # CHECK-NEXT: } # CHECK: "method": "workspace/applyEdit", # CHECK-NEXT: "params": { Index: unittests/clangd/ClangdTests.cpp =================================================================== --- unittests/clangd/ClangdTests.cpp +++ unittests/clangd/ClangdTests.cpp @@ -862,6 +862,8 @@ MockFSProvider FS; ErrorCheckingDiagConsumer DiagConsumer; MockCompilationDatabase CDB; + std::string SearchDirArg = (llvm::Twine("-I") + testRoot()).str(); + CDB.ExtraClangFlags.insert(CDB.ExtraClangFlags.end(), {SearchDirArg.c_str()}); ClangdServer Server(CDB, DiagConsumer, FS, /*AsyncThreadsCount=*/0, /*StorePreamblesInMemory=*/true); @@ -877,8 +879,10 @@ FS.Files[FooCpp] = Code; Server.addDocument(FooCpp, Code); - auto Inserted = [&](llvm::StringRef Header, llvm::StringRef Expected) { - auto Replaces = Server.insertInclude(FooCpp, Code, Header); + auto Inserted = [&](llvm::StringRef Original, llvm::StringRef Preferred, + llvm::StringRef Expected) { + auto Replaces = Server.insertInclude( + FooCpp, Code, Original, Preferred.empty() ? Original : Preferred); EXPECT_TRUE(static_cast(Replaces)); auto Changed = tooling::applyAllReplacements(Code, *Replaces); EXPECT_TRUE(static_cast(Changed)); @@ -886,8 +890,19 @@ (llvm::Twine("#include ") + Expected + "").str()); }; - EXPECT_TRUE(Inserted("\"y.h\"", "\"y.h\"")); - EXPECT_TRUE(Inserted("", "")); + EXPECT_TRUE(Inserted("\"y.h\"", /*Preferred=*/"","\"y.h\"")); + EXPECT_TRUE(Inserted("\"y.h\"", /*Preferred=*/"\"Y.h\"", "\"Y.h\"")); + EXPECT_TRUE(Inserted("", /*Preferred=*/"", "")); + EXPECT_TRUE(Inserted("", /*Preferred=*/"", "")); + + std::string OriginalHeader = URI::createFile(testPath("y.h")).toString(); + std::string PreferredHeader = URI::createFile(testPath("Y.h")).toString(); + EXPECT_TRUE(Inserted(OriginalHeader, + /*Preferred=*/"", "\"y.h\"")); + EXPECT_TRUE(Inserted(OriginalHeader, + /*Preferred=*/"", "")); + EXPECT_TRUE(Inserted(OriginalHeader, PreferredHeader, "\"Y.h\"")); + EXPECT_TRUE(Inserted("", PreferredHeader, "\"Y.h\"")); } } // namespace Index: unittests/clangd/HeadersTests.cpp =================================================================== --- unittests/clangd/HeadersTests.cpp +++ unittests/clangd/HeadersTests.cpp @@ -24,17 +24,28 @@ } protected: - // Calculates the include path for \p Header, or returns "" on error. - std::string calculate(PathRef Header) { + // Calculates the include path, or returns "" on error. + std::string calculate(PathRef Original, PathRef Preferred = "", + bool ExpectError = false) { + if (Preferred.empty()) + Preferred = Original; auto VFS = FS.getTaggedFileSystem(MainFile).Value; auto Cmd = CDB.getCompileCommand(MainFile); assert(static_cast(Cmd)); VFS->setCurrentWorkingDirectory(Cmd->Directory); - auto Path = - calculateIncludePath(MainFile, FS.Files[MainFile], Header, *Cmd, VFS); + auto ToHeaderFile = [](llvm::StringRef Header) { + return HeaderFile{Header, + /*Verbatim=*/!llvm::sys::path::is_absolute(Header)}; + }; + auto Path = calculateIncludePath(MainFile, FS.Files[MainFile], + ToHeaderFile(Original), + ToHeaderFile(Preferred), *Cmd, VFS); if (!Path) { llvm::consumeError(Path.takeError()); + EXPECT_TRUE(ExpectError); return std::string(); + } else { + EXPECT_FALSE(ExpectError); } return std::move(*Path); } @@ -66,7 +77,21 @@ FS.Files[MainFile] = R"cpp( #include "sub/bar.h" // not shortest )cpp"; - EXPECT_EQ(calculate(BarHeader), ""); + EXPECT_EQ(calculate("\"sub/bar.h\""), ""); // Duplicate rewritten. + EXPECT_EQ(calculate(BarHeader), ""); // Duplicate resolved. + EXPECT_EQ(calculate(BarHeader, "\"BAR.h\""), ""); // Do not insert preferred. +} + +TEST_F(HeadersTest, DontInsertDuplicatePreferred) { + std::string BarHeader = testPath("sub/bar.h"); + FS.Files[BarHeader] = ""; + FS.Files[MainFile] = R"cpp( +#include "sub/bar.h" // not shortest +)cpp"; + // Duplicate written. + EXPECT_EQ(calculate("\"original.h\"", "\"sub/bar.h\""), ""); + // Duplicate resolved. + EXPECT_EQ(calculate("\"original.h\"", BarHeader), ""); } TEST_F(HeadersTest, StillInsertIfTrasitivelyIncluded) { @@ -88,6 +113,17 @@ EXPECT_EQ(calculate(MainFile), ""); } +TEST_F(HeadersTest, PreferredHeader) { + FS.Files[MainFile] = ""; + std::string BarHeader = testPath("sub/bar.h"); + FS.Files[BarHeader] = ""; + EXPECT_EQ(calculate(BarHeader, ""), ""); + + std::string BazHeader = testPath("sub/baz.h"); + FS.Files[BazHeader] = ""; + EXPECT_EQ(calculate(BarHeader, BazHeader), "\"baz.h\""); +} + } // namespace } // namespace clangd } // namespace clang