[libcxx] [test] Account for differences in a trailing slash in weakly_canonical

[mstorsjo]

Summary

This seems to be a "documented" quirk in libc++'s implementation of
weakly_canonical (in a fixme comment in the weakly_canonical test).
Together with a difference between windows and posix regarding whether
paths can go through nonexistent dirs, this results in a difference in
a trailing slash. Just document this as expected.

[curdeius]

Wouldn't it be judicious to try to fix the underlying problem (as noted in the fixme comment)?
It seems to be of a similar nature as http://wg21.link/LWG3096, i.e. the fact whether (a part of) the path exists or not shouldn't matter.
WDYT?

[mstorsjo]

In D98642#2628622, @curdeius wrote:

Wouldn't it be judicious to try to fix the underlying problem (as noted in the fixme comment)?
It seems to be of a similar nature as http://wg21.link/LWG3096, i.e. the fact whether (a part of) the path exists or not shouldn't matter.
WDYT?

Well, maybe, but I'm afraid that might open a potentially big can of worms... Also, for reference, both MS STL and libstdc++ seem to have the same quirk regarding weakly_canonical sometimes returning paths with a trailing slash and sometimes not.

[Mordante]

In D98642#2628690, @mstorsjo wrote:

In D98642#2628622, @curdeius wrote:

Wouldn't it be judicious to try to fix the underlying problem (as noted in the fixme comment)?
It seems to be of a similar nature as http://wg21.link/LWG3096, i.e. the fact whether (a part of) the path exists or not shouldn't matter.
WDYT?

Well, maybe, but I'm afraid that might open a potentially big can of worms... Also, for reference, both MS STL and libstdc++ seem to have the same quirk regarding weakly_canonical sometimes returning paths with a trailing slash and sometimes not.

In that case I think it would be better to remove the FIXME and document the quirk including the fact that this matches the behaviour of MS STL and libstdc++.
WDYT?

[mstorsjo]

Degraded the fixme comment to a mere "note".

[Mordante]

I like the new comments!

[mstorsjo]

@curdeius WDYT about @Mordante 's suggestion to just degrade the fixme to a note, as the other implementations seem to have the exact same quirk?

[curdeius]

LGTM.

I must admit that I went down the rabbit hole a bit... But that's a different story and one might argue that the current behaviour is OK.

TL;DR:
The problem is in part in __weakly_canonical that should check whether the input path has (and so the output needs) a trailing separator.
The other part is in __canonical that calls ::realpath and this function, in turn, removes the trailing separator.

http://eel.is/c++draft/fs.op.weakly.canonical#2 says:

Postconditions: The returned path is in normal form ([fs.path.generic]).

http://eel.is/c++draft/fs.op.weakly.canonical#5:

Remarks: Implementations should avoid unnecessary normalization such as when canonical has already been called on the entirety of p.

So, I imply that canonical() should return the path in normal form.

But then, http://eel.is/c++draft/fs.op.canonical#1 says:

Effects: Converts p to an absolute path that has no symbolic link, dot, or dot-dot elements in its pathname in the generic format.

First of all, this phrase is ambiguous to me because it can be understood as either of:

1. "in the generic format" concerns elements in its pathname: There is no such element of the path p that would be in the generic format.
2. "in the generic format" concerns an absolute path: Converts p to an absolute path (...) in the generic format. So the result is in the generic format.

To be consistent with weakly_canonical, the interpretation should be 1.

Also, the normalization described in http://eel.is/c++draft/fs.path.generic#6 (as done by lexically_normal) does keep trailing separators (apart from the case where the path ends with "../" where it gets removed, cf. http://eel.is/c++draft/fs.path.generic#6.7).
Hence I would expect that weakly_canonical (and canonical) return normalized paths and keep trailing separators in the same way as lexically_normal does.
I see no reason for a distinction between existing and non-existing paths through the trailing separator.

So, the current tests for weakly_canonical seem wrong because they remove the trailing separator, specifically:

{static_env.SymlinkToDir / "dir2/.", static_env.Dir / "dir2"},
/// FIXME....
{static_env.SymlinkToDir / "dir2/./", static_env.Dir / "dir2"},

should be

{static_env.SymlinkToDir / "dir2/.", static_env.Dir / "dir2/"},
{static_env.SymlinkToDir / "dir2/./", static_env.Dir / "dir2/"},

I.e. having "path/." or "path/./" should not remove the separator after path.

The problem is in part in __weakly_canonical that should check whether the input path has (and so the output needs) a trailing separator.
The other part is in __canonical that calls ::realpath and this function, in turn, removes the trailing separator.

[mstorsjo]

In D98642#2637647, @curdeius wrote:

I must admit that I went down the rabbit hole a bit... But that's a different story and one might argue that the current behaviour is OK.

Thanks for doing the rabbit hole digging! That reinforces my initial feeling that I wouldn't want to go messing around in that unless strictly needed...

libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.exists/exists.pass.cpp

 }
 
 TEST_CASE(test_exist_not_found)
 {
     static_test_env static_env;
     const path p = static_env.DNE;
     TEST_CHECK(exists(p) == false);
 
+    TEST_CHECK(exists(static_env.Dir) == true);
+    TEST_CHECK(exists(static_env.Dir / "dne") == false);
+    // Whether <dir>/dne/.. is considered to exist or not is not necessarily
+    // something we need to define, but the platform specific behaviour
+    // does affect a few other tests, so clarify the root cause here.
+#ifdef _WIN32
+    TEST_CHECK(exists(static_env.Dir / "dne" / "..") == true);
+#else
+    TEST_CHECK(exists(static_env.Dir / "dne" / "..") == false);
+#endif
+
     std::error_code ec = GetTestEC();
     TEST_CHECK(exists(p, ec) == false);
     TEST_CHECK(!ec);
 }
 
 TEST_CASE(test_exists_fails)
 {
 #ifdef _WIN32

libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.relative/relative.pass.cpp

TEST_CASE(test_signature_8) {
   const fs::path output = fs::weakly_canonical(p);
   TEST_CHECK(output == fs::path::string_type(static_env.Dir2));
 }
 
 TEST_CASE(test_signature_9) {
   static_test_env static_env;
   fs::path p(static_env.SymlinkToDir / "dir2/../dir2/DNE/..");
   const fs::path output = fs::weakly_canonical(p);
+  // weakly_canonical has a quirk - if the path is considered to exist,
+  // it's returned without a trailing slash, otherwise it's returned with
+  // one (see a FIXME comment in
+  // fs.op.weakly_canonical/weakly_canonical.pass.cpp). On windows, a path
+  // like existent/nonexistentsubdir/.. is considered to exist, on posix
+  // it's considered to not exist. Therefore, the result here differs in
+  // the trailing slash.
+#ifdef _WIN32
+  TEST_CHECK(output == fs::path::string_type(static_env.Dir2));
+#else
   TEST_CHECK(output == fs::path::string_type(static_env.Dir2 / ""));
+#endif
 }
 
 TEST_CASE(test_signature_10) {
   static_test_env static_env;
   fs::path p(static_env.SymlinkToDir / "dir2/dir3/../DNE/DNE2");
   const fs::path output = fs::weakly_canonical(p);
   TEST_CHECK(output == fs::path::string_type(static_env.Dir2 / "DNE/DNE2"));
 }

libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.weakly_canonical/weakly_canonical.pass.cpp

fs::path expect;
       {static_env.Dir, static_env.Dir},
       {static_env.SymlinkToDir, static_env.Dir},
       {static_env.SymlinkToDir / "dir2/.", static_env.Dir / "dir2"},
       // FIXME? If the trailing separator occurs in a part of the path that exists,
       // it is omitted. Otherwise it is added to the end of the result.
       {static_env.SymlinkToDir / "dir2/./", static_env.Dir / "dir2"},
       {static_env.SymlinkToDir / "dir2/DNE/./", static_env.Dir / "dir2/DNE/"},
       {static_env.SymlinkToDir / "dir2", static_env.Dir2},
+#ifdef _WIN32
+      // On windows, this path is considered to exist (even though it
+      // passes through a nonexistent directory), and thus is returned
+      // without a trailing slash, see the fixme above.
+      {static_env.SymlinkToDir / "dir2/../dir2/DNE/..", static_env.Dir2},
+#else
       {static_env.SymlinkToDir / "dir2/../dir2/DNE/..", static_env.Dir2 / ""},
+#endif
       {static_env.SymlinkToDir / "dir2/dir3/../DNE/DNE2", static_env.Dir2 / "DNE/DNE2"},
       {static_env.Dir / "../dir1", static_env.Dir},
       {static_env.Dir / "./.", static_env.Dir},
       {static_env.Dir / "DNE/../foo", static_env.Dir / "foo"}
   };
   // clang-format on
   int ID = 0;
   bool Failed = false;