<filesystem>: `directory_entry(const path& p, error_code& ec)` does not return error code

[e4lam]

STL/stl/inc/filesystem

Lines 2106 to 2109 in d736fe4

	directory_entry(const filesystem::path& _Path_arg, error_code& _Ec) : _Cached_data{}, _Path(_Path_arg) {
	_Ec.clear();
	(void) _Refresh(_Cached_data, _Path);
	}

Notice the _Ec is cleared but never assigned the return value of _Refresh(). Compare this with:

STL/stl/inc/filesystem

Lines 2147 to 2149 in d736fe4

	void refresh(error_code& _Ec) noexcept {
	_Ec = _Make_ec(_Refresh(_Cached_data, _Path));
	}

This is at best inconsistent with calling std::directory_entry::refresh( std::error_code& ec) but I would argue that it's the constructor that's wrong.

This can be easily tested by giving these two methods the same non-existent file path. The expected behaviour is that both of these methods return the same error_code value. i.e. NOT having the constructor form return success for an error code, but then calling refresh() returns a failure error code.

Sample test code

// compile with: cl /std:c++17 /permissive- /EHsc /Od /Zi symlink.cpp
// run: .\symlink does_not_exist
//
#include <filesystem>
#include <iostream>
namespace fs = std::filesystem;

int main(int argc, const char* argv[])
{
  if (argc < 2)
  {
    std::cerr << "Missing file path argument\n";
    return -1;
  }
  fs::path dst(argv[1]);

  std::error_code err1;
  fs::directory_entry src(dst, err1);
  std::cout << "directory_entry(" << dst << ") = " << err1 << " " << err1.message() << "\n";

  std::error_code err2;
  src.refresh(err2);
  std::cout << "refresh(" << dst << ") = " << err2 << " " << err2.message() << "\n";

  return 0;
}

Also tracked by Microsoft-internal VSO-1012326 / AB#1012326.

[StephanTLavavej]

I read WG21-N4835 [fs.dir.entry.cons]/1 "Effects: Constructs an object of type directory_entry, then refresh() or refresh(ec), respectively." as saying that this is clearly a bug.

[e4lam]

A possibly separate issue is whether calling these with a non-existent path should ever return a failing error_code failure. There is also the directory_entry::exists() method that is supposed to be able to return true/false.

[StephanTLavavej]

Please file a separate issue if you believe that’s another bug.

[Arzaghi]

I read WG21-N4835 [fs.dir.entry.cons]/1 "Effects: Constructs an object of type directory_entry, then refresh() or refresh(ec), respectively." as saying that this is clearly a bug.

I'm investigating and trying to solve the issue.
However, I'm wondering calling refresh() means most of the member functions mentioned under [fs.dir.entry.cons] and [fs.dir.entry.mods] that, having similar Effects, should throw on errors.
However, not only are there many test cases in P0218R1_filesystem that don't except throwing, other implementations do not throw on bad inputs!
Am I right or I did miss something here?

[StephanTLavavej]

(Apologies for the delayed reply, I have been unable to keep up with my notifications)

I observe that directly calling src.refresh(); in your example still doesn't emit an exception - I am unsure exactly what situation is needed to trigger an exception. It does seem that many member functions should be calling refresh() and I'll add a comment in my review of your PR.

[BillyONeal]

I observe that directly calling src.refresh(); in your example still doesn't emit an exception - I am unsure exactly what situation is needed to trigger an exception. It does seem that many member functions should be calling refresh() and I'll add a comment in my review of your PR.

That is not permitted. The data can be cached or not, and the only operation allowed to affect cache status is a call to refresh on the part of the user. See http://eel.is/c++draft/fs.class.directory.entry

[BillyONeal]

This behavior is by design; we changed directory_iterator's constructors to always be non-failing. Atttempting to access any of the actual data when the directory_entry has been put into this state will try to access the data then.

Note that for portable behavior, callers must check for errors in those places too, because implementations might not cache in their directory_entry.

@StephanTLavavej This behavior was changed before GitHub:

• https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems/edit/660628/ " directory_entry(nonexistent) should not fail"
• https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_git/msvc/commit/90ad42af5b9bac66319e745e1b730fc3d7030989?path=%2Fsrc%2Fvctools%2Fcrt%2Fcrtw32%2Fstdhpp%2Ffilesystem&_a=contents

[BillyONeal]

Also note that we explicitly changelogged this in https://devblogs.microsoft.com/cppblog/cpp17-20-features-and-fixes-in-vs-2019/

std::filesystem::directory_entry‘s constructor was changed to store a failed result, rather than throwing an exception, when supplied a nonexistent target path.

[e4lam]

@BillyONeal Sorry, I'm not following your comments here but I'm probably just being dense.

This behavior is by design; we changed directory_iterator's constructors to always be non-failing. Atttempting to access any of the actual data when the directory_entry has been put into this state will try to access the data then.

Note that for portable behavior, callers must check for errors in those places too, because implementations might not cache in their directory_entry.

How does directory_iterator specifically relate to directory_entry(const filesystem::path& p, error_code& ec) with regards to this bug? Probably because I don't understand this part, the next "portable behaviour" comment does not compute. If one is calling a function that returns an error code, then of course it is the callers responsibility to check it.

Also note that we explicitly changelogged this in https://devblogs.microsoft.com/cppblog/cpp17-20-features-and-fixes-in-vs-2019/

std::filesystem::directory_entry‘s constructor was changed to store a failed result, rather than throwing an exception, when supplied a nonexistent target path.

When you say "store a failed result", where is it being stored? I would expect it to be (at least) stored in the ec out parameter of directory_entry(const filesystem::path& p, error_code& ec).

[BillyONeal]

@BillyONeal Sorry, I'm not following your comments here but I'm probably just being dense.

I'm probably just not explaining well :)

This behavior is by design; we changed directory_iterator's constructors to always be non-failing. Atttempting to access any of the actual data when the directory_entry has been put into this state will try to access the data then.
Note that for portable behavior, callers must check for errors in those places too, because implementations might not cache in their directory_entry.

How does directory_iterator specifically relate to directory_entry(const filesystem::path& p, error_code& ec) with regards to this bug?

directory_iterator generally fills in the cached contents itself, it never calls this constructor.

STL/stl/inc/filesystem

Line 2554 in bd7adb4

_Impl._Refresh(_Data); // can throw

The whole reason that directory_entry caches anything (and why the standard was changed to permit such caching) is that the underlying enumeration API on Windows, FindFirstFileW/FindNextFileW/FindClose, returns a bunch of this information as part of the enumeration process, and we wanted to avoid performance penalties of reopening every file again to get the information. This was http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0317r1.html . In that sense, it's related to directory_iterator because that's why the behavior exists.

Back when we initially implemented filesystem, we aggressively tried to cache information when a directory_entry was constructed, to make it indistinguishable from one generated by directory_iterator. Unfortunately, this caused problems for people upgrading experimental filesystem (N4100) code, where a common pattern was something like:

for (directory_entry&& e : directory_iterator("something")) {
    auto source = e;
    e.replace_filename("something-nonexistent"); // always safe in N4100, throws in initial MSVC filesystem implementation
    copy_file(source, e); // note implicit conversion to path
}

... which threw an exception in the new implementation because the result after replace_filename was nonexistent. After LEWG indicated that they did not like this outcome, meaning this "directory_iterator is not bound to an actual file" state could appear as part of normal operation, we changed the constructors to be non-failing in the same way replace_filename was changed to be non-failing. Eric Fiselier (of libc++ at the time) was supposed to write the paper to codify this change into the standard, but we implemented it early because the new behavior was permitted by the old wording.

Probably because I don't understand this part, the next "portable behaviour" comment does not compute. If one is calling a function that returns an error code, then of course it is the callers responsibility to check it.

It sounds like from your report that you expected the error to reported by the constructor but not from the actual data access function (status). My point is that the standard allows implementations to make either one, or both, or neither, failing. (And in fact, on most POSIX implementations, they don't cache anything in directory_entry so the constructor is non-failing for them)

Also note that we explicitly changelogged this in https://devblogs.microsoft.com/cppblog/cpp17-20-features-and-fixes-in-vs-2019/

std::filesystem::directory_entry‘s constructor was changed to store a failed result, rather than throwing an exception, when supplied a nonexistent target path.

When you say "store a failed result", where is it being stored? I would expect it to be stored in the ec out parameter of directory_entry(const filesystem::path& p, error_code& ec).

directory_entry has a "there is nothing in the cache because last time we tried to refresh() failed" state (resulting from the aforementioned behavior of replace_filename and the constructor changing). We intentionally don't report the failure to get the information there, and instead just try again when the user asks for file_size or status or ...

[BillyONeal]

/cc @EricWF did you ever get around to looking at that paper? :)