fs.cwd().openDir().iterate() panics if called without OpenDirOptions.iterate = true on Linux (Ubuntu), works fine on MacOS

[renatoathaydes]

Zig Version

0.10.0-dev.2577+5816d3eae

Steps to Reproduce

Call this:

var children = try std.fs.cwd().openDir("some-dir", .{});
defer children.close();
_ = children.iterate();

Expected Behavior

The call should return an error as the default is iterate=false.

On MacOS, no error happens at all, which is probably also a bug?

Actual Behavior

thread 1827 panic: reached unreachable code
/opt/hostedtoolcache/zig/zig-linux-x86_64-0.9.1/x64/lib/std/os.zig:4342:18: 0x2a5567 in std.os.lseek_SET (build)
        .BADF => unreachable, // always a race condition
                 ^
/opt/hostedtoolcache/zig/zig-linux-x86_64-0.9.1/x64/lib/std/fs.zig:597:45: 0x29c530 in std.fs.Iterator.next (build)
                            std.os.lseek_SET(self.dir.fd, 0) catch unreachable; // EBADF here likely means that the Dir was not opened with iteration permissions
                                            ^
/home/runner/work/zig-common-tasks/zig-common-tasks/build.zig:17:34: 0x29e288 in [email protected] (build)
    var child = try iterator.next();
                                 ^
/home/runner/work/zig-common-tasks/zig-common-tasks/build.zig:6:19: 0x2920c6 in [email protected] (build)
    testAllSamples(b, test_step) catch |e| {
                  ^
/opt/hostedtoolcache/zig/zig-linux-x86_64-0.9.1/x64/lib/std/special/build_runner.zig:220:28: 0x2838e4 in runBuild (build)
        .Void => root.build(builder),
                           ^
/opt/hostedtoolcache/zig/zig-linux-x86_64-0.9.1/x64/lib/std/special/build_runner.zig:202:17: 0x27855f in main (build)
    try runBuild(builder);
                ^
/opt/hostedtoolcache/zig/zig-linux-x86_64-0.9.1/x64/lib/std/start.zig:561:37: 0x26ff1a in std.start.callMain (build)
            const result = root.main() catch |err| {
                                    ^
/opt/hostedtoolcache/zig/zig-linux-x86_64-0.9.1/x64/lib/std/start.zig:495:12: 0x250b2e in std.start.callMainWithArgs (build)
    return @call(.{ .modifier = .always_inline }, callMain, .{});
           ^
/opt/hostedtoolcache/zig/zig-linux-x86_64-0.9.1/x64/lib/std/start.zig:409:17: 0x24fbc6 in std.start.posixCallMainAndExit (build)
    std.os.exit(@call(.{ .modifier = .always_inline }, callMainWithArgs, .{ argc, argv, envp }));
                ^
/opt/hostedtoolcache/zig/zig-linux-x86_64-0.9.1/x64/lib/std/start.zig:322:5: 0x24f9d2 in std.start._start (build)
    @call(.{ .modifier = .never_inline }, posixCallMainAndExit, .{});
    ^
error: the following build command crashed:
/home/runner/work/zig-common-tasks/zig-common-tasks/zig-cache/o/68e59b58dd56ed02a9db570b274bcbd5/build /opt/hostedtoolcache/zig/zig-linux-x86_64-0.9.1/x64/zig /home/runner/work/zig-common-tasks/zig-common-tasks /home/runner/work/zig-common-tasks/zig-common-tasks/zig-cache /home/runner/.cache/zig test

[renatoathaydes]

Currently, iterate cannot error, I realize... but I think it should? Or maybe it should not even be present when the iterate=true option is not given (assuming the options can be comptime)?

Anyway, I was surprised the call seems to work on MacOS anyway, but panics on Ubuntu... I can check Windows too if needed.

[ominitay]

This is expected behaviour, is intended, and is documented.

[renatoathaydes]

This is not documented in either openDir() or iterate(). Please show me where this is mentioned.

Also, is this a good idea you think?

[renatoathaydes]

Is this what you say documents this behaviour?

pub const OpenDirOptions = struct {
...
        /// `true` means the opened directory can be scanned for the files and sub-directories
        /// of the result. It means the `iterate` function can be called.
        iterate: bool = false,

I really disagree if so. Panics shouldn't be treated so lightly like that. Are we trying to write a language that is robust or what?

[ominitay]

This is not documented in either openDir() or iterate(). Please show me where this is mentioned.

It is stated here.

zig/lib/std/fs.zig

Lines 1486 to 1488 in 93ac87c

	/// `true` means the opened directory can be scanned for the files and sub-directories
	/// of the result. It means the `iterate` function can be called.
	iterate: bool = false,

Also, is this a good idea you think?

Yes, the status quo is correct.

I don't know why you're reacting with a 👎 to me stating the facts lol.

I really disagree if so. Panics shouldn't be treated so lightly like that. Are we trying to write a language that is robust or what?

You wrote incorrect code; I don't know what you expect Zig to do for you here...

[Deecellar]

Is this what you say documents this behaviour?

pub const OpenDirOptions = struct {
...
        /// `true` means the opened directory can be scanned for the files and sub-directories
        /// of the result. It means the `iterate` function can be called.
        iterate: bool = false,

I really disagree if so. Panics shouldn't be treated so lightly like that. Are we trying to write a language that is robust or what?

The language panics because you wrote wrong code, I think is fair...
Tho, I will agree we could do better messaging, I will not call this a bug (on the panic side) but a bug it works on Mac.

Please calm down on the panics, they are expected side of the language, the panics should probably guide more, and I think is better than a silent error.

[ominitay]

You could perhaps propose (or make a pull request) to add an explicit comment to iterate and walk that OpenDirOptions.iterate must be true?

[SpexGuy]

@renatoathaydes Thanks for opening this issue! You are right that there's an inconsistency here that should be fixed.
Zig generally marks errors that could be considered "programmer error" with unreachable/panics. This is one of those cases -- attempting to iterate over a directory object that was opened without iterate permissions. So a panic in this case is consistent with the design of the rest of the std lib. However, the specific panic which occurs has very little to do with the problem that caused it, which needs improvement. Additionally, std.fs is intended to provide a platform agnostic wrapper around os functions, so operations like this should succeed on all supported targets or fail on all supported targets. The specific actionables going forward for this issue are

• Give a better panic message in debug modes when iterating over a directory that was opened without iterate permissions
• Document the need for setting .iterate=true on functions which perform iteration on a directory that the user opened
• Add checks to cause a panic for this case in debug modes on MacOS targets

[ominitay]

Give a better panic message in debug modes when iterating over a directory that was opened without iterate permissions

// EBADF here likely means that the Dir was not opened with iteration permissions is literally in the provided error trace. What more could we do here? Are you suggesting a runtime check?

Add checks to cause a panic for this case in debug modes on MacOS targets

Same here -- this would require adding a check with a runtime overhead.

Document the need for setting .iterate=true on functions which perform iteration on a directory that the user opened

I'll get to doing this if nobody else does before me.

[SpexGuy]

What more could we do here? Are you suggesting a runtime check?

I am. I think this error is common enough that it warrants a runtime check in debug modes.

[ominitay]

I don't see this error so often, and the obvious comment in the error trace which points people who read it to where they need to go seems to be sufficient to me.

I'd prefer a comptime check if anything, but that's clearly not possible/feasible. Though I'm sure I'm missing something here, so will leave you to it lol.

It was suggested by @kubkon in Discord that not enough newbies understand that a common pattern in Zig is that incorrect code will cause panics or reach unreachable statements. Perhaps it is as or more important that this is better portrayed in Zig docs.

[renatoathaydes]

@ominitay It's not sufficient to me. By a large margin. Clearly, if it's sufficient to you, we have very different criteria to decide when documentation is sufficient to warn against a panic.

I don't know why you're reacting with a 👎 to me stating the facts lol.

Because you treat someone who takes the time to report an error and try the code in different operating systems with disdain as if we are idiots, and you know it all and have the power to decide when there's enough documentation, and others shouldn't have an opinion that contradicts yours otherwise you just "lol".

@SpexGuy thanks for the welcoming treatment. That's what I would expect from a community where people want to help each other instead of demanding full understanding of every option when calling a function.

Don't you think that iterate should be true by default? Iterating the directory is the main use case I can think of for opening a directory, isn't it? The main use case, in my opinion, should always be the default.

[nektro]

Iterating the directory is the main use case I can think of for opening a directory, isn't it?

maybe in some cases like a file browser, but generally speaking, not by a long shot. typically you know the exact pattern of file names you're going to access, which is not inhibited by .iterate being false.

[ominitay]

It's not sufficient to me. By a large margin.

I agree with you on this. That you didn't know about this behaviour demonstrates that the documentation isn't clear enough about this behaviour.

we have very different criteria to decide when documentation is sufficient to warn against a panic.

This will panic, that is the intended behaviour (as it is a programming mistake, not a runtime failure), and will not change. (Debug mode) panics are sufficient here, as the compiler can optimise better in release mode, and they allow devs to debug their code in debug mode.

Because you treat someone who takes the time to report an error and try the code in different operating systems with disdain as if we are idiots, and you know it all and have the power to decide when there's enough documentation, and others shouldn't have an opinion that contradicts yours otherwise you just "lol".

Please refrain from using personal attacks and making accusations against others. It doesn't help us get anywhere, and serves only to derail the discussion. I didn't treat you as or think of you as an idiot.

I also didn't say that there is enough documentation. You can see that for yourself in this post and my previous one above :)

Don't you think that iterate should be true by default? Iterating the directory is the main use case I can think of for opening a directory, isn't it? The main use case, in my opinion, should always be the default.

No, iterate should not be set to true by default, as it requires more permissions, and is unnecessary for all other operations using a directory. If it didn't require more permissions, then I'd agree with you here, and argue for it to be removed entirely.

I don't want any negativity between us, sorry if my message came across in a different tone than I intended.

[InKryption]

Just to throw this out there, another solution would be to make a separate type "IterableDir", which is simply wraps over Dir, but is always opened as iterable; then, you remove anything to do with iteration from Dir, and guide anyone who wants to iterate over a directory's contents to something like openIterableDir/take an IterableDir as a function parameter.

This would document, in code, at the type level, that there is a decision to be made about the explicitly desired capabilities of a directory handle.

Though, personally, I think a debug-only runtime check would be sufficient. More front-and-center documentation would be even better; perhaps on any relevant functions, as I'm not sure how many people really go out of their way to read the field comments of struct parameters.

[ominitay]

Just to throw this out there, another solution would be to make a separate type "IterableDir", which is simply wraps over Dir, but is always opened as iterable; then, you remove anything to do with iteration from Dir, and guide anyone who wants to iterate over a directory's contents to something like openIterableDir/take an IterableDir as a function parameter.

This would document, in code, at the type level, that there is a decision to be made about the explicitly desired capabilities of a directory handle.

This sounds interesting, although I question the practicality of it, in comparison with a simple debug runtime check (which I now feel I agree with having).

Though, personally, I think a debug-only runtime check would be sufficient. More front-and-center documentation would be even better; perhaps on any relevant functions, as I'm not sure how many people really go out of their way to read the field comments of struct parameters.

Better documentation is really important here. This reminds me kind of about HashMap, where a function didn't directly mention that it invalidated references, even though it was documented.

[InKryption]

Speaking on behalf of @ominitay, just wanted to mention that after discussing it over discord, they've come to the conclusion that IterableDir, or similar, would be the way to go, as it reflects intent more clearly, and loosely follows a precedent set by std.io.SeekableStream; that is, the ability to do X - like seeking, or iterating in this case - being tied to a separate interface type implemented on top of the basic interface.

[renatoathaydes]

That makes sense. An API that takes an options object but returns the same type regardless of the options, but which only allows some of the operations on the returned type to be used is simply under-using the power of types... in other words, it's leaning in the direction of dynamic typing almost... so having a more specific function that returns a more specific type that describes precisely the operations that can be performed is absolutely better.