[libc++] [P1164] [C++20] Make fs::create_directory() error if there is already a non-directory.

[curdeius]

Summary

Also mark LWG2935 and LWG3079 as complete.

Applied retroactively to previous standards too, as it's a DR.

• https://wg21.link/P1164
• https://wg21.link/lwg2935
• https://wg21.link/lwg3079

[curdeius]

libcxx/src/filesystem/operations.cpp

858

Question to reviewers, what should we return, error_code corresponding to errno (mec) or another error code, e.g not_a_directory?

888

I'll remove this line in the next revision.

[ldionne]

I'm not sure if it should be applied retroactively to C++17 too (as I've done currently).

Yes, we apply LWG resolutions retroactively to all standards.

Also, you'll need markup for some Apple system libraries that shipped Filesystem but don't contain that fix. They are not covered by CI right now but they will once I land https://reviews.llvm.org/D92794. I'll let you know and you can rebase on top of it -- it should be trivial to add the UNSUPPORTED annotations.

libcxx/docs/Cxx2aStatusIssuesStatus.csv

18

Isn't that "Nothing to do"?

86

Same here -- isn't that "Nothing to do"?

libcxx/src/filesystem/operations.cpp

858

I believe an error code corresponding to errno is the most sensible thing, since that's effectively surfacing the actual failure reported by mkdir to the user, which is what we want IMO. Also, the Standard gives us leeway here, it only seems to say that we need to report an error -- so either way appears to be conforming.

[curdeius]

• Mark LWG issues as Nothing To Do.
• Remove TODOs.

[curdeius]

OK. I'll wait for D92794 to rebase and update.

libcxx/docs/Cxx2aStatusIssuesStatus.csv

18

Good catch. Updated accordingly.

86

Ditto.

libcxx/src/filesystem/operations.cpp

858

Ok. Thanks.

[curdeius]

• Rebase.

[curdeius]

@ldionne, I was expecting tests to fail, but somehow they are already listed as unsupported on macOS 10.14...

https://buildkite.com/llvm-project/libcxx-ci/builds/671#028852c7-7ffc-4d95-8455-62d0d4c6c05e/6-545

libc++ :: std/input.output/filesystems/fs.op.funcs/fs.op.create_directory/create_directory.pass.cpp
libc++ :: std/input.output/filesystems/fs.op.funcs/fs.op.create_directory/create_directory_with_attributes.pass.cpp

How is that?

[curdeius]

Oh, I got my answer...
libcxx/test/std/input.output/filesystems/lit.local.cfg is:

if 'c++filesystem-disabled' in config.available_features:
  config.unsupported = True

But... haven't you said that filesystem landed in macOS 10.14?

And I see this in 10.14 build log:

llvm-lit: /tmp/buildkite-builds/mbp-2-local-1/llvm-project/libcxx-ci/libcxx/utils/libcxx/test/config.py:146: note: All available features: {'locale.fr_CA.ISO8859-1', 'modules-support', 'with_system_cxx_lib=macosx', 'libc++', 'apple-clang-12.0', 'thread-safety', 'long_tests', 'locale.fr_FR.UTF-8', 'fcoroutines-ts', '-faligned-allocation', 'locale.zh_CN.UTF-8', '-fsized-deallocation', 'libcxx-no-debug-mode', 'fdelayed-template-parsing', 'apple-clang-12.0.0', 'x86_64-apple', 'c++filesystem-disabled', '__dummy_use_system_cxx_lib', 'libcpp-no-concepts', 'locale.ru_RU.UTF-8', 'apple-clang', 'has-fobjc-arc', 'libcpp-abi-version=1', 'apple-clang-12', 'with_system_cxx_lib=x86_64-apple-macosx10.14', 'use_system_cxx_lib', 'target-x86_64', 'locale.en_US.UTF-8', 'with_system_cxx_lib=macosx10.14', 'locale.cs_CZ.ISO8859-2', 'objective-c++', 'diagnose-if-support', 'darwin', 'libcpp-has-thread-api-pthread', 'c++2a', 'has-fblocks'}

so, there is 'c++filesystem-disabled'.
Seems like a bug, nope?

[ldionne]

In D92769#2442014, @curdeius wrote:

Oh, I got my answer...
libcxx/test/std/input.output/filesystems/lit.local.cfg is:

if 'c++filesystem-disabled' in config.available_features:
  config.unsupported = True

But... haven't you said that filesystem landed in macOS 10.14?

I apologize for the confusion, I made a mistake: filesystem was introduced in macOS 10.15, not 10.14. Fixing in https://reviews.llvm.org/D92937.

[curdeius]

• Rebase to trigger CI on top of parent revision D92937.

[curdeius]

• Mark XFAIL on macosx10.15.

[ldionne]

LGTM once CI passes!

[mstorsjo]

Note that P1164 talks about both create_directory and create_directories, while this fix only covers create_directory.

[curdeius]

In D92769#2444967, @mstorsjo wrote:

Note that P1164 talks about both create_directory and create_directories, while this fix only covers create_directory.

Indeed, but it doesn't change the behaviour of create_directories per se because it calls create_directory for every part of the path.
(Actually it changes a phrase from the "Returns" clause that was actually implying a contradiction on what should really be returned, but libc++ was doing already the right thing.)

Anyway, I'll add a test for create_directories in a follow-up patch.

[mstorsjo]

In D92769#2445438, @curdeius wrote:

In D92769#2444967, @mstorsjo wrote:

Note that P1164 talks about both create_directory and create_directories, while this fix only covers create_directory.

Indeed, but it doesn't change the behaviour of create_directories per se because it calls create_directory for every part of the path.

Ah, right.

After revisiting the issue I was, it was that while create_directories does report failures, it reports a different error code than expected - at least with the windows filesystem APIs. (But maybe the testcase I'm running, one picked from MS STL's testsuite, is overly strict on checking the specific error codes?) I'll comment inline to point out and explain the issue.

libcxx/src/filesystem/operations.cpp

844

If parent does exist, but isn't a directory, the check if (not exists(parent_st)) above won't trigger, so it won't hit the case of calling __create_directory("parent_which_is_file"), but will instead call __create_directory("parent_which_is_file/subdir"). That call doesn't return the error code corresponding to EEXIST, but gives an error corresponding to no_such_file_or_directory/ENOENT instead.

By extending the check above into if (not exists(parent_st)) { ... } else if (!is_directory(parent)) return err.report(errc::file_exists);, one would get the more expected error code here too.

[curdeius]

libcxx/src/filesystem/operations.cpp

844

Moved discussion to D93026.

844

If parent does exist, but isn't a directory, the check if (not exists(parent_st)) above won't trigger, so it won't hit the case of calling __create_directory("parent_which_is_file"), but will instead call __create_directory("parent_which_is_file/subdir"). That call doesn't return the error code corresponding to EEXIST, but gives an error corresponding to no_such_file_or_directory/ENOENT instead.

By extending the check above into if (not exists(parent_st)) { ... } else if (!is_directory(parent)) return err.report(errc::file_exists);, one would get the more expected error code here too.

libcxx/docs/Cxx2aStatusIssuesStatus.csv

 "`2942 <https://wg21.link/LWG2942>`__","LWG 2873's resolution missed ``weak_ptr::owner_before``\ ","Toronto","|Complete|",""
 "`2954 <https://wg21.link/LWG2954>`__","Specialization of the convenience variable templates should be prohibited","Toronto","|Complete|",""
 "`2961 <https://wg21.link/LWG2961>`__","Bad postcondition for ``set_default_resource``\ ","Toronto","",""
 "`2966 <https://wg21.link/LWG2966>`__","Incomplete resolution of US 74","Toronto","|Nothing To Do|",""
 "`2974 <https://wg21.link/LWG2974>`__","Diagnose out of bounds ``tuple_element/variant_alternative``\ ","Toronto","|Complete|",""
 "","","","",""
 "`2779 <https://wg21.link/LWG2779>`__","[networking.ts] Relax requirements on buffer sequence iterators","Albuquerque","",""
 "`2870 <https://wg21.link/LWG2870>`__","Default value of parameter theta of polar should be dependent","Albuquerque","|Complete|",""
-"`2935 <https://wg21.link/LWG2935>`__","What should create_directories do when p already exists but is not a directory?","Albuquerque","",""
+"`2935 <https://wg21.link/LWG2935>`__","What should create_directories do when p already exists but is not a directory?","Albuquerque","|Nothing To Do|",""
 "`2941 <https://wg21.link/LWG2941>`__","[thread.req.timing] wording should apply to both member and namespace-level functions","Albuquerque","|Nothing To Do|",""
 "`2944 <https://wg21.link/LWG2944>`__","LWG 2905 accidentally removed requirement that construction of the deleter doesn't throw an exception","Albuquerque","|Nothing To Do|",""
 "`2945 <https://wg21.link/LWG2945>`__","Order of template parameters in optional comparisons","Albuquerque","|Complete|",""
 "`2948 <https://wg21.link/LWG2948>`__","unique_ptr does not define operator<< for stream output","Albuquerque","|Complete|",""
 "`2950 <https://wg21.link/LWG2950>`__","std::byte operations are misspecified","Albuquerque","|Complete|",""
 "`2952 <https://wg21.link/LWG2952>`__","iterator_traits should work for pointers to cv T","Albuquerque","|Complete|",""
 "`2953 <https://wg21.link/LWG2953>`__","LWG 2853 should apply to deque::erase too","Albuquerque","|Complete|",""
 "`2958 <https://wg21.link/LWG2958>`__","Moves improperly defined as deleted","Albuquerque","*We already do this*",""

 "`2139 <https://wg21.link/LWG2139>`__","What is a user-defined type?","Rapperswil","",""
 "`2970 <https://wg21.link/LWG2970>`__","Return type of std::visit misspecified","Rapperswil","",""
 "`3058 <https://wg21.link/LWG3058>`__","Parallel adjacent_difference shouldn't require creating temporaries","Rapperswil","",""
 "`3062 <https://wg21.link/LWG3062>`__","Unnecessary decay_t in is_execution_policy_v should be remove_cvref_t","Rapperswil","",""
 "`3067 <https://wg21.link/LWG3067>`__","recursive_directory_iterator::pop must invalidate","Rapperswil","|Nothing To Do|",""
 "`3071 <https://wg21.link/LWG3071>`__","[networking.ts] read_until still refers to ""input sequence""","Rapperswil","|Nothing To Do|",""
 "`3074 <https://wg21.link/LWG3074>`__","Non-member functions for valarray should only deduce from the valarray","Rapperswil","",""
 "`3076 <https://wg21.link/LWG3076>`__","basic_string CTAD ambiguity","Rapperswil","|Complete|",""
-"`3079 <https://wg21.link/LWG3079>`__","LWG 2935 forgot to fix the existing_p overloads of create_directory","Rapperswil","",""
+"`3079 <https://wg21.link/LWG3079>`__","LWG 2935 forgot to fix the existing_p overloads of create_directory","Rapperswil","|Nothing To Do|",""
 "`3080 <https://wg21.link/LWG3080>`__","Floating point from_chars pattern specification breaks round-tripping","Rapperswil","",""
 "`3083 <https://wg21.link/LWG3083>`__","What should ios::iword(-1) do?","Rapperswil","|Nothing To Do|",""
 "`3094 <https://wg21.link/LWG3094>`__","[time.duration.io]p4 makes surprising claims about encoding","Rapperswil","",""
 "`3100 <https://wg21.link/LWG3100>`__","Unnecessary and confusing ""empty span"" wording","Rapperswil","|Nothing To Do|",""
 "`3102 <https://wg21.link/LWG3102>`__","Clarify span iterator and ``const_iterator`` behavior","Rapperswil","|Complete|",""
 "`3104 <https://wg21.link/LWG3104>`__","Fixing duration division","Rapperswil","|Complete|",""
 "","","","",""
 "`2183 <https://wg21.link/LWG2183>`__","Muddled allocator requirements for ``match_results``\  constructors","San Diego","|Complete|",""

libcxx/docs/Cxx2aStatusPaperStatus.csv

 "","","","","",""
 "`P0339R6 <https://wg21.link/P0339R6>`__","LWG","polymorphic_allocator<> as a vocabulary type","Kona","",""
 "`P0340R3 <https://wg21.link/P0340R3>`__","LWG","Making std::underlying_type SFINAE-friendly","Kona","|Complete|","9.0"
 "`P0738R2 <https://wg21.link/P0738R2>`__","LWG","I Stream, You Stream, We All Stream for istream_iterator","Kona","",""
 "`P0811R3 <https://wg21.link/P0811R3>`__","LWG","Well-behaved interpolation for numbers and pointers","Kona","|Complete|","9.0"
 "`P0920R2 <https://wg21.link/P0920R2>`__","LWG","Precalculated hash values in lookup","Kona","Reverted by `P1661 <https://wg21.link/P1661>`__",""
 "`P1001R2 <https://wg21.link/P1001R2>`__","LWG","Target Vectorization Policies from Parallelism V2 TS to C++20","Kona","",""
 "`P1024R3 <https://wg21.link/P1024R3>`__","LWG","Usability Enhancements for std::span","Kona","|Complete|","9.0"
-"`P1164R1 <https://wg21.link/P1164R1>`__","LWG","Make create_directory() Intuitive","Kona","",""
+"`P1164R1 <https://wg21.link/P1164R1>`__","LWG","Make create_directory() Intuitive","Kona","|Complete|","12.0"
 "`P1227R2 <https://wg21.link/P1227R2>`__","LWG","Signed ssize() functions, unsigned size() functions","Kona","|Complete|","9.0"
 "`P1252R2 <https://wg21.link/P1252R2>`__","LWG","Ranges Design Cleanup","Kona","",""
 "`P1286R2 <https://wg21.link/P1286R2>`__","CWG","Contra CWG DR1778","Kona","",""
 "`P1357R1 <https://wg21.link/P1357R1>`__","LWG","Traits for [Un]bounded Arrays","Kona","|Complete|","9.0"
 "`P1458R1 <https://wg21.link/P1458R1>`__","LWG","Mandating the Standard Library: Clause 16 - Language support library","Kona","|Complete|","9.0"
 "`P1459R1 <https://wg21.link/P1459R1>`__","LWG","Mandating the Standard Library: Clause 18 - Diagnostics library","Kona","|Complete|","9.0"
 "`P1462R1 <https://wg21.link/P1462R1>`__","LWG","Mandating the Standard Library: Clause 20 - Strings library","Kona","|Complete|","9.0"
 "`P1463R1 <https://wg21.link/P1463R1>`__","LWG","Mandating the Standard Library: Clause 21 - Containers library","Kona","",""

libcxx/src/filesystem/operations.cpp

if (not status_known(parent_st))
       return err.report(m_ec);
     if (not exists(parent_st)) {
       __create_directories(parent, ec);
       if (ec && *ec) {
         return false;
       }
     }
   }
   return __create_directory(p, ec);
 }
 
 bool __create_directory(const path& p, error_code* ec) {
   ErrorHandler<bool> err("create_directory", ec, &p);
 
   if (::mkdir(p.c_str(), static_cast<int>(perms::all)) == 0)
     return true;
-  if (errno != EEXIST)
+
+  if (errno == EEXIST) {
+    error_code mec = capture_errno();
+    error_code ignored_ec;
+    const file_status st = status(p, ignored_ec);
+    if (!is_directory(st)) {
+      err.report(mec);
+    }
+  } else {
     err.report(capture_errno());
+  }
   return false;
 }
 
 bool __create_directory(path const& p, path const& attributes, error_code* ec) {
   ErrorHandler<bool> err("create_directory", ec, &p, &attributes);
 
   StatT attr_stat;
   error_code mec;
   auto st = detail::posix_stat(attributes, attr_stat, &mec);
   if (!status_known(st))
     return err.report(mec);
   if (!is_directory(st))
     return err.report(errc::not_a_directory,
                       "the specified attribute path is invalid");
 
   if (::mkdir(p.c_str(), attr_stat.st_mode) == 0)
     return true;
-  if (errno != EEXIST)
+
+  if (errno == EEXIST) {
+    error_code mec = capture_errno();
+    error_code ignored_ec;
+    const file_status st = status(p, ignored_ec);
+    if (!is_directory(st)) {
+      err.report(mec);
+    }
+  } else {
     err.report(capture_errno());
+  }
   return false;
 }
 
 void __create_directory_symlink(path const& from, path const& to,
                                 error_code* ec) {
   ErrorHandler<void> err("create_directory_symlink", ec, &from, &to);
   if (::symlink(from.c_str(), to.c_str()) != 0)
     return err.report(capture_errno());

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

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03
 
+// This test requires the dylib support introduced in D92769.
+// XFAIL: with_system_cxx_lib=macosx10.15
+
 // <filesystem>
 
 // bool create_directory(const path& p);
 // bool create_directory(const path& p, error_code& ec) noexcept;
 // bool create_directory(const path& p, const path& attr);
 // bool create_directory(const path& p, const path& attr, error_code& ec) noexcept;
 
 #include "filesystem_include.h"

 }
 
 TEST_CASE(dest_is_file)
 {
     scoped_test_env env;
     const path file = env.create_file("file", 42);
     std::error_code ec = GetTestEC();
     TEST_CHECK(fs::create_directory(file, ec) == false);
-    TEST_CHECK(!ec);
+    TEST_CHECK(ec);
+    TEST_CHECK(is_regular_file(file));
+}
+
+TEST_CASE(dest_part_is_file)
+{
+    scoped_test_env env;
+    const path file = env.create_file("file");
+    const path dir = env.make_env_path("file/dir1");
+    std::error_code ec = GetTestEC();
+    TEST_CHECK(fs::create_directory(dir, ec) == false);
+    TEST_CHECK(ec);
     TEST_CHECK(is_regular_file(file));
+    TEST_CHECK(!exists(dir));
+}
+
+TEST_CASE(dest_is_symlink_to_dir)
+{
+    scoped_test_env env;
+    const path dir = env.create_dir("dir");
+    const path sym = env.create_symlink(dir, "sym_name");
+    std::error_code ec = GetTestEC();
+    TEST_CHECK(create_directory(sym, ec) == false);
+    TEST_CHECK(!ec);
+}
+
+TEST_CASE(dest_is_symlink_to_file)
+{
+    scoped_test_env env;
+    const path file = env.create_file("file");
+    const path sym = env.create_symlink(file, "sym_name");
+    std::error_code ec = GetTestEC();
+    TEST_CHECK(create_directory(sym, ec) == false);
+    TEST_CHECK(ec);
 }
 
 TEST_SUITE_END()

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

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03
 
+// This test requires the dylib support introduced in D92769.
+// XFAIL: with_system_cxx_lib=macosx10.15
+
 // <filesystem>
 
 // bool create_directory(const path& p, const path& attr);
 // bool create_directory(const path& p, const path& attr, error_code& ec) noexcept;
 
 #include "filesystem_include.h"
 #include <type_traits>
 #include <cassert>

 
 TEST_CASE(dest_is_file)
 {
     scoped_test_env env;
     const path file = env.create_file("file", 42);
     const path attr_dir = env.create_dir("attr_dir");
     std::error_code ec = GetTestEC();
     TEST_CHECK(fs::create_directory(file, attr_dir, ec) == false);
-    TEST_CHECK(!ec);
+    TEST_CHECK(ec);
+    TEST_CHECK(is_regular_file(file));
+}
+
+TEST_CASE(dest_part_is_file)
+{
+    scoped_test_env env;
+    const path file = env.create_file("file", 42);
+    const path dir = env.make_env_path("file/dir1");
+    const path attr_dir = env.create_dir("attr_dir");
+    std::error_code ec = GetTestEC();
+    TEST_CHECK(fs::create_directory(dir, attr_dir, ec) == false);
+    TEST_CHECK(ec);
     TEST_CHECK(is_regular_file(file));
+    TEST_CHECK(!exists(dir));
 }
 
 TEST_CASE(attr_dir_is_invalid) {
   scoped_test_env env;
   const path file = env.create_file("file", 42);
   const path dest = env.make_env_path("dir");
   const path dne = env.make_env_path("dne");
   {
     std::error_code ec = GetTestEC();
     TEST_CHECK(create_directory(dest, file, ec) == false);
     TEST_CHECK(ErrorIs(ec, std::errc::not_a_directory));
   }
   TEST_REQUIRE(!exists(dest));
   {
     std::error_code ec = GetTestEC();
     TEST_CHECK(create_directory(dest, dne, ec) == false);
     TEST_CHECK(ErrorIs(ec, std::errc::not_a_directory));
   }
 }
 
-TEST_CASE(dest_is_symlink) {
+TEST_CASE(dest_is_symlink_to_unexisting) {
   scoped_test_env env;
-  const path dir = env.create_dir("dir");
+  const path attr_dir = env.create_dir("attr_dir");
   const path sym = env.create_symlink("dne_sym", "dne_sym_name");
   {
     std::error_code ec = GetTestEC();
-    TEST_CHECK(create_directory(sym, dir, ec) == false);
+    TEST_CHECK(create_directory(sym, attr_dir, ec) == false);
+    TEST_CHECK(ec);
+  }
+}
+
+TEST_CASE(dest_is_symlink_to_dir) {
+  scoped_test_env env;
+  const path dir = env.create_dir("dir");
+  const path sym = env.create_symlink(dir, "sym_name");
+  const path attr_dir = env.create_dir("attr_dir");
+  {
+    std::error_code ec = GetTestEC();
+    TEST_CHECK(create_directory(sym, attr_dir, ec) == false);
     TEST_CHECK(!ec);
   }
 }
 
+TEST_CASE(dest_is_symlink_to_file) {
+  scoped_test_env env;
+  const path file = env.create_file("file");
+  const path sym = env.create_symlink(file, "sym_name");
+  const path attr_dir = env.create_dir("attr_dir");
+  {
+    std::error_code ec = GetTestEC();
+    TEST_CHECK(create_directory(sym, attr_dir, ec) == false);
+    TEST_CHECK(ec);
+  }
+}
+
 TEST_SUITE_END()