fix(hir): keep the null-guard for an optional method call on a process.env read - #6058
Conversation
…ss.env` read
`process.env?.[k]?.method()` (env read accessed inline) silently dropped the
per-receiver null-guard on the `?.` before the method, so the method was
invoked on the `undefined` an unset variable reads as. The result was the
STRING "undefined" (from stringifying the missing value) instead of a
short-circuit to `undefined`.
Cause: `process.env[k]` lowers to `IndexGet { object: ProcessEnv, .. }`, and
the `a?.b?.method()` lowering in `arm_optchain` only re-adds the receiver
null-guard when the receiver is `opt_call_receiver_repeatable`. That predicate
did not list `ProcessEnv` / env reads, so the receiver was deemed unsafe to
evaluate twice and the guard was skipped — leaving `process.env[k].method()`
to dereference the unguarded `undefined`. (Hoisting the env read into a local
first — `const e = process.env; e?.[k]?.method()` — worked, because a
`LocalGet` receiver is repeatable and kept the guard.)
Env reads are pure, side-effect-free, and stable within an expression, so they
are safe to evaluate more than once (guard + call). Add `ProcessEnv`,
`EnvGet`, and `EnvGetDynamic` (repeatable iff its key is) to
`opt_call_receiver_repeatable`.
This shape is ubiquitous: SDKs read config via `readEnv(k)?.trim()`, and a
common HTTP-client base-URL default is `process.env.BASE_URL?.trim() ?? "…"`,
which silently became the string "undefined" and produced
`new URL("undefined/…")` failures.
Adds an e2e test covering computed/static keys, a set var flowing through, and
a raw missing read being JS `undefined` (not the string).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change extends the ChangesOptional-call env receiver guard fix
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Problem
process.env?.[k]?.method()— an optional method call whose receiver is an inlineprocess.envread — silently drops the per-receiver null-guard on the?.before the method. The method is then invoked on theundefinedan unset variable reads as, and the result is the string"undefined"(from stringifying the missing value) instead of a short-circuit toundefined.This shape is everywhere: SDKs read config via
readEnv(k)?.trim(), and a common HTTP-client base-URL default isprocess.env.BASE_URL?.trim() ?? "https://…"— which silently became the string"undefined"and then producednew URL("undefined/…")(Invalid URL) at request time.Root cause
process.env[k]lowers toIndexGet { object: ProcessEnv, .. }. Thea?.b?.method()lowering inarm_optchain.rsre-adds the receiver null-guard only when the receiver isopt_call_receiver_repeatable(it appears twice — in the guard and in the call — so it must be safe to evaluate more than once). That predicate whitelistedLocalGet/GlobalGet/This/literals/PropertyGet/IndexGetbut notProcessEnv, soIndexGet { object: ProcessEnv, .. }was deemed non-repeatable → the guard was skipped →process.env[k].method()dereferenced the unguardedundefined.The HIR shows it directly — buggy (one guard) vs. the env-in-a-local form (two guards):
Fix
Env reads are pure, side-effect-free, and stable within an expression, so they are safe to evaluate more than once. Add them to
opt_call_receiver_repeatable:Test
crates/perry/tests/issue_optchain_env_receiver_guard.rscompiles+runs the computed-key and static-key forms with an unset var (must short-circuit to the??default), a set var (must flow through — proves we didn't just null everything), and a raw missing read (must be JSundefined,typeof === "undefined", not the string):dyn=DEFAULT stat=DEFAULT set=hello rawIsUndef=true rawType=undefined.perry-hirlib tests remain 221/0.Note (separate, larger issue)
The underlying pattern — "receiver of
a?.b?.method()is not repeatable → drop the guard" — is also wrong for any side-effecting mid-chain receiver (e.g.sideEffect()?.[k]?.m()), which should bind the receiver to a temp rather than drop the guard or duplicate the effect. This PR only fixes the pureprocess.envfamily (the common, safe case); the general temp-binding fix is a separate, bigger change.Summary by CodeRabbit
Bug Fixes
process.envand similar expressions preserve the expected nullish guard.Tests