Refactor canonical_path: simplify canonical_path::parent

I want canonical_path to work with both POSIX paths and Windows paths.
Make this easier by determining path components ahead of time rather
than when canonical_path::parent is called.

This commit should not change behavior, except on Windows for non-ASCII
paths. Non-ASCII paths are already broken though; see the TODO comment
in windows_path_canonicalizer::result.

Author: strager

src/file-canonical.cpp

@@ -98,6 +98,48 @@ struct canonicalizing_path_io_error {
 
 canonical_path::canonical_path(std::string &&path) : path_(std::move(path)) {
   QLJS_ASSERT(!this->path_.empty());
+
+#if QLJS_PATHS_POSIX
+  std::string_view p(this->path_);
+  while (!ends_with(p, "/")) {
+    std::size_t slash_index = p.find_last_of("/");
+    QLJS_ASSERT(slash_index != std::string::npos);
+    if (slash_index == 0 || slash_index == 1) {
+      // Preserve the slash for resulting root paths.
+      this->path_lengths_.push_back(slash_index + 1);
+      p = p.substr(0, slash_index + 1);
+    } else {
+      this->path_lengths_.push_back(slash_index);
+      p = p.substr(0, slash_index);
+    }
+  }
+  std::reverse(this->path_lengths_.begin(), this->path_lengths_.end());
+#elif QLJS_PATHS_WIN32
+  // TODO(strager): path_lengths_ is in UTF-16 code units, but it's interpreted
+  // as UTF-8 code units! Fix by storing a std::wstring in canonical_path
+  // instead of a std::string.
+  // TODO(strager): Avoid string conversions and copying.
+  std::optional<std::wstring> wpath = mbstring_to_wstring(this->path_.c_str());
+  QLJS_ASSERT(wpath.has_value());
+  for (;;) {
+    HRESULT result = ::PathCchRemoveFileSpec(wpath->data(), wpath->size() + 1);
+    switch (result) {
+    case S_OK:
+      this->path_lengths_.push_back(std::wcslen(wpath->data()));
+      break;
+    case S_FALSE:
+      // Path is a root path already.
+      goto done;
+    default:
+      QLJS_UNIMPLEMENTED();
+      break;
+    }
+  }
+done:
+  std::reverse(this->path_lengths_.begin(), this->path_lengths_.end());
+#else
+#error "Unsupported platform"
+#endif
 }
 
 std::string_view canonical_path::path() const &noexcept { return this->path_; }
@@ -135,6 +177,7 @@ bool operator!=(const canonical_path &lhs, std::string_view rhs) noexcept {
 }
 
 void canonical_path::append_component(std::string_view component) {
+  this->path_lengths_.push_back(this->path_.size());
   if (this->path_[this->path_.size() - 1] !=
       preferred_component_separator_char) {
     this->path_.push_back(preferred_component_separator_char);
@@ -143,39 +186,13 @@ void canonical_path::append_component(std::string_view component) {
 }
 
 bool canonical_path::parent() {
-#if QLJS_PATHS_POSIX
-  if (this->path_[this->path_.size() - 1] == '/') {
+  if (this->path_lengths_.empty()) {
     return false;
   }
-  std::size_t slash_index = this->path_.find_last_of("/");
-  QLJS_ASSERT(slash_index != std::string::npos);
-  if (slash_index == 0 || slash_index == 1) {
-    // Preserve the slash for resulting root paths.
-    this->path_.resize(slash_index + 1);
-  } else {
-    this->path_.resize(slash_index);
-  }
+  std::size_t path_length = this->path_lengths_.back();
+  this->path_lengths_.pop_back();
+  this->path_.resize(path_length);
   return true;
-#elif QLJS_PATHS_WIN32
-  // TODO(strager): Avoid string conversions and copying.
-  std::optional<std::wstring> wpath = mbstring_to_wstring(this->path_.c_str());
-  QLJS_ASSERT(wpath.has_value());
-  HRESULT result = ::PathCchRemoveFileSpec(wpath->data(), wpath->size() + 1);
-  switch (result) {
-  case S_OK:
-    wpath->resize(std::wcslen(wpath->data()));
-    this->path_ = std::filesystem::path(*wpath).string();
-    return true;
-  case S_FALSE:
-    // Path is a root path already.
-    return false;
-  default:
-    QLJS_UNIMPLEMENTED();
-    break;
-  }
-#else
-#error "Unsupported platform"
-#endif
 }
 
 canonical_path_result::canonical_path_result(std::string &&path,
@@ -207,6 +224,10 @@ bool canonical_path_result::have_missing_components() const noexcept {
 }
 
 void canonical_path_result::drop_missing_components() {
+  while (!this->path_.path_lengths_.empty() &&
+         this->path_.path_lengths_.back() >= this->existing_path_length_) {
+    this->path_.path_lengths_.pop_back();
+  }
   this->path_.path_.resize(this->existing_path_length_);
 }
 

src/quick-lint-js/file-canonical.h

@@ -15,6 +15,7 @@
 #include <quick-lint-js/result.h>
 #include <string>
 #include <string_view>
+#include <vector>
 
 namespace quick_lint_js {
 class canonical_path_result;
@@ -72,6 +73,15 @@ class canonical_path {
  private:
   std::string path_;
 
+  // On POSIX, if path_ is "/hello/world/x", then path_lengths_ is {1, 6, 12}:
+  //
+  //   /hello/world/x
+  //    ^    ^     ^
+  //    1    6     12
+  //
+  // If path_ is a root path, then path_lengths_ is empty.
+  std::vector<std::size_t> path_lengths_;
+
   friend canonical_path_result;
 };
 

test/test-file-canonical.cpp

@@ -145,6 +145,25 @@ TEST_F(test_file_canonical, canonical_path_to_non_existing_file_succeeds) {
   EXPECT_EQ(canonical->path(), temp_dir_canonical->path());
 }
 
+TEST_F(test_file_canonical,
+       parent_of_present_canonical_components_of_non_existing_file) {
+  std::string temp_dir = this->make_temporary_directory();
+  result<canonical_path_result, canonicalize_path_io_error> temp_dir_canonical =
+      canonicalize_path(temp_dir);
+  ASSERT_TRUE(temp_dir_canonical.ok())
+      << temp_dir_canonical.error().to_string();
+
+  create_directory(temp_dir + "/dir");
+  result<canonical_path_result, canonicalize_path_io_error> canonicalized =
+      canonicalize_path(temp_dir + "/dir/does-not-exist.txt");
+  ASSERT_TRUE(canonicalized.ok()) << canonicalized.error().to_string();
+  canonicalized->drop_missing_components();
+
+  canonical_path canonical = std::move(*canonicalized).canonical();
+  canonical.parent();
+  EXPECT_EQ(canonical.path(), temp_dir_canonical->path());
+}
+
 TEST_F(test_file_canonical,
        canonical_path_to_non_existing_parent_directory_succeeds) {
   std::string temp_dir = this->make_temporary_directory();
@@ -987,6 +1006,20 @@ TEST_F(test_file_canonical, append_component_posix) {
         << "before = " << tc.before << "\ncomponent = " << tc.component;
   }
 }
+
+TEST_F(test_file_canonical, remove_appended_component_posix) {
+  canonical_path p("/dir/subdir");
+  p.append_component("file.txt");
+  p.parent();
+  EXPECT_EQ(p.path(), "/dir/subdir");
+}
+
+TEST_F(test_file_canonical, remove_component_appended_to_root_posix) {
+  canonical_path p("/");
+  p.append_component("file.txt");
+  p.parent();
+  EXPECT_EQ(p.path(), "/");
+}
 #endif
 
 #if defined(_WIN32)
@@ -1039,6 +1072,20 @@ TEST_F(test_file_canonical, append_component_win32) {
         << "before = " << tc.before << "\ncomponent = " << tc.component;
   }
 }
+
+TEST_F(test_file_canonical, remove_appended_component_win32) {
+  canonical_path p(R"(X:\dir\subdir)");
+  p.append_component("file.txt");
+  p.parent();
+  EXPECT_EQ(p.path(), R"(X:\dir\subdir)");
+}
+
+TEST_F(test_file_canonical, remove_component_appended_to_root_win32) {
+  canonical_path p(R"(X:\)");
+  p.append_component("file.txt");
+  p.parent();
+  EXPECT_EQ(p.path(), R"(X:\)");
+}
 #endif
 }
 }