fix(hir): keep the null-guard for an optional method call on a process.env read (#6058)

`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).

Co-authored-by: Ralph Küpper <ralph@skelpo.com>

Author: proggeramlug

crates/perry-hir/src/lower/lower_expr/helpers.rs

@@ -270,6 +270,16 @@ pub(crate) fn opt_call_receiver_repeatable(expr: &Expr) -> bool {
         | Expr::Number(_)
         | Expr::String(_)
         | Expr::Bool(_) => true,
+        // `process.env` and env-var reads are pure, side-effect-free, and
+        // stable within an expression, so they are safe to evaluate more than
+        // once (the guard AND the call). Without this, `process.env?.[k]?.m()`
+        // has a NON-repeatable receiver `IndexGet { object: ProcessEnv, .. }`,
+        // so the optional-method null-guard is dropped and `.m()` is called on
+        // the unguarded `undefined` an unset var reads as — e.g.
+        // `process.env.ANTHROPIC_BASE_URL?.trim()` returned the string
+        // "undefined" instead of short-circuiting to `undefined`.
+        Expr::ProcessEnv | Expr::EnvGet(_) => true,
+        Expr::EnvGetDynamic(key) => opt_call_receiver_repeatable(key),
         // `a.b` / `a[const]` chains over repeatable receivers stay repeatable
         // (property reads are not side-effecting in this codebase's model).
         Expr::PropertyGet { object, .. } => opt_call_receiver_repeatable(object),

crates/perry/tests/issue_optchain_env_receiver_guard.rs

@@ -0,0 +1,93 @@
+//! An optional method call whose receiver is an inline `process.env` read —
+//! `process.env?.[key]?.trim()` / `process.env.KEY?.trim()` — dropped its
+//! per-receiver null-guard and called the method on the `undefined` an unset
+//! variable reads as. It returned the STRING `"undefined"` (from stringifying
+//! the missing value) instead of short-circuiting to `undefined`.
+//!
+//! Root cause: `process.env[k]` lowers to `IndexGet { object: ProcessEnv, .. }`,
+//! and `opt_call_receiver_repeatable` did not treat `ProcessEnv`/env reads as
+//! repeatable, so the `a?.b?.method()` lowering took its "receiver not safe to
+//! duplicate → skip the guard" path. Env reads are pure and idempotent, so
+//! they are safe to evaluate twice (guard + call); classifying them repeatable
+//! restores the guard.
+//!
+//! This shape is ubiquitous — SDKs read config via `readEnv(k)?.trim()`; a
+//! popular client's base-URL default is
+//! `process.env.BASE_URL?.trim() ?? "https://…"`, which silently became the
+//! string `"undefined"` and produced `new URL("undefined/…")` failures.
+
+use std::path::PathBuf;
+use std::process::Command;
+
+fn perry_bin() -> PathBuf {
+    PathBuf::from(env!("CARGO_BIN_EXE_perry"))
+}
+
+/// Compile+run `source` with `env` cleared of the probe var names, return trimmed stdout.
+fn compile_and_run(dir: &std::path::Path, source: &str) -> String {
+    let entry = dir.join("main.ts");
+    let output = dir.join("main_bin");
+    std::fs::write(&entry, source).expect("write entry");
+
+    let compile = Command::new(perry_bin())
+        .current_dir(dir)
+        .arg("compile")
+        .arg(&entry)
+        .arg("-o")
+        .arg(&output)
+        .output()
+        .expect("run perry compile");
+    assert!(
+        compile.status.success(),
+        "perry compile failed\nstdout:\n{}\nstderr:\n{}",
+        String::from_utf8_lossy(&compile.stdout),
+        String::from_utf8_lossy(&compile.stderr)
+    );
+
+    let run = Command::new(&output)
+        // Ensure the probed vars are unset so the reads short-circuit.
+        .env_remove("PERRY_TEST_UNSET_A")
+        .env_remove("PERRY_TEST_UNSET_B")
+        .env_remove("PERRY_TEST_SET_C")
+        .env("PERRY_TEST_SET_C", "  hello  ")
+        .output()
+        .expect("run compiled binary");
+    assert!(
+        run.status.success(),
+        "compiled binary exited non-zero: {:?}\nstdout:\n{}\nstderr:\n{}",
+        run.status,
+        String::from_utf8_lossy(&run.stdout),
+        String::from_utf8_lossy(&run.stderr)
+    );
+    String::from_utf8_lossy(&run.stdout).trim().to_string()
+}
+
+#[test]
+fn optchain_method_on_inline_process_env_short_circuits() {
+    let dir = tempfile::tempdir().expect("tempdir");
+    let out = compile_and_run(
+        dir.path(),
+        r#"
+// Unset var, computed key, chained method (the base-URL default shape).
+const dyn = (k: string) => process.env?.[k]?.trim() ?? "DEFAULT";
+// Unset var, static key, chained method.
+const stat = process.env.PERRY_TEST_UNSET_B?.trim() ?? "DEFAULT";
+// SET var still flows through (proves we didn't just null everything).
+const setVal = process.env.PERRY_TEST_SET_C?.trim() ?? "DEFAULT";
+// A missing read must be JS `undefined`, not the string "undefined".
+const raw = process.env?.["PERRY_TEST_UNSET_A"]?.trim();
+
+process.stdout.write(
+  "dyn=" + dyn("PERRY_TEST_UNSET_A") +
+  " stat=" + stat +
+  " set=" + setVal +
+  " rawIsUndef=" + (raw === undefined) +
+  " rawType=" + typeof raw + "\n"
+);
+"#,
+    );
+    assert_eq!(
+        out, "dyn=DEFAULT stat=DEFAULT set=hello rawIsUndef=true rawType=undefined",
+        "optional method call on inline process.env read"
+    );
+}