src: replace kPathSeparator with std::filesystem

[anonrig]

We don't need kPathSeparator anymore since std::filesystem::path::preferred_separator exists.

cc @nodejs/cpp-reviewers @nodejs/fs

[nodejs-github-bot]

Review requested:

• @nodejs/security-wg

[targos]

Error on Windows:

D:\a\node\node\src\compile_cache.cc(208,47): error C2676: binary '+': 'std::string' does not define this operator or a conversion to a type acceptable to the predefined operator [D:\a\node\node\libnode.vcxproj]

[jasnell]

LGTM once CI is green

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/59769/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/59845/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/59850/

[RafaelGSS]

I didn't test it, but from std::filesystem::path::operator/= it might cause an unexpected replacement:

// Where "//host" is a root-name
path("//host")  / "foo" // the result is      "//host/foo" (appends with separator)
path("//host/") / "foo" // the result is also "//host/foo" (appends without separator)
 
// On POSIX,
path("foo") / ""      // the result is "foo/" (appends)
path("foo") / "/bar"; // the result is "/bar" (replaces)

Refs: https://en.cppreference.com/w/cpp/filesystem/path/append.

Possibly, the "*" append won't cause any error, but I would be more confident if we use path::preferred_separator here.

[anonrig]

I can replace it, but FYI, we can use concat and it will always work correctly.

#include <iostream>
#include <filesystem>

int main() {
  auto path = std::filesystem::path("foo").concat("/bar");
  printf("path -> %s\n", path.string().c_str());
  return 0;
}

Prints:

path -> foo/bar

[RafaelGSS]

I suspect it would cause more CPU cycles though. IMO the + concat is easier

[anonrig]

Agreed. I'm changing this. Thank you for the review.

[anonrig]

Unfortunately there has been a complication. On Windows, preferred_separator returns "wchar_t*". Therefore, it is not possible to make a simple addition of wchar_t and std::string.

The solution to fix this is:

#ifdef _WIN32
      // This is required for Windows since prefered separator is "wchar_t".
      auto separator = std::filesystem::path::preferred_separator;
      return res + std::string(separator.begin(), separator.end()) + "*";
#else
      return res + std::filesystem::path::preferred_separator + "*";
#endif
    }

But since we are talking about potential solutions, I thought what about just converting this whole function to use std::filesystem, rather than libuv, and make the whole code a 5 line function?

std::string WildcardIfDir(const std::string& res) noexcept {
  auto path = std::filesystem::path(res);
  auto file_status = std::filesystem::status(path);
  if (file_status.type() == std::filesystem::file_type::directory) {
    path /= "*";
  }
  return path.string();
}

I personally prefer the std::filesystem, but wanted to hear @RafaelGSS @tniessen and @jasnell's thoughts on this.

[jasnell]

For now I think the first change is better. I think we'll want to carefully consider switching too much to std::filesystem until we can be certain it's implemented consistently across all the platforms we support (unless you're thinking of using this variation only on Windows)

[RafaelGSS]

I would not change it to the former either as std::filesystem is very likely to perform syscalls and the Permission Model operates on non-existing paths and it would be more difficult to handle. That's why I used std::string_view

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/59860/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/59899/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/59920/

[RafaelGSS]

As I previously mentioned ::operator/ might cause some unexpected results.

[aduh95]

What's the status here? Is this ready to land?

[anonrig]

What's the status here? Is this ready to land?

If @RafaelGSS's comment is a non-blocker, it is ready to land.

[RafaelGSS]

My comments aren't non-blocking. I can change it later if needed. LGTM.

[nodejs-github-bot]

Landed in c1dc307

[joyeecheung]

This PR seems to be mostly incorrect - it's passing utf-8 encoded chars into the std::filesystem::path constructors directly, which on Windows, expect the chars to be encoded in UTF-16 (so they suffer from the same bug as #54476)

[joyeecheung]

Another regression came up: #54991

I think we should consider reverting most of the changes here and other similar std::filesystem::path changes because:

1. Most of the times, the path content comes from UTF8 encoded data in a char array - either some environment variable, or CLI flag, or converted from a JS string/buffer
2. In most cases, the path will end up being passed into libuv, which also expects paths encoded in UTF8

Now on POSIX systems, it's fine to pretend that std::string/char[] and std::filesystem::path can be converted from each other automatically, but on Windows the native encoding is UTF-16 which means in 1, they must be converted into std::u8string/char8_t before being passed into the std::filesystem::path constructor (which leads to an overhead in UTF8 -> UTF16 transcoding), and in 2, they must be converted from path.u8string() into std::string/char[] (which leads to another UTF16 -> UTF8 transcoding) and should not be just path.string(). This not only incurs unnecessary overhead, but is also very easy to get overlooked and lead to the crashes that have been reported recently.

I think we should just keep it simple and prefer to always use std::string/char to carry UTF-8-encoded data for paths, unless it's a path it will later be passed into other std::filesystem APIs.

[richardlau]

From a Release POV Node.js 22 is due to become LTS next month (29 Oct) which likely means we'll only have at most two more current releases (possibly only one) for Node.js 22. The latest a Node.js 22 current release can go out is 15 Oct because we want "two weeks in current" (usual Release WG LTS policy) for commits in LTS (the 29 Oct release).

I'm +1 for reverting the changes.

[anonrig]

I'm +1 on reverting as well. It has done more harm than good.

[anonrig]

I'll open a PR to revert it soon.

[joyeecheung]

By the way, I think we can still keep the original intent of the replacement, just do it as constexpr char kPathSeparator = std::filesystem::path::preferred_separator and drop the ifdefs (I am not 100% sure this is equivalent, so up to you if you want to just revert it as-is).