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

[paul-hammant]

Closes #1598. The report's diagnosis was exactly right, and the suggested fix (a sibling rename_refs_to used alongside rename_calls_to at both call sites) is what landed.

The bug

rename_calls_to matches only AST_FUNCTION_CALL, but a function used as a value is an AST_IDENTIFIER. So the definition moved and the reference didn't:

error: '_h_under' undeclared (first use in this function); did you mean 'ae_h_under'?

— the compiler suggesting the definition it had just renamed. Blocked aeo's two HTTP binaries, where every route registration is server_get(raw, "/health", _h_health, 0).

The extern-collision half is worse than the underscore half

The issue flagged rename_extern_colliding_functions (#1366) as having the identical hole but had no live repro. I built one — and it fails much more quietly. There the un-renamed reference resolves: to the real libc symbol the extern declared. So the program links cleanly and segfaults at runtime, handing libc's puts(const char*) an int. Verified on the pre-fix compiler:

$ ae run probe.ae --extra support.c     # pre-fix
Program crashed (signal 11: segmentation fault)
$ ae run probe.ae --extra support.c     # post-fix
207

A silent wrong-symbol bug rather than a compile error. That's why its regression is a runtime test — a compile-only check passes on the broken compiler.

The scope hazard a naive fix would have hit

A local may legally shadow a top-level function name, and such a local is emitted verbatim while the function is renamed:

int ae_thing(void) { ... }   // the function
int _thing = 42;             // the local — must NOT be rewritten

Renaming every matching identifier would have rewritten the variable and broken a program that compiles today. So rename_all_refs_to skips any top-level function body that rebinds the name. That direction is deliberate: it can only leave a reference un-renamed (the status quo for that one function), never rewrite a binding that should have stayed put.

One sharp edge found by testing, pre-existing

My first test put the value case and the shadowing case in the same main(), and it failed. Investigating showed that shape emits invalid C regardless of this change — the value reference would precede the local's declaration:

int a = takes_fp((void*)(intptr_t)(_fn));   // line 290
int _fn = 7;                                 // line 292

So it's a genuine pre-existing limitation, not something this fix should paper over. The tests are split accordingly and docs/c-interop.md documents it, recommending the trailing-underscore file-local convention to keep the two meanings apart.

Tests

All verified to fail on the unfixed compiler, except the shadowing one — which pins behaviour that must not change, so it correctly passes both ways.

• tests/regression/test_issue1598_underscore_fn_as_value.ae — the issue's 3-line repro (no imports, as requested), plus the call and non-underscore controls from its matrix.
• tests/regression/test_issue1598_fn_name_shadowed.ae — the shadowing case, in its own file because the skip is decided per top-level function.
• tests/integration/fn_value_rename/ — the extern-collision half as a runtime test with a C sidecar.

Also checked by hand: value references inside a non-main function and inside a closure body both work.

Verification

390 unit tests pass. Full .ae sweep: 1002 passing. Five failures, none from this change — windows_crt_symbols (verified identical on the unfixed compiler: an msvcrt-vs-ucrt archive symbol-set issue), http_server_h2 (documented in asks/h2-50-stream-stress-framing-layer-error.md as failing on clean main), http_request_header_iter and http_server_ops (both pass in isolation — sweep load flakes), and contrib_vulkan_portability (no GPU on this box).

🤖 Generated with Claude Code