From 37787290cb105253c3ac641e133ef20891dbfac9 Mon Sep 17 00:00:00 2001 From: Jacob Dufault Date: Tue, 23 May 2017 00:24:14 -0700 Subject: [PATCH] Make sure every include candidate is unique w.r.t. absolute path. Also do not follow symlinks when iterating a directory. --- src/include_completion.cc | 54 ++++++++++++++++++++++++++------------- src/include_completion.h | 10 +++++--- src/language_server_api.h | 19 -------------- src/platform.h | 2 ++ src/platform_linux.cc | 7 +++++ src/platform_win.cc | 4 +++ src/utils.cc | 15 +++++++---- 7 files changed, 66 insertions(+), 45 deletions(-) diff --git a/src/include_completion.cc b/src/include_completion.cc index 361dfff3..8360acd6 100644 --- a/src/include_completion.cc +++ b/src/include_completion.cc @@ -10,6 +10,11 @@ namespace { +struct CompletionCandidate { + std::string absolute_path; + lsCompletionItem completion_item; +}; + std::string ElideLongPath(Config* config, const std::string& path) { if (config->includeCompletionMaximumPathLength <= 0) return path; @@ -41,33 +46,33 @@ size_t TrimCommonPathPrefix(const std::string& result, const std::string& trimme } // Returns true iff angle brackets should be used. -bool TrimPath(Project* project, const std::string& project_root, std::string& insert_path) { +bool TrimPath(Project* project, const std::string& project_root, std::string* insert_path) { size_t start = 0; bool angle = false; - size_t len = TrimCommonPathPrefix(insert_path, project_root); + size_t len = TrimCommonPathPrefix(*insert_path, project_root); if (len > start) start = len; for (auto& include_dir : project->quote_include_directories) { - len = TrimCommonPathPrefix(insert_path, include_dir); + len = TrimCommonPathPrefix(*insert_path, include_dir); if (len > start) start = len; } for (auto& include_dir : project->angle_include_directories) { - len = TrimCommonPathPrefix(insert_path, include_dir); + len = TrimCommonPathPrefix(*insert_path, include_dir); if (len > start) { start = len; angle = true; } } - insert_path = insert_path.substr(start); + *insert_path = insert_path->substr(start); return angle; } -lsCompletionItem BuildCompletionItem(Config* config, std::string path, bool use_angle_brackets, bool is_stl) { +lsCompletionItem BuildCompletionItem(Config* config, const std::string& path, bool use_angle_brackets, bool is_stl) { lsCompletionItem item; if (use_angle_brackets) item.label = "#include <" + ElideLongPath(config, path) + ">"; @@ -103,6 +108,7 @@ void IncludeCompletion::Rescan() { return; completion_items.clear(); + seen_paths.clear(); if (!match_ && (!config_->includeCompletionWhitelist.empty() || !config_->includeCompletionBlacklist.empty())) match_ = MakeUnique(config_->includeCompletionWhitelist, config_->includeCompletionBlacklist); @@ -124,44 +130,56 @@ void IncludeCompletion::Rescan() { }); } -void IncludeCompletion::AddFile(std::string path) { - if (!EndsWithAny(path, config_->includeCompletionWhitelistLiteralEnding)) +void IncludeCompletion::AddFile(const std::string& absolute_path) { + if (!EndsWithAny(absolute_path, config_->includeCompletionWhitelistLiteralEnding)) return; - if (match_ && !match_->IsMatch(path)) + if (match_ && !match_->IsMatch(absolute_path)) return; - bool use_angle_brackets = TrimPath(project_, config_->projectRoot, path); - lsCompletionItem item = BuildCompletionItem(config_, path, use_angle_brackets, false /*is_stl*/); + std::string trimmed_path = absolute_path; + bool use_angle_brackets = TrimPath(project_, config_->projectRoot, &trimmed_path); + lsCompletionItem item = BuildCompletionItem(config_, trimmed_path, use_angle_brackets, false /*is_stl*/); if (is_scanning) { std::lock_guard lock(completion_items_mutex); - completion_items.insert(item); + if (seen_paths.insert(absolute_path).second) + completion_items.push_back(item); } else { - completion_items.insert(item); + if (seen_paths.insert(absolute_path).second) + completion_items.push_back(item); } } void IncludeCompletion::InsertIncludesFromDirectory( - const std::string& directory, + std::string directory, bool use_angle_brackets) { - std::vector result; + directory = NormalizePath(directory); + EnsureEndsInSlash(directory); + + std::vector results; GetFilesInFolder(directory, true /*recursive*/, false /*add_folder_to_path*/, [&](const std::string& path) { if (!EndsWithAny(path, config_->includeCompletionWhitelistLiteralEnding)) return; if (match_ && !match_->IsMatch(directory + path)) return; - result.push_back(BuildCompletionItem(config_, path, use_angle_brackets, false /*is_stl*/)); + CompletionCandidate candidate; + candidate.absolute_path = directory + path; + candidate.completion_item = BuildCompletionItem(config_, path, use_angle_brackets, false /*is_stl*/); + results.push_back(candidate); }); std::lock_guard lock(completion_items_mutex); - completion_items.insert(result.begin(), result.end()); + for (const CompletionCandidate& result : results) { + if (seen_paths.insert(result.absolute_path).second) + completion_items.push_back(result.completion_item); + } } void IncludeCompletion::InsertStlIncludes() { std::lock_guard lock(completion_items_mutex); for (const char* stl_header : kStandardLibraryIncludes) { - completion_items.insert(BuildCompletionItem(config_, stl_header, true /*use_angle_brackets*/, true /*is_stl*/)); + completion_items.push_back(BuildCompletionItem(config_, stl_header, true /*use_angle_brackets*/, true /*is_stl*/)); } } diff --git a/src/include_completion.h b/src/include_completion.h index 9eb45f1e..28796c78 100644 --- a/src/include_completion.h +++ b/src/include_completion.h @@ -16,17 +16,21 @@ struct IncludeCompletion { void Rescan(); // Ensures the one-off file is inside |completion_items|. - void AddFile(std::string path); + void AddFile(const std::string& absolute_path); // Scans the given directory and inserts all includes from this. This is a // blocking function and should be run off the querydb thread. - void InsertIncludesFromDirectory(const std::string& directory, bool use_angle_brackets); + void InsertIncludesFromDirectory(std::string directory, bool use_angle_brackets); void InsertStlIncludes(); // Guards |completion_items| when |is_scanning| is true. std::mutex completion_items_mutex; std::atomic is_scanning; - std::unordered_set completion_items; + std::vector completion_items; + // Paths inside of |completion_items|. Multiple paths could show up the same + // time, but with different bracket types, so we have to hash on the absolute + // path, and not what we insert into the completion results. + std::unordered_set seen_paths; // Cached references Config* config_; diff --git a/src/language_server_api.h b/src/language_server_api.h index 16edc5de..f8f296e2 100644 --- a/src/language_server_api.h +++ b/src/language_server_api.h @@ -460,14 +460,6 @@ struct lsCompletionItem { // An data entry field that is preserved on a completion item between // a completion and a completion resolve request. // data ? : any - - inline bool operator==(const lsCompletionItem& other) const { - if (textEdit && other.textEdit) - return textEdit->newText == other.textEdit->newText; - if (!insertText.empty()) - return insertText == other.insertText; - return label == other.label; - } }; MAKE_REFLECT_STRUCT(lsCompletionItem, label, @@ -478,17 +470,6 @@ MAKE_REFLECT_STRUCT(lsCompletionItem, insertText, insertTextFormat, textEdit); -namespace std { -template<> struct hash { - std::size_t operator()(const lsCompletionItem& t) const { - if (t.textEdit) - return std::hash()(t.textEdit->newText); - if (!t.insertText.empty()) - return std::hash()(t.insertText); - return std::hash()(t.label); - } -}; -} struct lsTextDocumentItem { // The text document's URI. diff --git a/src/platform.h b/src/platform.h index c4abcf53..a5c33032 100644 --- a/src/platform.h +++ b/src/platform.h @@ -41,5 +41,7 @@ int64_t GetLastModificationTime(const std::string& absolute_path); void CopyFileTo(const std::string& destination, const std::string& source); +bool IsSymLink(const std::string& path); + // Returns any clang arguments that are specific to the current platform. std::vector GetPlatformClangArguments(); \ No newline at end of file diff --git a/src/platform_linux.cc b/src/platform_linux.cc index 28c34a7f..452c3beb 100644 --- a/src/platform_linux.cc +++ b/src/platform_linux.cc @@ -227,6 +227,13 @@ out_error: close(fd_to); } +bool IsSymLink(const std::string& path) { + struct stat buf; + int result; + result = lstat(path.c_str(), &buf); + return result == 0; +} + std::vector GetPlatformClangArguments() { return {}; } diff --git a/src/platform_win.cc b/src/platform_win.cc index 7bd1d429..2cc2835e 100644 --- a/src/platform_win.cc +++ b/src/platform_win.cc @@ -207,6 +207,10 @@ void CopyFileTo(const std::string& destination, const std::string& source) { false /*failIfExists*/); } +bool IsSymLink(const std::string& path) { + return false; +} + std::vector GetPlatformClangArguments() { return { "-fms-compatibility", diff --git a/src/utils.cc b/src/utils.cc index a0e7fd50..55a014d3 100644 --- a/src/utils.cc +++ b/src/utils.cc @@ -1,5 +1,9 @@ #include "utils.h" +#include "platform.h" + +#include + #include #include #include @@ -10,8 +14,6 @@ #include #include -#include - namespace { // See http://stackoverflow.com/a/217605 @@ -120,12 +122,15 @@ static void GetFilesInFolderHelper( } // Skip all dot files. + // We must always ignore the '.' and '..' directories, otherwise + // this will loop infinitely. + // The nested ifs are intentional, branching order is subtle here. if (file.name[0] != '.') { if (file.is_dir) { if (recursive) { - // Note that we must always ignore the '.' and '..' directories, otherwise - // this will loop infinitely. The above check handles that for us. - GetFilesInFolderHelper(file.path, true /*recursive*/, output_prefix + file.name + "/", handler); + std::string child_dir = output_prefix + file.name + "/"; + if (!IsSymLink(child_dir)) + GetFilesInFolderHelper(file.path, true /*recursive*/, child_dir, handler); } } else {