File/Dir deinit invalidates, close doesn't

[marler8997]

Andrew has stated that Zig requires a way to close and invalidate a File. These are my proposed changes to address that without requiring all File instances to be mutable. This PR provides that by adding the "deinit" function to File which will call "close" and then invalidate. At the same time, I've modified Dir to use the same pattern, where deinit will close and invalidate whereas close will only release the OS resource. Because Dir did not do this before, zig code was unable to use "const dir" but can now do so with this change. I've gone through std looking for ".close(" calls and modified the respective Dir or File handles to be const when possible. I also found a few places where tests were using mutable files, in those cases I modified them to call deinit to invalidate the file. To me this makes the code more readable because using const reduces the mental load when reading code. When I see var dir or var file my brain makes a mental note that the code can modify the file handle, but with const I know it won't.

[SpexGuy]

I think in this case close() should invalidate. deinit comes as a pair to init, but there is no init function for files/dirs. Instead, the documentation of close says

/// Upon success, the stream is in an uninitialized state. To continue using it,
/// you must use the open() function.

So if anything should invalidate, it's close. There's no need for a new deinit function here.

[marler8997]

There's no need for a new deinit function here.

Are you saying there's no need for the ability to close a file/dir without invalidation, or are you just concerned about the names of the functions and misleading comment? My proposition is that allowing file/dir instances to be const is the reason for supporting close without validation, so there is a reason, but I can understand if you think that is not a good enough reason? Is that the case or are you just concerned about the function names/comment?

[SpexGuy]

My proposition is that allowing file/dir instances to be const is the reason for supporting close without validation, so there is a reason, but I can understand if you think that is not a good enough reason? Is that the case or are you just concerned about the function names/comment?

I'm saying I don't think it's worth having both functions. Having two functions that release resources and leave the object in an unusable state is unnecessary complexity. Personally I also don't think it's worth preserving allowing File and Dir to be used as const, since they are mired in syscalls and optimization is really not an issue here. But there's no reason to add deinit, we should only change close to the desired behavior. If the desired behavior is to allow deinit of const, no changes are needed.

[marler8997]

@SpexGuy the purpose of const is not for optimization, it's for "code readability" in my proposition. Just to be clear, you're saying you don't think the code readability that const provides justifies the added complexity of 2 ways to cleanup a file?

[SpexGuy]

Just to be clear, you're saying you don't think the code readability that const provides justifies the added complexity of 2 ways to cleanup a file?

Yeah, I agree with that. Maybe with better names, like close and closeConst, this would be more palatable. But the obvious match to open is close, so to a user it's really non-obvious when deinit should be called. A reasonable looking approach would be to call both close and deinit, in that order! The ergonomics are also important -- if we have two, deinit should be the preferred, "default" one, because it is safer. closeConst should be the second approach, used in cases where deinit doesn't work.

The readability argument for File and Dir is a good one, and it makes me think that the one existing close function is correct, and no changes are needed. I don't think the safety of invalidation is worth the complexity of adding another function.

[marler8997]

@SpexGuy I'm perfectly happy with close vs closeConst and I agree with your reasoning as to why that's preferable to deinit vs close. The only reason I wouldn't start with something like that is it's a pretty big breaking change, so we would need justification for it before making it. Since you're not sure this added complexity is worth it I won't make the change to use close and closeConst unless Andrew indicates this is what we want. I'll leave this open for any further discussion but Andrew feel free to close this if you're sure it's not what we want.

I'm also working on the opposite of this proposal, making close invalidate so we can see what that looks like, PR incoming.

[SpexGuy]

Since you're not sure this added complexity is worth it I won't make the change to use close and closeConst unless Andrew indicates this is what we want.

Sounds good, thanks!

it's a pretty big breaking change, so we would need justification for it before making it.

I don't necessarily agree with this. Zig is openly not stable yet, especially the std lib. We should schedule breaking changes carefully with the release schedule in mind, but other than that I don't think breaking changes need special consideration.

[andrewrk]

Closing in favor of #10214

[marler8997]

@andrewrk do you mind sharing your thoughts on why you prefer #10214? I've shared some reasons on IRC/Discord why immutable file/directory handles are simpler to work with/understand. Do you not think immutable handles are simpler? Or maybe you agree they are simpler, just not enough to justify the addition of a closeConst that doesn't invalidate alongside close that does? Forgive me for being a little slow to understand your reasoning sometimes, I'm not one to "go with the flow" to avoid confrontation, I tend to continue asking questions and pressing until I at least understand the other persons viewpoint. I'm willing to accept differences of opinion but my goal is to understand those opinions enough to find them reasonable. Any insight here would be helpful to me, thanks.

P.S. Also if we are giving up immutability for the sake of invalidation, should be be doing this for everything? For example, should allocator.reallocFn take the slice by mutable reference so it can be invalidated?

var slice = allocator.alloc(u8, 100);
defer allocator.free(&slice);