introduce reachable enum tag to std.builtin.BranchHint; all functions reachable by default

[andrewrk]

3 problems have the same root cause:

• Compile errors for unused functions (compile errors for unused things #335) cannot be emitted because it is ambiguous whether a function is truly unused or if it is used under a different build configuration (i.e. linux vs windows).
• Functions sometimes bitrot due to being unreferenced. Currently, people resort to filthy, disgusting hacks such as refAllDecls.
• Compile-time detected unreachable is unable to be reported at compile time (comptime-known unreachable is a runtime error when it should be a compiler error #18226), because the function might be never called (or the branch never executed).

Currently, with the exception of _start and main, functions in the Zig language may or may not be reachable. This proposal is to add reachable as a std.builtin.BranchHint enum tag, allowing functions and branches to be marked as reachable, solving all three problems above.

Currently it looks like function-level @branchHint is lowered into ZIR like this:

      %793 = extended(builtin_value(branch_hint)) node_offset:808:5 to :808:23
      %794 = decl_literal(%793, "cold") node_offset:808:17 to :808:22
      %795 = extended(branch_hint(%794)) node_offset:808:5 to :808:23

However, since the builtin has the rule that it must be the first statement of any block, it could be elevated to function flags, observable without peering into the function body. In the case of a literal .reachable value being used, the unused function error could even be caught during ast-check.

There is the question of defaults. Here are two choices:

• (A) Only exported functions default to reachable; other functions default to none. This is pretty similar to status quo.
• (B) All functions default to reachable. This will cause an error for non-pub functions that are not referenced (according to Sema!) which can be silenced with @branchHint(.none);.

Either option would unfortunately mean that a lot functions need to be annotated against the default. For example, every function inside std.os.linux would need to be marked as none so that it does not cause compile errors or bloat when compiling for Windows, and vice versa. Something that might address this could be the ability to set the default branch hint at any scope, for example comptime { @setBranchHint(.reachable); } at file scope. Note that it is already planned to allow overriding safety checking at any scope via this same mechanism.

I think a reasonable path forward would be the more aggressive default (B) combined with the ability to override at any scope.

This has implications for incremental compilation and parallel semantic analysis because it allows the compiler to treat every reachable function as a root node in the dependency graph.

Combined with escape analysis, this would provide a way to report use-after-free of stack locals as compile errors (#3180). For example, if a function always returns a pointer to a local variable, and that function's returned pointer is accessed in a reachable branch, a compile error can be emitted.

If this were implemented, assert could become inline, making it automatically report compile errors in the case that the asserted value was comptime-known (#425).

[Jarred-Sumner]

I think a reasonable path forward would be the more aggressive default (B) combined with the ability to override at any scope.

Are there regression tests for binary size changes in release builds of Zig? I'd be worried about unintentionally increasing every Zig binary size after this change.

(A) Only exported functions default to reachable; other functions default to none. This is pretty similar to status quo.

As a data point, here is a terrible hack we do related to this: https://github.com/oven-sh/bun/blob/73e0637cd33b83f6bc3260ccd2e290ced586ce4c/src/napi/napi.zig#L1779-L1933

[RetroDev256]

A few unhinged ramblings loose thoughts:

I really like this proposal - out of the two options I prefer B. (More errors... and better code!)

I can't say I fully understand the inner workings of @branchHint(). In the case of some functions (example: the panic handler), I want to decouple whether a function is frequently called (it is .cold already) with whether it is called at all (it is almost always .reachable, yet only actually reachable in a program that can panic). Would adding .reachable as a @branchHint() option make it impossible to specify .cold? Or rather, would specifying .cold as the @branchHint() operand make it .reachable by default? I assume that the second behavior is the intended behavior, yet I may want to make it "may or may not be reached", as in .none behavior:

I am assuming that by the .none option, we assert that the function could or could not be .reachable, but won't cause a compiler error if it is not reachable (instead, it would be automatically marked as unreachable in the AST and violently removed from the program binary if it is not eventually used/referenced by a .reachable function).

If I fully understand this proposal, then I would want this to happen to my code:

• exported functions are reachable by default (we don't have to special-case main() and _start() with this, as they are exported in start.zig)
• root level pub functions are .none by default (this may be helpful in the case that the code is actually a module used in other projects, where the end-user may not use all of a modules` functionality)
• All other functions are .unreachable by default, which get marked as .reachable in the AST parser if they are called/referenced from a .reachable function (or chain of functions leading to a .reachable function). This would probably be the best place to emit a compiler error for a .unreachable function.

I don't know if what I wrote made much sense, but I also don't know if adding "function reachability" hints to the @branchHint() builtin would be a good idea (due to the ramble about the "cold" vs "reachable" paragraph I wrote earlier).

Whatever option is taken though, I would say that accepting this proposal in any form would be great, and I particularly like the idea of raising @branchHint() to function flags, instead of the first item inside of a block.

[rohlem]

The main mechanism proposed is to force analysis of functions via @branchHint(.reachable).
Having such an ability is interesting for developer quality-of-life.

As pointed out above, other branch hints such as .cold could be intended regardless of this new feature.
Therefore it would imo compose better if it were decoupled as a totally separate @forceAnalysis();/@lazyAnalysis(); builtin.
At that point we could also make it a keyword eager/lazy to add to any container-level declaration.
That way we could also mark data constants as pub lazy const x: S = .init();, which imo also looks sensible.

Another unfortunate asymmetry I wanted to point out is that, while it works for single-instantiation functions,
functions with comptime or anytype parameters can (currently, afaik) only be fully analyzed once a callsite specifies enough about the arguments to instantiate it.
For them to benefit from eager analysis, f.e. by triggering compile errors, afaik we'd need a separate fuzzy analysis mode that only analyzes the independent parts of a function (which doesn't seem worth the effort to me personally).

[190n]

As a data point, here is a terrible hack we do related to this: https://github.com/oven-sh/bun/blob/73e0637cd33b83f6bc3260ccd2e290ced586ce4c/src/napi/napi.zig#L1779-L1933

I don't think that's really related to any change Zig could make: those functions are all declared and defined in C++, and I thought the dead code elimination that we're preventing had been done by the linker, not Zig. We just chose our Zig code as the place to reference those functions from to stop them from being deleted, as opposed to referencing them from a C or C++ object.

[ethernetsellout]

This proposal is misidentifying the root cause of the problem: Zig has no way of detecting whether and under what conditions a function gets used, due to different build configurations. It's also applying a solution which only works in a subset of cases, and causes surprising behavior in edge cases (you write @branchHint(.unlikely), and suddenly your function is no longer analyzed because you've opted out of .reachable).

However, since the builtin has the rule that it must be the first statement of any block, it could be elevated to function flags, observable without peering into the function body. In the case of a literal .reachable value being used, the unused function error could even be caught during ast-check.

Unfortunately, this also means that it would be impossible to specify, say, both .cold and .reachable, addressing the question of RetroDev256. Overall adding this as a tag to @branchHint seems inappropriate; this is not actually a branch hint, it's essentially a function attribute telling the compiler to force analysis. Also, @branchHint is allowed in conditionals as well, meaning you may indeed have to peer into the function body.

[andrewrk]

I agree, reachability is orthogonal to the other options in @setBranchHint.