fix(codegen): rewrite VALUE references to renamed functions, not just calls (#1598)

CHANGELOG.md

@@ -9,6 +9,30 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 `main`, the release pipeline automatically replaces `[current]` with the next
 version number before tagging the release.
 
+## [current]
+
+### Fixed
+- codegen: a top-level function whose name is renamed — a leading
+  underscore (#279, kept out of C's reserved namespace) or a collision
+  with a declared extern (#1366) — now has its VALUE references rewritten
+  too, not just its calls (#1598). `rename_calls_to` matched only
+  `AST_FUNCTION_CALL`, so `takes_fp(_handler)` (an `AST_IDENTIFIER`) kept
+  the old spelling while the definition moved, and the emitted C named a
+  symbol that no longer existed: *"'_h_health' undeclared; did you mean
+  'ae_h_health'?"* — the compiler suggesting the definition it had just
+  renamed. This blocked aeo's HTTP route registration, which is
+  `server_get(raw, "/health", _h_health, 0)` throughout.
+  The extern-collision half failed far more quietly and was fixed with
+  it: the un-renamed reference resolved to the real libc symbol the
+  extern declared, so the program linked cleanly and SEGFAULTED at
+  runtime (verified on the pre-fix compiler), handing libc's
+  `puts(const char*)` an int.
+  The rename skips any function body that REBINDS the name, because a
+  local may legally shadow a top-level function and is emitted verbatim —
+  rewriting it would break a program that compiles today. That direction
+  is deliberate: it can only leave a reference un-renamed, never rename a
+  binding that should have stayed put.
+
 ## [0.542.0]
 
 ### Added

docs/c-interop.md

@@ -1115,6 +1115,28 @@ function `bind(...)` and the compiler does the right thing. The only
 place you see the prefix is in the emitted C (useful if you're
 debugging with `nm`, `objdump`, or a stack trace).
 
+A renamed function is equally usable **as a value** — passing it across an
+ABI boundary as a `ptr` handler reaches the renamed definition, not a
+same-named libc symbol:
+
+```aether
+_h_health(req: ptr, res: ptr, ud: ptr) -> int { return 0 }
+server_get(raw, "/health", _h_health, 0)   // references ae_h_health
+```
+
+(Before #1598 only *calls* were rewritten, so a value reference kept the
+original spelling and either failed to compile or — for an
+extern-collision rename — silently bound to the libc symbol.)
+
+One sharp edge worth knowing: a **local variable may shadow a top-level
+function's name**, and such a local is emitted verbatim rather than
+renamed. That works on its own, but a single function that *both* shadows
+the name and passes the function as a value cannot work in either
+spelling — in the emitted C the value reference would precede the local's
+declaration. Pick one meaning per scope; the trailing-underscore
+file-local convention (`h_health_`) is the idiomatic way to keep them
+apart.
+
 ### Collisions with the Aether standard library
 
 The curated list above covers libc, but `libaether.a` exports a couple of

tests/ae_sweep_prune.txt

@@ -69,6 +69,7 @@ tests/integration/extern_tuple_return/
 tests/integration/extern_tuple_var_passthrough/
 tests/integration/fault_cross_module/
 tests/integration/fn_typed_local_call/
+tests/integration/fn_value_rename/
 tests/integration/fs_glob_recursive_dedupe/
 tests/integration/fs_read_binary_nul/
 tests/integration/fs_write_binary_nul/

tests/integration/fn_value_rename/probe.ae

@@ -0,0 +1,35 @@
+// #1598 (extern-collision half): an Aether function whose name matches a
+// declared extern is renamed by rename_extern_colliding_functions (#1366)
+// to `ae_<name>`. That pass shared rename_calls_to with the
+// leading-underscore pass, so it had the identical hole: a VALUE
+// reference kept the old spelling.
+//
+// This half is nastier than the underscore one. There the mismatch was a
+// compile error; here the un-renamed reference RESOLVES — to the real
+// libc symbol the extern declared. Pre-fix this program linked cleanly
+// and SEGFAULTED at runtime, calling libc's puts(const char*) with an
+// int. A silent wrong-symbol bug, which is why it earns a runtime test
+// rather than a compile check.
+@extern("puts") puts(s: string) -> int
+@extern("reg_cb") reg_cb(h: ptr)
+@extern("run_cb") run_cb(v: int) -> int
+
+// Collides with the `puts` extern above -> renamed to ae_puts.
+puts(v: int) -> int { return v + 200 }
+
+main() {
+    // Passed as a VALUE: must reference the RENAMED definition, not the
+    // libc symbol of the same name.
+    reg_cb(puts)
+    got = run_cb(7)
+    if got != 207 {
+        println("FAIL: expected 207 from the renamed handler, got ${got}")
+        return
+    }
+    // And a direct call must reach the same function.
+    if puts(1) != 201 {
+        println("FAIL: direct call to the renamed fn is wrong")
+        return
+    }
+    println("PASS: extern-colliding fn correct as value and as call")
+}

tests/integration/fn_value_rename/test_fn_value_rename.sh

@@ -0,0 +1,48 @@
+#!/bin/sh
+# #1598: when codegen renames a top-level function, every reference has to
+# move with the definition — calls AND bare value references.
+#
+# Two passes rename functions, and both shared the call-only rewrite:
+#   * rename_leading_underscore_functions (#279)  `_h`   -> `ae_h`
+#   * rename_extern_colliding_functions   (#1366) `puts` -> `ae_puts`
+#
+# The underscore half is pinned by tests/regression/ (it fails to compile,
+# which the bulk .ae sweep catches). This directory pins the EXTERN half,
+# which needs a C sidecar to exercise — and which failed far more quietly:
+# the un-renamed reference resolved to the real libc symbol the extern
+# declared, so the program linked cleanly and SEGFAULTED at runtime,
+# handing libc's puts(const char*) an int. Verified on the pre-fix
+# compiler: "Program crashed (signal 11: segmentation fault)".
+#
+# So this is a runtime test on purpose: a compile-only check would have
+# passed on the broken compiler.
+
+set -e
+
+SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
+ROOT="$(cd "$SCRIPT_DIR/../../.." && pwd)"
+AE="$ROOT/build/ae"
+[ -n "${EXE_EXT:-}" ] && AE="$AE$EXE_EXT"
+
+[ -x "$AE" ] || { echo "  [SKIP] fn_value_rename: ae not built"; exit 0; }
+
+cd "$ROOT" || exit 1
+TMP="$(mktemp -d)"
+trap 'rm -rf "$TMP"' EXIT
+
+fail() {
+    echo "  [FAIL] fn_value_rename: $1"
+    [ -f "$TMP/out.log" ] && sed 's/^/        /' "$TMP/out.log"
+    exit 1
+}
+
+AETHER_HOME="$ROOT" "$AE" run "$SCRIPT_DIR/probe.ae" \
+    --extra "$SCRIPT_DIR/support.c" >"$TMP/out.log" 2>&1 || fail "probe did not run"
+
+# The probe prints its own verdict; a segfault or a wrong value would show
+# up as a missing PASS line rather than a diff.
+grep -q "PASS: extern-colliding fn correct as value and as call" "$TMP/out.log" \
+    || fail "extern-colliding function was wrong as a value or as a call"
+
+echo "  [PASS] fn_value_rename: renamed fn correct as value and as call (#1598)"
+exit 0

tests/regression/test_issue1598_fn_name_shadowed.ae

@@ -0,0 +1,30 @@
+// Regression companion to test_issue1598_underscore_fn_as_value.ae.
+//
+// The #1598 fix teaches the leading-underscore rename (#279) to rewrite
+// bare VALUE references, not just calls. The hazard is that a local may
+// legally shadow a top-level function's name, and such a local is a
+// plain variable emitted verbatim — so a blanket identifier rename would
+// rewrite the variable and break a program that compiles today.
+//
+// The rename therefore skips any top-level function whose body rebinds
+// the name. This file is that skip: it compiled before the fix and must
+// keep compiling after it.
+//
+// Separate file from the value-reference test on purpose: the decision
+// is per top-level function, so a body that both rebinds the name AND
+// passes the function as a value cannot work either way — in C the
+// reference would precede the local's declaration. Aether resolves such
+// a name to the local throughout, which is what this pins.
+
+_thing() -> int { return 1 }
+
+main() {
+    // `_thing` here is a LOCAL, shadowing the function above. It must be
+    // emitted as `_thing`, while the function itself became `ae_thing`.
+    _thing = 42
+    if _thing != 42 {
+        println("FAIL: local shadowing a fn name was rewritten by the rename")
+        return
+    }
+    println("PASS: local shadowing a leading-underscore fn name is untouched")
+}

tests/regression/test_issue1598_underscore_fn_as_value.ae

@@ -0,0 +1,65 @@
+// Regression: #1598 — a top-level function whose name starts with `_`,
+// passed as a VALUE rather than called, emitted an undeclared C name.
+//
+// The #279/MSVCRT pass renames such a definition (`_h` -> `ae_h`) to keep
+// Aether names out of C's reserved leading-underscore namespace, then
+// rewrote references — but only AST_FUNCTION_CALL nodes. A function used
+// as a value is an AST_IDENTIFIER, so it kept the old spelling while the
+// definition had moved:
+//
+//     error: '_h_under' undeclared; did you mean 'ae_h_under'?
+//
+// (The compiler's own suggestion naming the definition it had just
+// renamed.) Both conditions were required — calling `_h_under` was fine,
+// and passing a non-underscore function as a value was fine.
+//
+// Found on 0.541.0 upgrading aeo, where every HTTP route registration is
+// `server_get(raw, "/health", _h_health, 0)` — exactly this shape.
+//
+// This file pins the CALL and VALUE spellings staying in step. The
+// shadowing case a naive "rename every matching identifier" fix would
+// have broken — a local legally taking a function's name, emitted
+// verbatim — lives in test_issue1598_fn_name_shadowed.ae, in its own
+// file because the rename is decided per top-level function: a body
+// that rebinds the name is skipped wholesale, so the two cases cannot
+// share one main(). The extern-collision sibling pass (#1366), which
+// had the identical hole, is pinned by
+// tests/integration/fn_value_rename/.
+
+// A leading-underscore function, used both ways.
+_h_under(n: int) -> int { return n + 1 }
+
+// A plain function, as the control: this shape always worked.
+h_plain(n: int) -> int { return n + 2 }
+
+// Takes a function value. `ptr` is how a handler crosses an ABI
+// boundary — the aeo shape that surfaced this.
+takes_fp(f: ptr) -> int {
+    if f == null { return 0 }
+    return 1
+}
+
+main() {
+    // 1. VALUE use of a leading-underscore function — the bug. Before the
+    //    fix this failed to compile, so reaching runtime at all is the
+    //    assertion; the non-null check confirms a real address was passed.
+    got_under = takes_fp(_h_under)
+    if got_under != 1 {
+        println("FAIL: underscore fn passed as value did not arrive")
+        return
+    }
+
+    // 2. CALL use of the same function must still resolve.
+    if _h_under(41) != 42 {
+        println("FAIL: underscore fn call broken")
+        return
+    }
+
+    // 3. The control: a plain function as a value.
+    if takes_fp(h_plain) != 1 {
+        println("FAIL: plain fn passed as value did not arrive")
+        return
+    }
+
+    println("PASS: underscore fn as value, call, and control")
+}