ioeric created this revision.
ioeric added a reviewer: sammccall.
Herald added subscribers: cfe-commits, jkorous-apple, ilya-biryukov, klimek.
Changes:
o Store both the original header and the canonical header in LSP command.
o Also check that both original and canonical headers are not already included
by comparing both resolved header path and written literal includes.
This addresses the use case where private IWYU pragma is defined in a private
header while it would still be preferrable to include the private header, in the
internal implementation file. If we have seen that the priviate header is
already
included, we don't try to insert the canonical include.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D43510
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/CodeComplete.cpp
clangd/Headers.cpp
clangd/Headers.h
clangd/Protocol.cpp
clangd/Protocol.h
test/clangd/insert-include.test
unittests/clangd/ClangdTests.cpp
unittests/clangd/HeadersTests.cpp
Index: unittests/clangd/HeadersTests.cpp
===================================================================
--- unittests/clangd/HeadersTests.cpp
+++ unittests/clangd/HeadersTests.cpp
@@ -24,14 +24,14 @@
}
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 = "") {
auto VFS = FS.getTaggedFileSystem(MainFile).Value;
auto Cmd = CDB.getCompileCommand(MainFile);
assert(static_cast<bool>(Cmd));
VFS->setCurrentWorkingDirectory(Cmd->Directory);
- auto Path =
- calculateIncludePath(MainFile, FS.Files[MainFile], Header, *Cmd, VFS);
+ auto Path = calculateIncludePath(MainFile, FS.Files[MainFile], Original,
+ Preferred, *Cmd, VFS);
if (!Path) {
llvm::consumeError(Path.takeError());
return std::string();
@@ -66,7 +66,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 +102,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, "<bar>"), "<bar>");
+
+ std::string BazHeader = testPath("sub/baz.h");
+ FS.Files[BazHeader] = "";
+ EXPECT_EQ(calculate(BarHeader, BazHeader), "\"baz.h\"");
+}
+
} // namespace
} // namespace clangd
} // namespace clang
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,17 +879,30 @@
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);
EXPECT_TRUE(static_cast<bool>(Replaces));
auto Changed = tooling::applyAllReplacements(Code, *Replaces);
EXPECT_TRUE(static_cast<bool>(Changed));
return llvm::StringRef(*Changed).contains(
(llvm::Twine("#include ") + Expected + "").str());
};
- EXPECT_TRUE(Inserted("\"y.h\"", "\"y.h\""));
- EXPECT_TRUE(Inserted("<string>", "<string>"));
+ EXPECT_TRUE(Inserted("\"y.h\"", /*Preferred=*/"","\"y.h\""));
+ EXPECT_TRUE(Inserted("\"y.h\"", /*Preferred=*/"\"Y.h\"", "\"Y.h\""));
+ EXPECT_TRUE(Inserted("<string>", /*Preferred=*/"", "<string>"));
+ EXPECT_TRUE(Inserted("<string>", /*Preferred=*/"", "<string>"));
+
+ 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=*/"<Y.h>", "<Y.h>"));
+ EXPECT_TRUE(Inserted(OriginalHeader, PreferredHeader, "\"Y.h\""));
+ EXPECT_TRUE(Inserted(/*Original=*/"", PreferredHeader, "\"Y.h\""));
+ EXPECT_TRUE(Inserted(/*Original=*/"<y.h>", PreferredHeader, "\"Y.h\""));
}
} // namespace
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":"<vector>","textDocument":{"uri":"test:///main.cpp"}}],"command":"clangd.insertInclude"}}
+{"jsonrpc":"2.0","id":3,"method":"workspace/executeCommand","params":{"arguments":[{"originalHeader":"\"/usr/include/bits/vector\"", "preferredHeader":"<vector>","textDocument":{"uri":"test:///main.cpp"}}],"command":"clangd.insertInclude"}}
# CHECK: "id": 3,
# CHECK-NEXT: "jsonrpc": "2.0",
-# CHECK-NEXT: "result": "Inserted header <vector>"
+# CHECK-NEXT: "result": "Inserted header \"/usr/include/bits/vector\" (<vector>)"
# CHECK-NEXT: }
# CHECK: "method": "workspace/applyEdit",
# CHECK-NEXT: "params": {
Index: clangd/Protocol.h
===================================================================
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -427,11 +427,19 @@
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 original header corresponding to this insertion. This may be different
+ /// from preferredHeader 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.
+ std::string originalHeader;
+ /// The preferred header to be inserted. The has the same semantic as
+ /// originalHeader.
+ /// If empty, originalHeader 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("originalHeader", R.originalHeader) &&
+ 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},
+ {"originalHeader", II.originalHeader},
+ {"preferredHeader", II.preferredHeader}};
}
json::Expr toJSON(const ApplyWorkspaceEditParams &Params) {
Index: clangd/Headers.h
===================================================================
--- clangd/Headers.h
+++ clangd/Headers.h
@@ -19,18 +19,30 @@
namespace clang {
namespace clangd {
+/// Returns true if \p Include is literal include like "path" or <path>.
+bool isLiteralInclude(llvm::StringRef Include);
+
/// 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'.
///
+/// If non-empty, \p PreferredHeader is used to calculate the include path;
+/// otherwise, \p OriginalHeader is used.
+///
/// \param File is an absolute file path.
-/// \param Header is an absolute file path.
-/// \return A quoted "path" or <path>. 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 OriginalHeader is the original header corresponding to \p
+/// PreferredHeader. If non-empty, it is either an absolute path or a
+/// literal include like "header" or <header>.
+/// \param PreferredHeader The preferred header to be included, if non-empty.
+/// the semantic is the same as \p OriginalHeader.
+// \return A quoted "path" or <path>. This returns an empty string if:
+/// - Either \p OriginalHeader or \p PreferredHeader is already (directly)
+/// included in the file (including those included via different paths).
+/// - \p OriginalHeader or \p PreferredHeader is the same as \p File.
llvm::Expected<std::string>
-calculateIncludePath(PathRef File, llvm::StringRef Code, llvm::StringRef Header,
+calculateIncludePath(PathRef File, llvm::StringRef Code,
+ llvm::StringRef OriginalHeader,
+ llvm::StringRef PreferredHeader,
const tooling::CompileCommand &CompileCommand,
IntrusiveRefCntPtr<vfs::FileSystem> FS);
Index: clangd/Headers.cpp
===================================================================
--- clangd/Headers.cpp
+++ clangd/Headers.cpp
@@ -24,36 +24,52 @@
class RecordHeaders : public PPCallbacks {
public:
- RecordHeaders(std::set<std::string> &Headers) : Headers(Headers) {}
+ RecordHeaders(std::set<std::string> &WrittenHeaders,
+ std::set<std::string> &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<std::string> &Headers;
+ std::set<std::string> &WrittenHeaders;
+ std::set<std::string> &ResolvedHeaders;
};
} // namespace
+bool isLiteralInclude(llvm::StringRef Include) {
+ return Include.startswith("<") || Include.startswith("\"");
+}
+
/// FIXME(ioeric): we might not want to insert an absolute include path if the
/// path is not shortened.
llvm::Expected<std::string>
calculateIncludePath(llvm::StringRef File, llvm::StringRef Code,
- llvm::StringRef Header,
+ llvm::StringRef OriginalHeader,
+ llvm::StringRef PreferredHeader,
const tooling::CompileCommand &CompileCommand,
IntrusiveRefCntPtr<vfs::FileSystem> FS) {
- assert(llvm::sys::path::is_absolute(File) &&
- llvm::sys::path::is_absolute(Header));
+ assert(llvm::sys::path::is_absolute(File));
+ auto IsValidHeader = [](llvm::StringRef Header) {
+ return Header.empty() || isLiteralInclude(Header) ||
+ llvm::sys::path::is_absolute(Header);
+ };
+ assert(IsValidHeader(PreferredHeader) && IsValidHeader(OriginalHeader));
- if (File == Header)
+ if (File == OriginalHeader || File == PreferredHeader)
+ return "";
+ if (OriginalHeader.empty() && PreferredHeader.empty())
return "";
FS->setCurrentWorkingDirectory(CompileCommand.Directory);
@@ -95,29 +111,40 @@
return llvm::make_error<llvm::StringError>(
"Failed to begin preprocessor only action for file " + File,
llvm::inconvertibleErrorCode());
- std::set<std::string> ExistingHeaders;
+ std::set<std::string> WrittenHeaders;
+ std::set<std::string> ResolvedHeaders;
Clang->getPreprocessor().addPPCallbacks(
- llvm::make_unique<RecordHeaders>(ExistingHeaders));
+ llvm::make_unique<RecordHeaders>(WrittenHeaders, ResolvedHeaders));
if (!Action.Execute())
return llvm::make_error<llvm::StringError>(
"Failed to execute preprocessor only action for file " + File,
llvm::inconvertibleErrorCode());
- if (ExistingHeaders.find(Header) != ExistingHeaders.end()) {
+ auto Included = [&](llvm::StringRef Header) {
+ return WrittenHeaders.find(Header) != WrittenHeaders.end() ||
+ ResolvedHeaders.find(Header) != ResolvedHeaders.end();
+ };
+ if (Included(OriginalHeader) || Included(PreferredHeader))
return llvm::make_error<llvm::StringError>(
- Header + " is already included in " + File,
+ OriginalHeader + "(" + PreferredHeader + ") is already included in " +
+ File,
llvm::inconvertibleErrorCode());
- }
auto &HeaderSearchInfo = Clang->getPreprocessor().getHeaderSearchInfo();
bool IsSystem = false;
+
+ llvm::StringRef ToInclude =
+ PreferredHeader.empty() ? OriginalHeader : PreferredHeader;
+ if (isLiteralInclude(ToInclude))
+ return ToInclude;
+
std::string Suggested = HeaderSearchInfo.suggestPathToFileForDiagnostics(
- Header, CompileCommand.Directory, &IsSystem);
+ ToInclude, CompileCommand.Directory, &IsSystem);
if (IsSystem)
Suggested = "<" + Suggested + ">";
else
Suggested = "\"" + Suggested + "\"";
- log("Suggested #include for " + Header + " is: " + Suggested);
+ log("Suggested #include for " + ToInclude + " is: " + Suggested);
return Suggested;
}
Index: clangd/CodeComplete.cpp
===================================================================
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -301,9 +301,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.originalHeader = IndexResult->CanonicalDeclaration.FileURI;
+ Insertion.preferredHeader = D->IncludeHeader;
Insertion.textDocument.uri = URIForFile(FileName);
Cmd.includeInsertion = std::move(Insertion);
I.command = std::move(Cmd);
Index: clangd/ClangdServer.h
===================================================================
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -235,12 +235,17 @@
UniqueFunction<void(Expected<std::vector<tooling::Replacement>>)>
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 OriginalHeader The original header corresponding to this insertion.
+ /// This may be different from preferredHeader 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.
+ /// \p PreferredHeader The preferred header to be inserted. The has the same
+ /// semantic as \p OriginalHeader. If empty, \p OriginalHeader is used to
+ /// calculate the #include path.
Expected<tooling::Replacements> insertInclude(PathRef File, StringRef Code,
- StringRef Header);
+ StringRef OriginalHeader,
+ StringRef PreferredHeader);
/// 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
@@ -315,29 +315,35 @@
Expected<tooling::Replacements>
ClangdServer::insertInclude(PathRef File, StringRef Code,
- llvm::StringRef Header) {
- std::string ToInclude;
- if (Header.startswith("<") || Header.startswith("\"")) {
- ToInclude = Header;
- } else {
+ StringRef OriginalHeader,
+ StringRef PreferredHeader) {
+ auto Resolve = [File](StringRef Header) -> llvm::Expected<std::string> {
+ if (Header.empty() || Header.startswith("<") || Header.startswith("\""))
+ return Header;
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);
- }
+ return *Resolved;
+ };
+ std::string ToInclude;
+ auto ResolvedOrginal = Resolve(OriginalHeader);
+ if (!ResolvedOrginal)
+ return ResolvedOrginal.takeError();
+ auto ResolvedPreferred = Resolve(PreferredHeader);
+ 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/ClangdLSPServer.cpp
===================================================================
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -189,12 +189,15 @@
ExecuteCommandParams::CLANGD_INSERT_HEADER_INCLUDE +
" called on non-added file " + FileURI.file())
.str());
- auto Replaces = Server.insertInclude(FileURI.file(), *Code,
- Params.includeInsertion->header);
+ auto Replaces = Server.insertInclude(
+ FileURI.file(), *Code, Params.includeInsertion->originalHeader,
+ Params.includeInsertion->preferredHeader);
if (!Replaces) {
std::string ErrMsg =
("Failed to generate include insertion edits for adding " +
- Params.includeInsertion->header + " into " + FileURI.file())
+ Params.includeInsertion->originalHeader + " (" +
+ Params.includeInsertion->preferredHeader + ") into " +
+ FileURI.file())
.str();
log(ErrMsg + ":" + llvm::toString(Replaces.takeError()));
replyError(ErrorCode::InternalError, ErrMsg);
@@ -204,7 +207,8 @@
WorkspaceEdit WE;
WE.changes = {{FileURI.uri(), Edits}};
- reply("Inserted header " + Params.includeInsertion->header);
+ reply("Inserted header " + Params.includeInsertion->originalHeader + " (" +
+ Params.includeInsertion->preferredHeader + ")");
ApplyEdit(std::move(WE));
} else {
// We should not get here because ExecuteCommandParams would not have
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
