File::Mode flag enum

[straight-shoota]

#8011 proposes a File::Mode flag enum as an alternative and eventual replacement for the string-based file mode configuration which is inherited from fopen.

I think this is a good change and it has received positive feedback. But some of the API details may need some ironing out, and I'd rather do that in a dedicated issue than the PR itself, to focus on the implementation there.

The proposed File::Mode looks good. Could use some documentation, but the API should be fine.

For File.new and .open, I'm not very happy with mode being a splat parameter. That's essentially a hack to allow easy combination of short names for the flag enum members. :create | :truncate doesn't work.
Maybe that's a good solution, but I'm not convinced about that. I would suggest to exclude the splat parameter for now. It can be just mode : Mode for now. At call site it's possible to use File::Mode.flags(Create, Truncate) or File::Mode::Create | File::Mode::Truncate. The splat parameter is a convenience feature and not required to make the API work. We can still add it later, but I'd like to think a bit more about this. I'd suggest looking into alternatives, such as language support for flag enum construction at autocast (#10680). If it works, it would be a superior and generic solution of the original problem, and remove the need for a hack.

The param name should be singular to make sense as a named argument: File.new(path, mode: :read).

[Blacksmoke16]

related: #10680

[didactic-drunk]

I'd suggest looking into alternatives, such as language support for flag enum construction at autocast. If it works, it would be a superior and generic solution of the original problem, and remove the need for a hack.

How does that work and when?

[HertzDevil]

If certain combinations of flags are relatively common then they should be defined as constants somewhere. | can be used on unqualified names inside File::Mode, but this might affect flag iteration. Outside that scope (e.g. File::DEFAULT_NEW_FILE_MODE) it matters less which syntactic form is used at the constant initializer.

To be honest I find File::Mode::Create | File::Mode::Truncate perfectly acceptable. Perhaps this is because a lot of LibC constants that should have been enums are unscoped, i.e. they were #defines in the original C sources?

Don't know if anyone mentioned this, but since these flags are not rooted in POSIX we could skip the binary / text mode flags entirely. However, we should still merge #11824 until the string modes are deprecated and removed.

[straight-shoota]

since these flags are not rooted in POSIX we could skip the binary / text mode flags entirely.

Good point. They're already not represented in File::Mode.

[didactic-drunk]

@HertzDevil Common combined modes are provided with input from a variety of other languages and discussion in #7857
Examples: (CAPS = POSIX_MODE, Ucfirst = Crystal mode)

• Create = CREAT | Write
• CreateNew = CREAT | EXCL | Write

Some possible additions:

• Overwrite = Create | Truncate (provides "w" equivalent)
• ReadWrite = Read | Write (I consider this a marginal benefit as Create is often necessary which implies Write)
• Log or LogFile = Create | Append

I provided a non exhaustive list of possible file modes without platform specifics in here. Summary: there are tons of variations between open modes. Those listed above and in the PR probably provide 95% use cases with 1 or sometimes 2 flags.

Also note the current combined modes make some open() calls impossible. Block/device files, lock files w/o w perm and a few corner cases can't be handled without passing POSIX modes.

All platform specific options TBD in future PR's.

[yxhuvud]

One option that was not mentioned in #7857 was O_CLOEXEC and O_NONBLOCK. Both are sometimes set today using fcntl. Could be worth supporting directly to avoid extra syscalls (for creating sockets, for example).

[didactic-drunk]

For files CLOEXEC/NOINHERIT are always set as open flags, not fcntl. NONBLOCK unfortunately doesn't work for files.

[yxhuvud]

Well, that makes it even more obvious of a question: Would this enum be created only for external APIs or would it be used internally as well? If the latter, then CLOEXEC should be part of it, no?

Sockets are also files. Different types of files have different flags that are relevant.

[didactic-drunk]

This issue is Crystal portable public File only API.

The current flags can't apply to sockets:

• Create, CreateNew
• Read, Write
• Truncate
• NoSymFollow

In a future PR, flags supported on most platforms that can gracefully degrade if not supported such as:

• NoAtime
• Fsync, Dsync
• Direct
• Sequential

[didactic-drunk]

Bump?

[straight-shoota]

Sockets are also files.

That may apply to the underlying OS implementation. But it's not represented in Crystal's stdlib APIs. File and Socket are distinct types and there should be distinct APIs for flag modes.

[straight-shoota]

In order to move this forward, I'd suggest to start with a new PR. It should be as minimal as possible and avoid any additional changes.¹

It should focus on only adding File::Mode enum and overloads to file methods for accepting this type as an alternative to mode strings. The splat argument for mode should be dropped.
Adapting use of mode strings to the new enum in stdlib and compiler is good. We should promote that API.
But let's not deprecate any mode string overloads just now. It might be useful to keep them around because it's a very common way to express file open modes. So we should follow up with a discussion whether they should be removed or not.

@didactic-drunk Do you want to continue with this?

Footnotes

1. Such as removing the default argument value in calls (https://github.com/crystal-lang/crystal/pull/8011/files#diff-f00efc8cfef4f5f2a97d2d4465a70d7f813199adc7efe8856d76a79387241c3c) or refactoring calls from File.open to File.new (https://github.com/crystal-lang/crystal/pull/8011/files#diff-f88e4524ddffb38d230c4720a9a414614b52e2755c544f8c4a88db9d3b6c5348). Those changes may be good, but they can and should be applied independently. ↩

[didactic-drunk]

@didactic-drunk Do you want to continue with this?

Yes, but my bandwidth is low. Can't do immediately