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

@@ -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<GroupMatch>(config_->includeCompletionWhitelist, config_->includeCompletionBlacklist);
@@ -124,44 +130,56 @@ void IncludeCompletion::Rescan() {
   });
 }
 
-void IncludeCompletion::AddFile(std::string path) {
+void IncludeCompletion::AddFile(const std::string& absolute_path) {
-  if (!EndsWithAny(path, config_->includeCompletionWhitelistLiteralEnding))
+  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);
+  std::string trimmed_path = absolute_path;
-  lsCompletionItem item = BuildCompletionItem(config_, path, use_angle_brackets, false /*is_stl*/);
+  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<std::mutex> 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<lsCompletionItem> result;
+  directory = NormalizePath(directory);
+  EnsureEndsInSlash(directory);
+
+  std::vector<CompletionCandidate> 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<std::mutex> 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<std::mutex> 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*/));
   }
 }

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<bool> is_scanning;
-  std::unordered_set<lsCompletionItem> completion_items;
+  std::vector<lsCompletionItem> 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<std::string> seen_paths;
 
   // Cached references
   Config* config_;

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<lsCompletionItem> {
-  std::size_t operator()(const lsCompletionItem& t) const {
-    if (t.textEdit)
-      return std::hash<std::string>()(t.textEdit->newText);
-    if (!t.insertText.empty())
-      return std::hash<std::string>()(t.insertText);
-    return std::hash<std::string>()(t.label);
-  }
-};
-}
 
 struct lsTextDocumentItem {
   // The text document's URI.

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<std::string> GetPlatformClangArguments();

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<std::string> GetPlatformClangArguments() {
   return {};
 }

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<std::string> GetPlatformClangArguments() {
   return {
     "-fms-compatibility",

src/utils.cc

@@ -1,5 +1,9 @@
 #include "utils.h"
 
+#include "platform.h"
+
+#include <tinydir.h>
+
 #include <algorithm>
 #include <cassert>
 #include <cctype>
@@ -10,8 +14,6 @@
 #include <sstream>
 #include <unordered_map>
 
-#include <tinydir.h>
-
 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
+          std::string child_dir = output_prefix + file.name + "/";
-          // this will loop infinitely. The above check handles that for us.
+          if (!IsSymLink(child_dir))
-          GetFilesInFolderHelper(file.path, true /*recursive*/, output_prefix + file.name + "/", handler);
+            GetFilesInFolderHelper(file.path, true /*recursive*/, child_dir, handler);
         }
       }
       else {