diff --git a/changelog.d/7891-declared-array-claim-string-key.md b/changelog.d/7891-declared-array-claim-string-key.md new file mode 100644 index 0000000000..2979d4ddca --- /dev/null +++ b/changelog.d/7891-declared-array-claim-string-key.md @@ -0,0 +1,63 @@ +### fix(codegen): keep a string or symbol key off #7890's declared-array claim, and cover the shape #7890 added + +Two follow-ups to #7890, both found by writing the coverage #7890 was missing. + +#### A. A string/symbol key must not ride the claim (#7891) + +#7890 lets a property read whose receiver's *declared* property type is an array +(`e.vals[i]`, `p.toks[p.pos]`) reach `expr/index_get.rs`'s array arm. That is a +CLAIM, not a proof, and it was admitted on the grounds that the array arm +re-checks `GC_TYPE_ARRAY` on the receiver and falls back. + +That is true of the arm **as a whole** and false of one route inside it. The two +key routes have different receiver-validation strength: + +* **numeric** — `js_array_get_f64`, which classifies the receiver through + `clean_arr_ptr` / `array_object_receiver` and answers correctly for a string, + an array-like object, a typed array or a number. +* **static string / symbol** — `js_array_get_index_or_string` → + `array_get_property_by_key` → `js_object_get_field_by_name`, which has **no + string-receiver index arm** and answers `undefined` for `s["0"]` where JS + answers the character. + +So only the numeric route is claim-safe. The claim now requires a non-string, +non-symbol key; a string or symbol key keeps exactly the generic path it had +before #7890. `interp`'s and `iso_miss`'s reads are all numeric, so the measured +result is unchanged — every one of the 19 corpus binaries is byte-identical to +the ones timed for #7890. + +The `undefined` answer itself is **pre-existing on `main`** and reachable without +any of this, through a plain non-union declared receiver: + +```ts +type Bag = { items: string[] }; +function mk(v: any): Bag { return { items: v }; } +function viaDeclared(b: Bag): string { return "" + b.items["0"] + "/" + b.items[0]; } +const s: any = "ss"; +console.log(viaDeclared(mk(s))); // node: s/s perry: undefined/s +const direct: any = "ss"; +console.log("" + direct["0"] + "/" + direct[0]); // node: s/s perry: s/s +``` + +The same read on a bare `any` is correct, which is the tell: the wrong answer is +selected by the ANNOTATION, not by the value. Tracked as **#7891**; not checked in +as a gap test, because it would be red by construction and +`test-parity/gap_snapshot.json` is generated on Linux and must not be hand-edited. + +#### B. Coverage for the shape #7890 actually added + +`test-files/test_gap_7890_declared_array_receiver_element_read.ts`. #7854's own +test always routes through an intermediate local (`const items = e.items`), so +nothing covered a `PropertyGet` used **directly** as the receiver — which is +exactly what #7890 added. The new file reads `e.items[i]` / `e.items.length` +through a `type` alias, an `interface`, a class, a nullable reassigned cursor and +a nested chain, handed an array, a string, a number, an array-like object with +numeric and with non-numeric `length`, a typed array, a function, `null` and +`undefined`, plus negative / fractional / out-of-range indexes, a store through +the same shape, and static string keys (which A leaves on the generic path). +Byte-identical to node on every row. + +Live rather than decorative: on that file the guarded-read `arr.fast` blocks go +**11 → 15** and the `js_dyn_index_get` calls go **5 → 1**. + +Writing it is what found #7891. diff --git a/crates/perry-codegen/src/expr/index_get.rs b/crates/perry-codegen/src/expr/index_get.rs index 5986cfbbed..05a2f8321d 100644 --- a/crates/perry-codegen/src/expr/index_get.rs +++ b/crates/perry-codegen/src/expr/index_get.rs @@ -1126,6 +1126,17 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { recv_ty, None | Some(perry_hir::types::Type::Any) | Some(perry_hir::types::Type::Unknown) ); + // #5525: route every non-static-string/symbol read on an unknown + // receiver through `js_dyn_index_get` (numeric, runtime-string, and + // runtime-symbol are all triaged in the runtime). The earlier + // `is_numeric_expr(index)` gate missed `lr[off]`/`lr[off + 1]` + // (bcryptjs `_encipher`'s `off` is an `any` param, so `off + 1` is + // not provably numeric); statically-known string-literal / symbol + // keys keep their dedicated interned-handle / symbol routes below. + let index_is_static_string_or_symbol = matches!( + index.as_ref(), + Expr::String(_) | Expr::WtfString(_) | Expr::SymbolFor(_) + ) || is_string_expr(ctx, index); // #7854 recovered a receiver's declared array type for a LOCAL // (`const names = e.names`), never for the read used directly as a // receiver (`e.vals[i]`, `p.toks[p.pos]`) — the HIR types a @@ -1141,20 +1152,22 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // fallback. A violated claim costs a branch, not an answer. (#6132 // records that the same guard is what makes a typed-array-valued // member receiver safe on this path.) - let claimed_array = - recv_unknown && crate::type_analysis::declared_array_property_claim(ctx, object); + // + // Restricted to a NON-string, NON-symbol key, and that restriction is + // load-bearing rather than tidy. The string-key arm of the array + // branch is `js_array_get_index_or_string`, whose string half calls + // `js_object_get_field_by_name` on the receiver — and that answers + // `undefined` for `s["0"]` on a heap STRING receiver, where JS + // answers the character. That is a pre-existing wrong answer on + // `main` — reachable today through a plain non-union declared + // receiver, filed as #7891 with a minimal repro — and a claim must + // not widen the set of shapes that reach it. With + // the restriction, a string or symbol key takes exactly the generic + // path it takes today; only the numeric read moves. + let claimed_array = recv_unknown + && !index_is_static_string_or_symbol + && crate::type_analysis::declared_array_property_claim(ctx, object); let recv_unknown = recv_unknown && !claimed_array; - // #5525: route every non-static-string/symbol read on an unknown - // receiver through `js_dyn_index_get` (numeric, runtime-string, and - // runtime-symbol are all triaged in the runtime). The earlier - // `is_numeric_expr(index)` gate missed `lr[off]`/`lr[off + 1]` - // (bcryptjs `_encipher`'s `off` is an `any` param, so `off + 1` is - // not provably numeric); statically-known string-literal / symbol - // keys keep their dedicated interned-handle / symbol routes below. - let index_is_static_string_or_symbol = matches!( - index.as_ref(), - Expr::String(_) | Expr::WtfString(_) | Expr::SymbolFor(_) - ) || is_string_expr(ctx, index); if recv_unknown && !index_is_static_string_or_symbol { // #7640 section B: receiver live across an unconstrained index. return rooting::with_operands_rooted(ctx, &[object, index], |ctx, vals| { diff --git a/test-files/test_gap_7890_declared_array_receiver_element_read.ts b/test-files/test_gap_7890_declared_array_receiver_element_read.ts index 2a9380aa8f..7a6a8816e8 100644 --- a/test-files/test_gap_7890_declared_array_receiver_element_read.ts +++ b/test-files/test_gap_7890_declared_array_receiver_element_read.ts @@ -162,3 +162,15 @@ function scan(b: Bag, needle: string): string { console.log(scan(mkAlias(["a", "b"]), "a")); console.log(scan(mkAlias([1, 2, 3] as any), "a")); console.log(scan(mkAlias({ length: 3 }), "a")); + +// A STRING-literal key on the same receiver shape is deliberately NOT admitted +// to the array arm by #7890 — see `index_get.rs`. It stays on the generic path, +// which is what these rows pin. +function stringKey(b: Bag): string { + return ( + "" + b.items["length"] + "/" + b.items["nope"] + "/" + typeof b.items["constructor"] + ); +} +console.log(stringKey(mkAlias(["k"]))); +console.log(stringKey(mkAlias({ length: 2, 0: "obj0", nope: "here" }))); +console.log(stringKey(mkAlias(9)));