feat(env): getenv() answers the whole environment, and the superglobals carry it - #802
feat(env): getenv() answers the whole environment, and the superglobals carry it#802Guikingone wants to merge 10 commits into
getenv() answers the whole environment, and the superglobals carry it#802Conversation
PHP separates a variable that is NOT SET from one set to the empty string:
`getenv()` answers `false` for the first and `""` for the second, and every
"is this configured" check is written on that separation — `getenv($n) !== false`.
elephc collapsed them, so that test was true for every name. Silently: no error,
no warning, just the other branch.
The information was never missing. libc already distinguishes the two — a
missing name gives NULL, an empty value gives a valid pointer to a zero-length
string — and `__rt_getenv` already returned a null pointer for the first case.
What discarded it was the descriptor, which overrode the checker's `string|false`
to a plain `Str` "for present and missing variables alike". The checker had it
right the whole time; the EIR result now carries the same union, and the lowering
boxes it through `box_owned_string_or_false_result`, the convention `ob_get_clean`
and a dozen others already use.
The found path copies the value out of the environment block first. That is a
contract requirement rather than an observed crash: the result is boxed as an
OWNED string, and anything later classifying that payload reads its heap header
eight bytes before a string the allocator never handed out. Removing the copy
leaves every test green, because a foreign free is range-rejected — so the reason
is written where the copy is instead of asserted by a test that would pass
against its own bug.
One caller-visible consequence, which is the honest price of an honest type:
assigning the result into a local already typed `string` is now a type error
(`cannot reassign $x from string to string|false`). A fresh local is unaffected,
and the whole existing corpus compiles.
`test_getenv_nonexistent` kept passing against this for as long as it existed:
`strlen(false)` and `strlen("")` are both 0, so it pinned the length and never
the distinction. Its comment now says so, and the new test uses `=== false` in
both directions on one program — the empty-but-set case being what a fix in the
wrong place breaks. Mutation-verified: dropping the boxing reproduces the
original output exactly, `unset:string ... idiom:taken`.
The same shape exists in exactly one other builtin, `readline` (checker
`string|false`, descriptor narrowed to `Str`). Left alone deliberately: it reads
a line rather than an environment variable, PHP strips its newline where `fgets`
keeps it, and EOF is signalled differently — measuring that is its own change,
and bundling it unverified with this one would make neither trustworthy.
`elephc` lib 1488 · bins 1661 · error_tests 1437 · codegen callables 445 ·
ir_backend_smoke 257 · opcache_env_override 12. Output diffed against `php -n`:
identical on every getenv form.
github-actions
Bot
added
area:builtins
`Result type source: shared -> checked`, in the generated builtin page and the registry the generator writes beside it. That is exactly what dropping getenv's `eir_result_type` override means: the EIR result now comes from the checker's `string|false` rather than from a shared override narrowing it to `Str`. The CI gate that caught this regenerates and compares, so the fix is to run the generator, not to edit the page. `audit_builtins.py` and `gen_php_comparison.py` report no further drift.
Greptile SummaryThe PR expands
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/codegen_support/runtime/system/getenv_all.rs | Adds platform-specific live environment enumeration; the corrected x86_64 prologue preserves ABI stack alignment across nested calls. |
| src/superglobals.rs | Adds pay-for-use CLI initialization of environment and server superglobals while retaining the web-mode ownership boundary. |
| src/builtins/system/getenv.rs | Updates checking and result typing for the optional name and local-only parameters. |
| crates/elephc-builtin-contract/src/catalog_data.rs | Expands the shared getenv contract to two optional parameters and a mixed result. |
| docs/php/builtins/filesystem/getenv.md | Updates the generated public signature for the expanded getenv surface. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[PHP source references getenv or CLI superglobals] --> B[Compiler selects required runtime setup]
B --> C{Requested surface}
C -->|getenv name| D[Read one live environment value]
C -->|getenv no arguments| E[Enumerate live process environment]
C -->|CLI superglobals| F[Build startup environment snapshot]
E --> G[Box associative array]
F --> H[Seed ENV and SERVER]
D --> I[Return string or false]
Reviews (8): Last reviewed commit: "docs(boxing): the readline helper took i..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -7056,7 +7062,7 @@ pub(crate) static CONTRACTS: &[BuiltinContract] = &[ | |||
| arity_error: None, | |||
| returns: TypeSpec::Mixed, | |||
| by_ref_return: false, | |||
| summary: "Gets the value of an environment variable.", | |||
| summary: "Gets the value of an environment variable, or the whole environment.", | |||
There was a problem hiding this comment.
This changes the public getenv signature and adds the no-argument array result without the required docs/php/ update or an example under examples/, leaving the new overload, parameters, and return behavior absent from the repository's user-facing guidance.
Context Used: CLAUDE.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/elephc-builtin-contract/src/catalog_data.rs
Line: 7046-7065
Comment:
**Undocumented getenv surface change**
This changes the public `getenv` signature and adds the no-argument array result without the required `docs/php/` update or an example under `examples/`, leaving the new overload, parameters, and return behavior absent from the repository's user-facing guidance.
**Context Used:** CLAUDE.md ([source](https://github.com/illegalstudio/elephc/blob/main/CLAUDE.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
a33980d to
ecc8eaf
Compare
|
Rebased onto #801's docs fix, and regenerated for this PR's own signature change — Same gate as #801, one level up: changing a builtin's parameters changes its generated documentation, and the CI gate regenerates and compares rather than trusting the committed page. Folded into the feature commit rather than added as a follow-up, so that commit is self-consistent. |
|
Swept the prose docs for claims this makes false, and found four. Three were plainly stale — The fourth was worse than stale, because it gave a reason rather than a rule. All four pages are hand-written; the generator was re-run and left them alone, and no generated page drifted. |
github-actions
Bot
added
area:magician
|
Both findings addressed in The x86_64 alignment is a real defect, and it was backwards from what the Removed rather than widened to 16: nothing used those eight bytes, so the padding Not observed as a crash — aarch64 is unaffected and the x86_64 shard is CI's, not On the documentation: |
…ls carry it PHP's CLI SAPI hands a script three things elephc did not have: `getenv()` with no argument answering the environment as an array, a `$_ENV` equal to it, and a `$_SERVER` holding the same plus nine keys of its own. The first was a compile error; the other two existed but were seeded EMPTY, which reads to any program that looks like "not set" rather than like a gap. Measured against `php -n` throughout, on macOS and in a Linux container: the array and its count, a value containing `=` (the FIRST one separates — splitting on the last renames every variable whose value holds one), `false` for a name that is not set, `local_only`, `$_ENV == getenv()`, `$_SERVER`'s nine own keys, and the five request superglobals staying empty as they are in PHP. `local_only` is accepted and ignored, because measurement says that is what it means here: in the CLI SAPI `getenv($n)` and `getenv($n, true)` agree for a shell variable, a `putenv` one and `PATH`, and `getenv() == getenv(null, true)`. There is no environment separate from the process's for the flag to select. The four path-shaped `$_SERVER` keys are `$argv[0]`. PHP names the script it was handed; a compiled program has no script at run time, and the thing that WAS invoked is the closest true answer rather than a fabricated path. Three defects of my own on the way, all the same shape — a convention assumed instead of read: `__rt_hash_set`'s x86_64 registers were guessed and wrong, which would have corrupted the hash silently on Linux. `__rt_hash_new` was called with no arguments at all, so it took whatever was in the capacity register and asked for `garbage * 64` bytes — the exact failure its own docblock warns about. The long one: the result was declared `AssocArray` while the value is a hash BOXED in a Mixed cell, so `count()` read the cell's tag and answered 5 for a 65-entry environment. `getdate` declares `Mixed` for precisely this reason. The type is the representation, and losing `array<string,string>` is the price of the box. The environment is read LIVE rather than from the `envp` handed to `main`. That was the first design and it was wrong in a way only `putenv` shows: libc may reallocate the entry vector when a variable is added, so the startup pointer goes stale — and elephc's own one-name `getenv($n)` goes through libc and saw the addition, so the two halves of one builtin disagreed. Reading it live costs the platform split the startup pointer avoided (`environ` on Linux, `_NSGetEnviron()` on macOS), which is the same shape as `errno` elsewhere in this runtime. The startup capture is removed rather than left behind. `putenv` does NOT reach `$_ENV`/`$_SERVER`, only `getenv()`. Measured, not assumed, and asserted: they are snapshots taken before the program ran, and PHP answers the same asymmetry. Two existing tests pinned the old empty seeding. One of them pinned a diagnostic PHP does not have — `unset($_SERVER); $_SERVER = 5;` is legal PHP and elephc refused it — so its guard moves to `$_GET`, whose type still makes the refusal observable, and the property it actually protects (unset must not abandon global storage) is now measured directly as a heap difference. That measurement turned up a pre-existing leak, confirmed against `getdate()`: a boxed assoc array from a builtin is never released, `frees=2` either way. Not caused here and not fixed here. `elephc` lib 1488 · bins 1661 · error_tests 1437 · codegen misc 41 · constants_and_system 134; fmt and clippy clean. A `--web` build still skips the seeding entirely: it references no `__rt_getenv_all` and keeps its request prelude.
Three said the CLI superglobals are empty, which was the divergence this branch closes: `$_SERVER` now carries the environment plus PHP's nine own keys, `$_ENV` equals `getenv()`, and `getenv()`'s signature has both its optional parameters. The runtime table gains `__rt_getenv_all` and says what `__rt_getenv` now returns for a name that is not set — a null pointer, which is what becomes PHP `false`. The fourth was worse than stale, because it gave a REASON that is now wrong: the opcache override page said an empty environment variable is treated as unset "because `getenv()` cannot distinguish the two". It can, as of the previous commit. The behaviour is unchanged — the override reads its value through a string cast, and an unset name and an empty one both arrive as `""` after it — so what needed correcting was the explanation, not the rule. A doc that argues against its own code is worse than a doc that is merely behind. These four pages are hand-written: the generator was re-run and left them alone, and no generated page drifted.
…ication The comment said "~60 entries, so this avoids a rebuild" beside a request for 256, which is four times what that reason asks for and 16 KB of table per call. The table grows past 75% load, so 128 is what clears 60 entries without a rebuild — the number the sentence was already describing. Left over from bisecting the heap-exhaustion bug, where the capacity was raised to rule out a mid-build reallocation. It was not the cause, and the raise was never walked back. Re-measured against `php -n` on both hosts: six fixtures identical, and the Linux container answers the same, `putenv` case included.
…alse `readline()` had two defects that hid each other. It kept the trailing newline, which `__rt_fgets` supplies because that is right for `fgets` and wrong here; and its descriptor narrowed the checker's `string|false` to a plain `Str`, so end of input came back as `""`. A program reading lines could not tell an empty line from the end of them, and `while (($l = readline()) !== false)` never terminated. Measured against `php -n`, one call per input, five inputs: `"abc\n"` -> `"abc"`, `"abc"` unterminated -> `"abc"`, `"\n"` -> `""`, nothing -> `false`, and `"abc\r\n"` -> four bytes, because php removes exactly one `\n` and the `\r` belongs to the line. The ORDER between the two halves is the whole correctness argument, which is why they are one helper rather than a strip followed by the existing `box_stream_string_or_false_on_empty_result`. That helper reads an empty result as end of input, which is correct for `fgets` — keeping the newline means an empty line is one byte and only EOF is zero. `readline` strips, so stripping first turns a line the user typed into zero bytes and reports EOF for it. Mutation-verified: moving the EOF test after the strip turns the empty-line case from `str0` into `false`. `test_readline` had covered this call since it existed and could not see either defect: it compares `trim($line)`, and trimming removes the newline whether or not `readline` did. Its comment now says so, and the new test prints LENGTHS, because `""` and `false` look alike when echoed and so does a trailing newline. This was the last builtin narrowing a falsy union to a scalar — a sweep of every `BuiltinResultType::Shared` override now finds none, and the EIR test that used `getenv` and then `readline` as its narrowing example says that emptiness is the point rather than hunting for a third. Generated docs regenerated for the declaration change, `result_type: shared -> checked`. `elephc` lib 1488 · bins 1661 · error_tests 1437 · codegen::io 666; fmt and clippy clean.
…ate I never ran CI went red on `Non-Codegen Tests`, both Linux architectures, on a parity test in `elephc-magician`: it asserts the interpreter's declared registry matches the static catalogue, and I had added `local_only` to `getenv` in the catalogue without touching it. The expectation is updated rather than relaxed, and gains the defaults alongside the names — `phpversion` right above it already asserts both, and a parameter that exists with the wrong default is a different function. What actually failed here was my verification, not the change. Every suite I ran was `-p elephc`; CI's `Non-Codegen Tests` runs the WHOLE workspace minus three heavy binaries, so it sees `elephc-magician` and I did not. `cargo test --workspace --lib` is the cheap gate that would have caught it, and it is now green across all thirteen crates. Running the full workspace under `nextest` locally is not that gate: it schedules ~7600 tests including compile-and-link ones in parallel and a dozen fail or time out for that alone. Confirmed by replaying three of them alone — `var_export`, `runtime_cache`, `macos_dead_strip` — all green in isolation, so the batch is the artefact.
Review finding, and it is exactly backwards from what the comment claimed. A `call` leaves `rsp` at 8 mod 16 on entry; the five pushes bring it back to 0. The `sub rsp, 8` then took it to 8 — misaligning `__rt_hash_new`, `__rt_str_persist` and `__rt_hash_set` — under a comment reading "keep rsp 16-byte aligned for the nested calls". The padding is removed rather than widened to 16: nothing used those eight bytes. Prologue and epilogue are five pushes and five pops, and the reasoning is written down so the next reader can check the arithmetic instead of trusting the comment. Not observed as a crash — aarch64 is unaffected and the x86_64 shard is CI's, not this machine's. It is a convention violation that leaves the path at the mercy of any callee that uses aligned moves. Also from review: the new `getenv` surface reaches the docs AND an example. `docs/php/system-and-io.md` and `docs/php/namespaces.md` were updated earlier on this branch; `examples/system-info/main.php` now shows the three forms, the false-vs-empty distinction, `$_ENV`/`$_SERVER`, and the asymmetry where a later `putenv()` reaches `getenv()` but not the snapshots. Its environment section is byte-identical to `php -n`. Workspace `--lib` across 13 crates · bins 1661; fmt clean. Re-measured on Linux: the whole environment surface answers as before, `putenv` case included.
86f6cf2 to
d7dec4d
Compare
|
I reviewed this PR against the current state of In addition to the rebase, I reproduced three blocking issues:
There are also two stale docblocks: the arity test still says that calling The published CI is green and the direct AOT tests pass, but they do not cover the three paths above. My current assessment is changes requested: first rebase onto |
2 participants
PHP's CLI SAPI hands a script three things elephc did not have:
getenv()(no argument)array<string,string>getenv($n, true)count($_ENV)0count($_SERVER)0$_SERVER['argv']The superglobals existed but were seeded empty, which reads to any program
that looks like not set rather than like a gap.
Measured against
php -nthroughout, on macOS and in a Linux container: thearray and its count, a value containing
=(the first one separates —splitting on the last renames every variable whose value holds one),
falsefora name that is not set,
local_only,$_ENV == getenv(),$_SERVER's nine ownkeys, and the five request superglobals staying empty as they are in PHP.
Decisions taken from measurement rather than from the manual
local_onlyis accepted and ignored, because that is what it means here: inthe CLI SAPI
getenv($n)andgetenv($n, true)agree for a shell variable, aputenvone andPATH, andgetenv() == getenv(null, true). There is noenvironment separate from the process's for the flag to select.
The four path-shaped
$_SERVERkeys are$argv[0]. PHP names the script itwas handed; a compiled program has no script at run time, and the thing that
was invoked is the closest true answer rather than a fabricated path.
putenvdoes not reach$_ENV/$_SERVER, onlygetenv(). They aresnapshots taken before the program ran. PHP has the same asymmetry — checked,
because it is easy to get wrong in either direction — and it is asserted.
Seeding stays pay-for-use: only the names the source spells are emitted, which
is also what PHP's
auto_globals_jitdoes, and for the same reason.The environment is read live
The first design captured the
envphanded tomain, which avoids a platformsplit. It was wrong in a way only
putenvshows: libc may reallocate the entryvector when a variable is added, so the startup pointer goes stale — and
elephc's own one-name
getenv($n)goes through libc and saw the addition, so thetwo halves of one builtin disagreed. Reading it live costs the split back
(
environon Linux,_NSGetEnviron()on macOS), which is the same shape aserrnoelsewhere in this runtime. The startup capture is removed rather thanleft behind looking load-bearing.
Three defects of mine on the way, all one shape
A convention assumed instead of read.
__rt_hash_set's x86_64 registers were guessed, and wrong — that would havecorrupted the hash silently on Linux.
__rt_hash_newwas called with no arguments, so it took whatever was in thecapacity register and asked for
garbage * 64bytes. Its own docblock warnsabout exactly that.
AssocArraywhile the value is a hashboxed in a Mixed cell, so
count()read the cell's tag and answered5fora 65-entry environment.
getdatedeclaresMixedfor precisely this reason.The type is the representation, and losing
array<string,string>is the priceof the box.
Two existing tests pinned the old behaviour
One pinned a diagnostic PHP does not have:
unset($_SERVER); $_SERVER = 5;islegal PHP and elephc refused it. Its guard moves to
$_GET, whose type stillmakes the refusal observable, and the property it actually protects —
unsetmustnot abandon global storage — is now measured directly, as a heap difference.
That measurement turned up a pre-existing leak, out of scope here: a boxed
assoc array from a builtin is never released. Confirmed against
getdate(),which leaks its 13 blocks the same way (
allocs=15 frees=2) and predates all ofthis. Same shape likely in
stat(),localtime(),parse_url(),ob_get_status(). Worth its own issue.Suites
elephclib 1488 · bins 1661 ·error_tests1437 · codegen misc 41 ·constants_and_system134;fmtand clippy clean. Verified on Linux in acontainer, including the
putenvcase. A--webbuild still skips the seedingentirely: it references no
__rt_getenv_alland keeps its request prelude.