don't do feature detection with `@hasDecl`

[andrewrk]

Violations:

../lib/std/os.zig:pub const have_sigpipe_support = @hasDecl(@This(), "SIG") and @hasDecl(SIG, "PIPE");
../lib/std/os.zig:    if (@hasDecl(system, "pipe2")) {
../lib/std/debug.zig:pub const have_ucontext = @hasDecl(os.system, "ucontext_t") and
../lib/std/debug.zig:pub const have_getcontext = @hasDecl(os.system, "getcontext") and
../lib/std/debug.zig:        } else if (@hasDecl(os.system, "msync") and native_os != .wasi and native_os != .emscripten) {
../lib/std/debug.zig:        if (@hasDecl(root, "os") and @hasDecl(root.os, "debug") and @hasDecl(root.os.debug, "openSelfDebugInfo")) {
../lib/std/debug.zig:    .freebsd, .openbsd => @hasDecl(os.system, "ucontext_t"),
../lib/std/debug.zig:    if (@hasDecl(windows, "CONTEXT")) {
../lib/std/os/test.zig:const dl_phdr_info = if (@hasDecl(os.system, "dl_phdr_info")) os.dl_phdr_info else anyopaque;
../lib/std/os/test.zig:    if (!@hasDecl(os.system, "rlimit")) {
../lib/std/os/uefi/tables/boot_services.zig:        if (!@hasDecl(protocol, "guid"))
../lib/std/os/linux.zig:    if (@hasDecl(VDSO, "CGT_SYM")) {
../lib/std/crypto/tlcsprng.zig:const os_has_arc4random = builtin.link_libc and @hasDecl(std.c, "arc4random_buf");
../lib/std/crypto/tlcsprng.zig:    if (builtin.link_libc and @hasDecl(std.c, "arc4random_buf")) {
../lib/std/heap.zig:    pub const malloc_size = if (@hasDecl(c, "malloc_size"))
../lib/std/heap.zig:    else if (@hasDecl(c, "malloc_usable_size"))
../lib/std/heap.zig:    else if (@hasDecl(c, "_msize"))
../lib/std/heap.zig:    pub const supports_posix_memalign = @hasDecl(c, "posix_memalign");
../lib/std/fs/File.zig:        if (!@hasDecl(@TypeOf(self.stat), "birthtime")) return null;
../lib/std/fs/Dir.zig:                const next_index = self.index + if (@hasDecl(posix.system.dirent, "reclen")) bsd_entry.reclen() else bsd_entry.reclen;
../lib/std/fs/Dir.zig:    if (@hasDecl(posix.system, "LOCK")) {
../lib/std/builtin.zig:    if (@hasDecl(builtin, "explicit_subsystem")) break :blk builtin.explicit_subsystem;

Marking as bug because there's probably a bug among these instances. This is a brittle way to do feature detection. As a general rule: don't do it.

[rohlem]

This is a brittle way to do feature detection. As a general rule: don't do it.

What alternative mechanism(s) is(/are) recommended?
I guess directly accessing a structured/typed configuration value (such as std.Options introduced in #18712), one for each usage site?

[andrewrk]

Let's look at the very first result:

pub const have_sigpipe_support = @hasDecl(@This(), "SIG") and @hasDecl(SIG, "PIPE");

The first condition is already bitrotted to always returning true without anyone noticing. The second condition will start returning the wrong value when type safety is added by converting the struct namespace to an enum.

Whether the operating system has a pipe signal is determined by the operating system. So that's what the value should be based on - the operating system.

Should emscripten be marked as supporting SIGPIPE? Probably not. It's currently set to true.

[jacobly0]

You missed a funny match:

src/Builtin.zig:        \\/// feature detection (i.e. with `@hasDecl` or `@hasField`) over version checks.

[andrewrk]

Indeed. That text was introduced in a0ad2de

[blblack]

I'm struggling a bit with related issues trying to add a syscall to std.(c|posix|linux) that only exists on some POSIXy platforms, and making sense of all the recent improvements in these areas, and the desire to reduce/eliminate both the usingnamespace and @hasDecl patterns for setting up and/or detecting platform support for a given call.

My example case I'm working through is mlockall(), latest imperfect iteration shown here: 94ef2a6 . I can probably imagine (and perhaps find existing examples in std of!) 10+ other distinct ways to set these up, but they all have varying tradeoffs and it's kind of a minefield unless we've adopted a standard way of doing these things that we're happy with.

The main problem with the variant linked above is that it's relying on both @hasDecl and a typeof check against void in the preamble of its std.posix wrapper (which seem out of fashion, at the very least!). These checks are to ensure that it does an @compileError() when called on a non-supporting platform (a libc without the function, or a platform like plan9 which doesn't use libc and also lacks the syscall), and the same basic check in the test of the posix function, so that we don't try to test it on such platforms.

I could imagine copying the list of supporting libcs/platforms that was used in lib/std/c.zig over to lib/std/posix.zig as well, but then you've got the same list in 2-3 different files where they might drift out of sync and bitrot from each other.

I could also imagine just moving the libc declarations (slightly-redundantly, but seems kinda ok to me tbh) down to all the individual lib/std/c/foo.zig instead of the native_os switching and private.foo magic, and then relying on just @hasDecl without the void typeof check. Still uses @hasDecl, but to me seems at least a little cleaner.

You could also argue to just unconditionally put these C decls in lib/std/c.zig without caring about which platforms support it, and/or similarly for the lib/std/posix.zig interface on top of it. The problem with that approach is that when someone does call it on an unsupported platform, they're now going to end up with an undefined symbol at link time instead of an earlier and clearer error. On top of that, there's no good way to exercise it in lib/std/posix/test.zig at that point, because it will fail on random platforms without some platform-list guard at that layer.

I don't have any stellar ideas for the One True Pattern that seems to make the best design sense for all similar cases. So far, though, various approaches that end up involving @hasDecl seem cleaner than others I can come up with.

[rohlem]

I could imagine copying the list [...], but then you've got the same list in 2-3 different files [...] .

@blblack Just my 2c, but generally for deduplication of common logic I'd recommend writing a common function that is accessed at every usage site.
(To allow for extra logic each usage site can also additionally have its own function which calls the one common logic/implementation.)
This function should probably mostly be evaluated at comptime; marking it as inline fn might help with that.

In the particular case of mlockall you linked, I would personally avoid a conditionally-typed declaration, since it doesn't seem necessary:

• Provide a const mlockall_supported: bool for checking whether the current system supports the function.
  • If querying for other systems should be supported, the logic can be provided as fn mlockallSupported(os: Os) bool, otherwise it can just be written inline as initializer.
• Provide a single fn mlockall which checks mlockall_supported with @compileError on unsupported systems before delegating to the particular implementation.

With this, user code can reliably use mlockall_supported to query native support, or just call mlockall directly and get an appropriate compile error if it isn't supported (instead of a strange type mismatch).

[blblack]

@rohlem - I get what you're driving at, and I'll try to spin that around in my head a bit and think on it. I'm not a big fan of the type mismatch either, it was just the latest of a few different avenues suggested/attempted (after we got rid of relying on usingnamespace to solve part of the problem, the way many such things did until recently). The hard part is maybe finding the right layers to put some bits of this in (e.g. std.c vs std.c.foo vs std.os.foo vs std.posix) while keeping things as minimal and elegant and future-proof as possible (and preferably avoiding having any missing syscall get "diagnosed" by an eventual link-time missing symbol error, which seems like the worst way to find out). Anyways, I'll ponder your suggestion overnight and make a new attempt at the mlockall patch and see how it plays out, thank you!

[jayschwa]

Whether the operating system has a pipe signal is determined by the operating system. So that's what the value should be based on - the operating system.

Using switch (builtin.os.tag) for feature detection should be thought of the same way as browser sniffing. Consider the recent change:

const have_rlimit = switch (builtin.os.tag) {
    .windows, .wasi => false,
    else => true,
};
if (!have_rlimit) return;

Defaulting to true isn't correct for freestanding or other since nothing can be assumed about the capabilities of those targets. It likely isn't correct for many other esoteric targets as well. You could invert the logic so that false is the default, but then you've got to remember to include xyzbsd in your long laundry list of operating systems that have the feature.

Maybe @hasDecl isn't cutting it and there's a better pattern to adopt, but I don't think using OS tag in more places is going to be a satisfying alternative that scales well with either contributors or operating system support.

[andrewrk]

The problem here is that there is no precise enough way to specify intent. The OS is not precise enough and whether a particular namespace has a declaration is also not precise enough. The code needs to be able to ask the correct question.