std.fs: split Dir into IterableDir

[Vexu]

walk and iterate are moved to a new type IterableDir. Dir receives a new openIterableDir function which returns an IterableDir and also an intoIterable function which converts the Dir into an IterableDir asserting that it was opened with iterate = true.

Closes #12007

[andrewrk]

It seems like the main benefit of this change is so that calling intoIterable gives an assert in safe modes, so that when testing on an operating systems which allow all dirs to iterate, the programmer will not incorrectly believe that it works when it will fail on a different operating system that needs the iterable flag to be correct.

This benefit could be achieved more simply by adding the safety flag to Dir instead of introducing IterableDir as a new type.

I'm a bit concerned here with the added clunkiness of the API; having two types associated with the same resource. It does prevent a certain kind of mistake from being made, but it also causes confusion about which type has which methods and makes usage sites more convoluted.

We had a similar pattern earlier with mutexes. It used to be:

var held = mutex.acquire();
defer held.release();

...which was a nice way to help the user not forget to call unlock(). However it ended up being really annoying and actually more error prone any time the API needed to be wrapped, and we ended up just going back to the more straightforward:

mutex.lock();
defer mutex.unlock();

How do you feel about my counter proposal of adding the safety field directly to Dir?

[andrewrk]

Suggested change

	if (builtin.mode == .Debug) {
	if (std.debug.runtime_safety) {

[andrewrk]

Suggested change

	const iterable_safety = if (builtin.mode == .Debug) false else {};
	const iterable_safety = if (std.debug.runtime_safety) false else {};

[Vexu]

It seems like the main benefit of this change is so that calling intoIterable gives an assert in safe modes, so that when testing on an operating systems which allow all dirs to iterate, the programmer will not incorrectly believe that it works when it will fail on a different operating system that needs the iterable flag to be correct.

This benefit could be achieved more simply by adding the safety flag to Dir instead of introducing IterableDir as a new type.

I don't think the main benefit here is intoIterable giving an assertion. I think the main benefit is that attempting to iterate a regular Dir will give a compile error and force the programmer to think about this issue before they even have a chance to test their program.

I'm a bit concerned here with the added clunkiness of the API; having two types associated with the same resource. It does prevent a certain kind of mistake from being made, but it also causes confusion about which type has which methods and makes usage sites more convoluted.

I think the separation between the types is quite clear here, IterableDir only allows iteration and walking while Dir only allows all other directory operations. dir.intoIterable and iterable_dir.dir also makes it easy to go between the types depending on which kind of operations you need to do.

We had a similar pattern earlier with mutexes. It used to be:

var held = mutex.acquire();
defer held.release();

...which was a nice way to help the user not forget to call unlock(). However it ended up being really annoying and actually more error prone any time the API needed to be wrapped, and we ended up just going back to the more straightforward:

mutex.lock();
defer mutex.unlock();

I don't think this situation is comparable. Mutex.Held used to encode state of the resource while IterableDir encodes available operations. Whether or not a directory supports iteration is decided on creation and you cannot change that state without reopening the directory.

[ominitay]

Just rounding up comments made about this on Discord:

OpenDirFlags.iterate should be removed, if we are to add the type-based system; there isn't a need to have two methods. The only difference between them is that one is a runtime crash and one is a compile error. TmpDir should be split into TmpDir and TmpIterableDir to correspond with this.

[Vexu]

OpenDirFlags.iterate should be removed, if we are to add the type-based system; there isn't a need to have two methods. The only difference between them is that one is a runtime crash and one is a compile error. TmpDir should be split into TmpDir and TmpIterableDir to correspond with this.

Removing the ability to easily go between the different types causes a lot of unnecessary repetition or loss of type information via the use of anytype for what seems to be like no benefit.

[ominitay]

You've said this a number of times, but could you please provide an actual example of this?

There is no case in which going from Dir to IterableDir is necessary, as the types exist specifically to indicate the permissions. If a directory has been opened as iterable, one will have IterableDir. It is easy to go from an IterableDir to a Dir, which is the only operation which seems necessary...

If a function needs to iterate on a directory, it should take IterableDir. Else, it should take Dir. This should be enforced by the API, imo.

Have you thought this through? anytype wouldn't work with an interface like this anyway...

[Vexu]

There is no case in which going from Dir to IterableDir is necessary, as the types exist specifically to indicate the permissions. If a directory has been opened as iterable, one will have IterableDir. It is easy to go from an IterableDir to a Dir, which is the only operation which seems necessary...

This is not the point you were making earlier or at least you couldn't be bothered to make it clear. This I could agree on.

Have you thought this through? anytype wouldn't work with an interface like this anyway...

That is exactly how anytype is being used, just take a look at the thousands of instances of writer: anytype out there.

[InKryption]

@Vexu Would you maybe consider omitting the components of this PR that would allow for conversion from Dir to IterableDir? At the very least, as an experiment to see if any of the problems you're alluding to actually emerge. They can always be added later on (wouldn't be as easy to remove them if present, being that it would break any code relying on it).

Otherwise, I think it'll turn out to be far more harmful for people to unwittingly take a Dir parameter/use a Dir field, only to then use intoIterable, when it'd have made far more sense to just have an IterableDir in the first place (an incorrect path of least resistance).

[ominitay]

This is not the point you were making earlier or at least you couldn't be bothered to make it clear. This I could agree on.

Couldn't be bothered?? 🙄 This is my entire criticism I've been making of your PR: that intoIterable() (and therefore OpenDirFlags.iterate) should be removed, as they serve no purpose.

One of my concerns is that, for example, someone has an already-written function which takes a Dir. They add in the requirement to iterate on the directory which they take as an argument after the fact. The presence of intoIterable() provides low resistance, and is likely to be the change they make, despite the 'correct' way to be to take an IterableDir as an argument.

That is exactly how anytype is being used, just take a look at the thousands of instances of writer: anytype out there.

But you can only call iterate() or walk() on an IterableDir. It's nothing like a Writer, as writers all have a common set of functions. The entire point of IterableDir is that it doesn't.

[Vexu]

Couldn't be bothered?? roll_eyes This is my entire criticism I've been making of your PR: that intoIterable() (and therefore OpenDirFlags.iterate) should be removed, as they serve no purpose.

One of my concerns is that, for example, someone has an already-written function which takes a Dir. They add in the requirement to iterate on the directory which they take as an argument after the fact. The presence of intoIterable() provides low resistance, and is likely to be the change they make, despite the 'correct' way to be to take an IterableDir as an argument.

And yet, I though you wanted to remove the conversion in both directions which is why I was emphasizing how it would lead to repetition of code.

But you can only call iterate() or walk() on an IterableDir. It's nothing like a Writer, as writers all have a common set of functions. The entire point of IterableDir is that it doesn't.

Not directly, but all it takes is a little comptime logic:

fn iterableOrNotIDontCare(dir: anytype) void {
    // non iterating operations on `dir`..
    if (some_comptime_condition) iterableOnly(dir);
    // more non iterating operations on `dir`..
}

(This of course is only when IterableDir has all the functions in Dir which I now understand is not what you wanted)

[ominitay]

And yet, I though you wanted to remove the conversion in both directions which is why I was emphasizing how it would lead to repetition of code.

Oh I see why you could have thought that lol. Glad we're on the same page now, then!

[andrewrk]

A similar pattern can be drawn with passing OpenMode.read_only when opening a file. You'll get an error from the operating system if you try any of the write API. And yet I think having WritableFile and ReadableFile would make the API worse. I do prefer status quo, both with File and with Dir. However, I'm willing to try @Vexu's new API for a while and see how it feels.

[ominitay]

I do prefer status quo, both with File and with Dir.

I do also, but if we're to add a check for iterability, I think it's probably best to try a comptime, type-based solution first.

[marler8997]

For a real-world example, here's how this change affects zigup:

marler8997/zigup@c3b4d8b

As I was moving to the new API, I forgot to change one of my opens to openIterableDirAbsolute. Rather than a runtime error, the compiler caught it for me, which was nice. It gave me confidence that once I did compile, I must have changed all of them. On the other hand I recognize this does add complexity to the API.

[ominitay]

I've seen mixed feedback on this change. There are people who are very unhappy with it, proclaiming it "pointless" and a terrible API, and then you with the benefit that you know your code will be correct when it compiles. I'm not sure where we should go with this, whether we should revert and use a runtime check or stick with it. Maybe worth giving it more time?

[kubkon]

I am on the fence with this change. If you look at the updated fetch-them-macos-headers ziglang/fetch-them-macos-headers@96b1195, the required changes look and feel rather clunky, not to mention confusing: having to call a .dir on an iterable dir feels overly complicated. Perhaps wrapping it in a function would better clarify the intent. tmpIterableDir is suffering from the same problem now that we have to "dereference" it twice: tmp.iterable_dir.dir. I think this really should always be a single call such as tmp.getDir() or something. I am also worried that should we decide for another level of abstraction where we would wrap IterableDir in some YetAnotherDir, then the chain of wrapped .dir fields becomes ridiculous: some.dir.dir.dir and so on.

[Jarred-Sumner]

This change makes using std.fs more tedious

If Zig had a way to do interfaces/"inheritance"/traits/etc without adding an extra layer of nesting (instead of .dir, just make all the functions on Dir work with an IterableDir) then it would be fine

[andrewrk]

After running with this change for about a year now, I have to say that I strongly don't like it and I would like to revert it.

In fact, I'm sorry @Vexu but I am very confident in this decision and will use my BDFL veto powers to make the call here. It's going to be reverted.