Expand OS\ design notes

[fredemmott]

Current notes are here: https://github.com/hhvm/hsl/blob/main/src/os/README.md

When discussing subproccess in facebook/hhvm#8946 (cc @Atry), there's some related discussions that should be clarified here; separate threads below, but summarizing here for readability.

Language-based changes

OS\ makes changes to the functions when the functions are designed a certain way because of limitations of the C language. For example:

• if a C function's return value only indicates success or failure, and the user is expected to check errno on failure, the OS\ function returns void, or throws on error
• if a C function uses pointers to have multiple 'out' parameters, tuple returns are used instead

Questions

• How should a C type that is used like a shape be represented? For example, posix_spawnattr_t?
  • Should it just be a shape?
• How should a C type that is used like a vec<A|B|C> be represented? For example, posix_spawn_file_actions_t?
  • use vec<WeCreateASupertype> with vec[foo_addA(), ...]
  • use vec<WeCreateASupertype> with vec[foo_OTHERVERB_a(), ...] given that it's no longer being added to a foo_t
  • use vec[foo_t::a(), ...]
  • use vec[foo_t\a(), ...]
  • preserve the $container = foo_init(); foo_addA($a, ...); foo_addB($b, ...) C API
  • use (new container_t())->addA(...)->addB(...)
  • use container_t_init()->addA(...)->addB(...)
  • support any combination of the previous 3

Non-portable extensions

For example, there is posix_spawn_file_actions_addchdir_np in BSD libc and Glibc (expected without the NP suffix in Issue 8).

• should we directly mirror the _np extension...
  • when it's only in glibc or BSD libc, not both
  • when glibc and BSD libc have compatible extensions
  • when glibc and BSD libc have incompatible extensions
  • when neither have it, but we want it in HHVM

... or should we create _np_bsd, np_glibc, _np_hhvm suffixes for clarity?

[fredemmott]

Thread: How should a C type that is used like a shape be represented?

For example, posix_spawnattr_t?
Should it just be a shape?

[Atry]

• we should try to define optionally

Would it be benefit if we have a default encoding which is technically able to encode most (if not all) setters as the first step? Then we can work out more exceptional rules to simulate C design patterns, like type hierarchy or unordered setters. I used the term "optional" to denote these exceptional rules because they do not apply to all the use cases.

[Atry]

Suppose we are creating an assumed FFI code generator, the first step is to create general rules to technically cover as much as possible APIs, then the next step is to create detailed rules for certain patterns to make the API "nicer".
The vec-of-setters approach is more general than shape or class hierarchy, because:

1. Discarding the order of calls to setters is a special case of preserving the order.
2. The C language does not support inheritance, therefore the original C type hierarchy always applies at the function level.

[Atry]

I am not worried about type hierarchy at the moment because it is not a blocker of the subprocess API design, and it is not a priority to review existing sockaddr design.

[fredemmott]

Perhaps it would be best to reframe the PRs as "here's a few ways to represent C structs/pointers in Hack", and instead of trying to give guidelines for when to use them, give a few examples?

Guidelines would be useful eventually, but if we're focussed on only addressing the subproblems we need doing now, we should avoid making the README imply they're suitable for the general problem

[fredemmott]

it is not a priority to review existing sockaddr design.

Not fully, but it should be reviewed enough to give the two APIs a consistent feel, and if we do want guidelines/rules, existing usage does need to be a consideration.

If it's just writing down more potential approaches without the 'if x' part, then yep, we don't need to look at it beyond 'does OS\posix_spawn() feel like the rest of OS?`

[fredemmott]

Thread: How should a C type that is used like a vec<A|B|C> be represented?

For example, posix_spawn_file_actions_t

• use vec<WeCreateASupertype> with vec[foo_addA(), ...]
• use vec<WeCreateASupertype> with vec[foo_OTHERVERB_a(), ...] given that it's no longer being added to a foo_t
• use vec[foo_t::a(), ...]
• use vec[foo_t\a(), ...]
• preserve the $container = foo_init(); foo_addA($a, ...); foo_addB($b, ...) C API
• use (new container_t())->addA(...)->addB(...)
• use container_t_init()->addA(...)->addB(...)
• support any combination of the previous 3

[Atry]

The only limit is that it assumes that the order between the setters is ignorable.

If the order between setters matters, we can encode it as following:

type posix_spawn_file_actions_t = vec<shape(
  ?'posix_spawn_file_actions_addopen' => (
    /* filedes = */ int,
    /* path = */ string,
    /* flags = */ int,
    /* mode = */ int,
  ),
  ?'posix_spawn_file_actions_adddup2' => (
    /* filedes = */ OS\FileDescriptor,
    /* newfiledes = */ int,
  ),
  ?'Bsd\\posix_spawn_file_actions_addchdir_np' => string,
  // Other posix_spawn_file_actions_addxxx functions ...
)>;

[fredemmott]

• the order matters
• if we used existing structures, I think we'd want shapes, not tuples: open is a particularly strong example of where it won't be intuitive when writing the code - IDEs will just say '(int, string, int, int)`

The one-shape-for-any approach technically works, but feels a bit strange: the C interface is really vec<FileAction>, and this approach is vec<?ConcreteActionA & ?ConcreteActionB & ?ConcreteActionC>

[Atry]

The one-shape-for-any approach technically works, but feels a bit strange: the C interface is really vec<FileAction>, and this approach is vec<?ConcreteActionA & ?ConcreteActionB & ?ConcreteActionC>

If we can use union types, it would be

type posix_spawn_file_actions_t = vec<shape(
  'posix_spawn_file_actions_addopen' => shape(
    'filedes' => int,
    'path' => string,
    'flags' => int,
    'mode' => int,
  ),
) | shape(
  'posix_spawn_file_actions_adddup2' => shape(
    'filedes' => OS\FileDescriptor,
    'newfiledes' => int,
  ),
) | shape(
  'Bsd\\posix_spawn_file_actions_addchdir_np' => shape(
    'path' => string,
  ),
)
// Other posix_spawn_file_actions_addxxx functions ...
>;

[fredemmott]

FWIW, in the environments where they work, union types don't operate on a field basis - this would have to be shape(foo)|shape(bar), not shape('foo' => foo | 'bar' => bar).

While we could use them in the HSL, I don't currently think we should do things that aren't permitted/representable in user code - but perhaps we should.

Going back to HSL IO (intersection types), for example, while we could do ReadHandle & WriteHandle, this is invalid in most projects - but we could possibly define type ReadWriteHandle = ReadHandle & WriteHandle then users coudl continue to use ReadWriteHandle, but we'd have simpler internals. That wouldn't be as good as fully allowing intersection types in Hack, but it could still be worthwhile - and it would be backwards+forwards compatible except for people who implement the interfaces in custom handles.

For context, intersection types aren't planned, but are more likely than union types; union types are held back out of concerns for typechecker performance implications, and this is partially based on the experience of enabling them in flow.

[fredemmott]

Also, I don't think unions really work well in hack for shapes - unions/intersections work well with hierarchical types, but not so much for structural types. An as/is/parameter type check would have to traverse all the values and all the shape definitions, rather than just the type data.

If we do eventually un-ban union/intersection types, it seems likely that we would ban them for shapes/tuples for performance (both runtime and typechecker)

[fredemmott]

Thread: Non-portable extensions

For example, there is posix_spawn_file_actions_addchdir_np in BSD libc and Glibc (expected without the NP suffix in Issue 8).

• should we directly mirror the _np extension...

  • when it's only in glibc or BSD libc, not both
  • when glibc and BSD libc have compatible extensions
  • when glibc and BSD libc have incompatible extensions
  • when neither have it, but we want it in HHVM

... or should we create _np_bsd, np_glibc, _np_hhvm suffixes for clarity?

[fredemmott]

This _np convention seems to only be used for functions - for example, POSIX_SPAWN_SETEXEC is an apple extension (which gets posix-exec-without-fork behavior)

[Atry]

even if Bsd and Glibc behave identically, just because it's not standardized?

I think we can directly put the function in the OS namespace as long as it behaves identically among all the OS we supported, because we did not promise that OS functions would literally reflect POSIX functions.

[Atry]

Also I don't think we should normally use the Glibc or HHVM namespace. Neither Glibc nor HHVM is an OS and the users might not care about where the function comes from. I think Bsd and Linux could be good namespace names for OS specific functions.

(However, if we have a FFI code generator to export all functions from a C library, we might want to use the library name as the namespace)

[fredemmott]

I think we can directly put the function in the OS namespace as long as it behaves identically among all the OS we supported, because we did not promise that OS functions would literally reflect POSIX functions.

What should we do if we expand our listed of supported operating systems in the future? We should ideally include Windows in the distant future.

[fredemmott]

Neither Glibc nor HHVM is an OS and the users might not care about where the function comes from.

I think this is more about OS\ being a compromise name more than correctness (it was almost called Libc\); hardly anything here is actually OS provided.

Using Linux\ to mean glibc in particular is risky: it's possible to use glibc on non-linux platforms, and to use other libcs on Linux. While it's definitely not worth the work for us to build and test hhvm on linux/uclibc, it has occasionally been requested for containerized deployment, and we should keep our options open for it in the future.

Nit (and I know, I started this one :) ) - we would want to distinguish between 'apple extensions', 'all BSDs', 'Apple and NetBSD', etc.

[Atry]

Since the goal of OS package is to provide an unbiased mechanism to access C API, I wonder if we could generate the Hack definition automatically from C headers. An example of this approach in another language is LWJGL.

[fredemmott]

If the shape contains optional fields, then each field would have 3 states(true/false/unset), which does not map well to the 2 state flags.

Not all flags are two-state; I think this depends on what the C interface is: for example, int bitmasks are 2-state, ones only accessed through functions arguably have a third unset/default state

Both shapes and keyset also require the assumption that there are Hack definitions for all flags that matter; in particular, that set_flags(get_flags() | ADDITIONAL_FLAG) either:

• does not need to be supported
• is not lossy - i.e. the C function backing get_flags() never returns flags that aren't defined in Hack - as putting them in the keyset is invalid id they're not in the enum

A lot of the ptr-as-shapes discussion also applies here.

Another possibility would be to add a Bitflags<EnumType> with safe enum-based test-and-set functions, but also with unsafe raw int access.

[fredemmott]

Regardless of what we do, if it's not raw ints with bitmasking directly:

• we should ask if the new rules say that OS\open $flags and $mode are mistakes (similar for mkostemps()
• ditto for fcntl, setsockopt, and getsockopt; I've listed these separately as I feel there's a much stronger argument that these need to support arbitrary non-platform non-portable values

[fredemmott]

That said, even open() appears compelling for allowing platform-specific flags, e.g.:

// macos
           O_SHLOCK        atomically obtain a shared lock
           O_EXLOCK        atomically obtain an exclusive lock
// linux
       O_DSYNC
              Write operations on the file will complete according to
              the requirements of synchronized I/O data integrity
              completion.

              By the time write(2) (and similar) return, the output data
              has been transferred to the underlying hardware, along
              with any file metadata that would be required to retrieve
              that data (i.e., as though each write(2) was followed by a
              call to fdatasync(2)).  See NOTES below.

Portable software can't blindly use these, but it seems reasonable to allow hack programs to do platform-specific code based on these.

Interestingly, neither macos nor linux support 2 of the 'required' flags:

O_EXEC
Open for execute only (non-directory files). The result is unspecified if this flag is applied to a directory.

O_EXEC does not require read permissions, and is similar to O_PATH on glibc

O_SEARCH
Open directory for search only. The result is unspecified if this flag is applied to a non-directory file.

[Atry]

I think newtype Bitflags<TEnum as int> as int = int would be a good encoding, like this:

newtype Bitflags<+TEnum as int> as int = int;
function bitflags<TEnum as int>(TEnum ...$flags): Bitflags<TEnum>;
function add_flag<TEnum as int>(Bitflags<TEnum> $flags, TEnum $additional_flag): Bitflags<TEnum>;
function contains_flag<TEnum as int>(Bitflags<TEnum> $flags, TEnum $flag_to_test): bool;
function from_keyset<TEnum as int>(keyset<TEnum> $flags): Bitflags<TEnum>;

// Throw if there is any undefined flag
function to_keyset<reify TEnum as int>(Bitflags<TEnum> $flags): keyset<TEnum>;

1. It supports type safe access to predefined flags in the enum, with the help of the utility functions.
2. It supports access to undefined flags by casting Bitflags to int.
3. The runtime representation of OS\open is unchanged when migrating to Bitflags, and the migration can be as easy as a type cast to the newtype.

I would expect that TEnum defines all known possible flags, including those platform specific flags.

[fredemmott]

I would expect that TEnum defines all known possible flags, including those platform specific flags.

Addressing this first as the other points assume it: this means we must check all supported libcs, and keep up to date with new releases of glibc and the macos sdk, and check for changes. I don't think this is a commitment we can (or should) make; we're much better off allowing unknown values.

Of course, if we don't define them all, the typechecker can't enforce that the values are valid - though it can't enforce that they're not going to be ignored or do something different anyway, as e.g. a MacOS-specific constant may have the same value as a different constant on linux that is also in the bitmask. The type safety is misleading.

the migration can be as easy as a type cast to the newtype.

This is not supported:

Typing[4195] Invalid as expression hint [1]
-> The as operator cannot be used with a type with generics, because generics are erased at runtime [1]

test.hack:10:19
     8 |
     9 | function main(): void {
[1] 10 |   do_stuff(123 as Foo<_>);
    11 | }

Can't be fixed with reify as reify is only supported for classes and functions.

---

Using bitflags with the utility functions would also be extremely verbose - if there were a good way to convert from int to bitflags, I suspect users would generally choose to use it instead of the functions, even when not migrating existing code.

My current view is that for OS\, we should just use raw ints for bitflags:

• it leads to concise and readable code
• it's the thinnest approach over C
• it doesn't require us to predefine all acceptable flags and keep the enum up to date

[Atry]

I just created #170 to include the conclusions from here

[fredemmott]

How to represent posix_spawnattr_t

I'm splitting this out from 'How should a C type that is used like a shape be represented?' as:

• that's a bad question (mine :) )
• posix_spawnattr_t is actually special IMO, and not suitable as a general example:

I think it's special as it is extremely shape-like: it has both fields, and flags that indicate the presence of these fields. This is necessary as 0 is a valid, meaningful value for (almost) all of these fields, and needs to be distinguished between 'leave as default'. This is exactly what shapes do.

• The distinction between 0 and default appears to be important for all the standard options
• MacOS extensions (posix_spawnattr_setbinpref_np, posix_spawnattr_setspecialport_np, posix_spawnattr_setexceptionports_np) do not have flags, and NULL/0 are either invalid values, or equal to the default behavior

posix_spawnattr_setschedparam is arguably a counterexample:

• it's in the standard, linux, and some BSDs, but not macos
• however, the default value is explicitly undefined. 0/null may or may not be a behavior change