test(nulls): lock in ?? string-miss default selection#605
Merged
nahime0 merged 1 commit intoJul 24, 2026
Conversation
…o#554) Add regression coverage for issue illegalstudio#554: `($a[i][j] ?? 'dflt')` must select the default when the accessed slot is statically typed `Str` and the read misses, while a real present empty string keeps `''`. Root cause (already resolved upstream): `??` lowers its left-hand access silently and tests the result with `IsNull`. Before PR illegalstudio#533, `emit_is_null_result` let statically-known `PhpType::Str` values fall through to constant-false, so a missed string slot (fallback `{ptr = NULL_SENTINEL, len = 0}`) was indistinguishable from an empty string — every miss yielded `''` instead of `'dflt'`. PR illegalstudio#533 closed the gap by routing `PhpType::Str` through `emit_string_null_bool`, which compares the string pointer against `NULL_SENTINEL`; legitimate empty strings never carry that pointer (they use a null/valid data pointer), so the distinction is sound. These tests had no dedicated coverage. They pass on current `main` and fail when illegalstudio#533's `PhpType::Str` arm is reverted to constant-false, guarding the behavior against silent regression. Indices are bound to `$argc`-derived locals so the element read stays statically `Str` (an inline arithmetic index is typed `Mixed` and would route through the boxed-Mixed null path instead). Covers the issue's regression checklist: first-index miss, second-index miss, present empty string, string-keyed associative arrays, nullable string locals/returns, `??`-path silence, and heap-debug cleanliness. Verified under `--ir-opt=on` and `--ir-opt=off`. Claude-Session: https://claude.ai/code/session_01HRvbcjPHa3kAojdCjNZL5L
github-actions
Bot
added
area:triage
Guikingone
pushed a commit
to Guikingone/elephc
that referenced
this pull request
Jul 23, 2026
A `match` expression with no `default` arm throws `UnhandledMatchError` at the implicit no-match point, and codegen materializes that class reference — but it was not among elephc's declared builtin throwables, so every default-less `match` failed type resolution with "Undefined class: UnhandledMatchError" (ward-sse DatastarEventSerializer and other match-dispatch classes). Declare `UnhandledMatchError extends Error` alongside TypeError/ValueError/ ArithmeticError in `inject_builtin_throwables`, and add it to the autoload builtin-class list. Like its siblings it is a nominal marker subclass — constructor/getMessage/etc. are inherited from Error transitively. Whole-codebase survey 852 -> 854 OK (80.4%), zero regressions: error_tests byte-identical to baseline, full codegen sweep unchanged at the pre-existing 440 failures, a default-less `match` now compiles and runs natively. Adds regression test test_match_without_default_compiles. Refs illegalstudio#605. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Member
|
Given that this PR introduces only tests to cover what #533 fixed for #554, I'm gonna merge it directly. Thanks @mirchaemanuel |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #554.
Summary
($a[i][j] ?? 'dflt')must select the default when the accessed slot is statically typedStrand the read misses, while a present empty string keeps''. This PR adds the regression coverage the issue's checklist calls for. The underlying behavior is already correct onmain— it was fixed as a side effect of #533 — so this is a test-only change that locks the behavior in against silent regression and lets #554 be closed.Root cause (resolved by #533)
??lowers its left-hand access silently (ArrayGetSilent) and then tests the result withIsNull. Before #533,emit_is_null_resultlet statically-knownPhpType::Strvalues fall through to constant-false, so a missed string slot — whose fallback is{ptr = NULL_SENTINEL, len = 0}— was indistinguishable from a real empty string, and every miss yielded''instead of'dflt'.#533 closed the gap by routing
PhpType::Strthroughemit_string_null_bool, which compares the string pointer againstNULL_SENTINEL. This is sound because legitimate empty strings never carry that pointer: they use a null (0) or valid data pointer regardless of storage class (literal, persistent, scratch/concat, heap zero-length). Only the miss fallback stamps the sentinel, and it is consumed by the coalesce branch — never materialized as a usable string.Design rationale vs the two sketched directions
The issue sketched (a) giving
Stra global explicit null representation, or (b) preserving missingness through the silent-access/coalesce lowering. #533 realized the minimal form of (b): the miss information is carried locally in the string pointer (the sentinel already flowed there for the crash guards) and consumed at theIsNulldecision, with no change to string storage classes or ownership contracts — avoiding the broad audit the issue feared.Test plan (maps to the issue's regression checklist)
$a[7][1] ?? 'dflt'→'dflt'silently — ✅$a[1][7] ?? 'dflt'→'dflt'silently — ✅''— ✅??— ✅--ir-opt=onand--ir-opt=off— ✅ (both pass)??path stays silent; fix(codegen): guard container consumers against the null-container sentinel (chained-read miss segfault) #533's null-container crash guards and warning behavior preserved — ✅Tests fail if #533's
PhpType::Strarm is reverted to constant-false (reproducing the''-instead-of-'dflt'symptom) and pass as shipped. Indices are bound to$argc-derived locals so the element read stays staticallyStr(an inline arithmetic index is typedMixedand would route through the boxed-Mixed null path instead).https://claude.ai/code/session_01HRvbcjPHa3kAojdCjNZL5L