From 539842da0afdfde5eb2e08649344df64b0dcc791 Mon Sep 17 00:00:00 2001 From: Ralph Date: Fri, 19 Jun 2026 05:14:48 -0700 Subject: [PATCH] =?UTF-8?q?fix(hir):=20#5195=20=E2=80=94=20don't=20fold=20?= =?UTF-8?q?property-access=20`.indexOf`/`.includes`=20to=20ArrayIndexOf?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `lower/expr_call/array_only_methods.rs` folded a `PropertyGet` receiver of `.indexOf` / `.includes` to `Expr::ArrayIndexOf` / `Expr::ArrayIncludes` (an element-equality search) unless the property name was in a hardcoded `stack|message|name|sourceSQL|expandedSQL` allowlist. A genuine string property (`interface P { id: string }`, `arr[i].id.indexOf("PRO")`) fell through that allowlist and was searched as an array, silently returning -1 (and `.filter(p => p.id.indexOf("PRO") >= 0)`, the same predicate shape, returned 0 matches) with no error. Fix: only fold receivers that are *provably* array-producing (`.map`/`.filter`/`.sort`/`.slice`/`.split`/`Object.keys`/`Object.values`/ array literals/`Array.from`). Every property access now falls through to the generic `js_native_call_method` dispatch, which checks the runtime value type and routes string- vs. array-`indexOf`/`includes` correctly — the same way the sibling `slice` arm already behaved, and the same path a known-string local (`const pid = arr[i].id; pid.indexOf(...)`) already took. Genuine array-typed properties keep correct semantics through that dynamic dispatch. Removes the unsustainable property-name allowlist band-aid in both arms. Regression test: crates/perry/tests/issue_5195_property_method_chain.rs. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/lower/expr_call/array_only_methods.rs | 93 ++++++------ .../tests/issue_5195_property_method_chain.rs | 138 ++++++++++++++++++ 2 files changed, 184 insertions(+), 47 deletions(-) create mode 100644 crates/perry/tests/issue_5195_property_method_chain.rs diff --git a/crates/perry-hir/src/lower/expr_call/array_only_methods.rs b/crates/perry-hir/src/lower/expr_call/array_only_methods.rs index b0ce37dbb..455a42161 100644 --- a/crates/perry-hir/src/lower/expr_call/array_only_methods.rs +++ b/crates/perry-hir/src/lower/expr_call/array_only_methods.rs @@ -909,29 +909,32 @@ pub(super) fn try_array_only_methods( // Fall through to string/generic dispatch. } else { let array_expr = lower_expr(ctx, &member.obj)?; - let is_known_string_prop = matches!(&array_expr, - Expr::PropertyGet { property, .. } - if matches!( - property.as_str(), - "stack" | "message" | "name" | "sourceSQL" | "expandedSQL" - ) - ); - if !is_known_string_prop - && matches!( - &array_expr, - Expr::ArrayMap { .. } - | Expr::ArrayFilter { .. } - | Expr::ArraySort { .. } - | Expr::ArraySlice { .. } - | Expr::Array(_) - | Expr::ArrayFrom(_) - | Expr::ArrayFromArrayLikeHoley(_) - | Expr::StringSplit(_, _) - | Expr::ObjectKeys(_) - | Expr::ObjectValues(_) - | Expr::PropertyGet { .. } - ) - { + // #5195: a `PropertyGet` receiver + // (`arr[i].id.indexOf(...)`, `p.id.indexOf(...)`) is + // NOT necessarily an array — the property is very often + // a string. Folding it to `ArrayIndexOf` (an + // element-equality search) silently returned the wrong + // result for string-typed properties; the old hardcoded + // `stack|message|name|…` allowlist only patched a + // handful of property names. Only fold receivers that + // are *provably* array-producing; every other receiver + // (all property accesses included) falls through to the + // generic method dispatch, which checks the runtime + // value type and routes string- vs. array-`indexOf` + // correctly — exactly as the `slice` arm above does. + if matches!( + &array_expr, + Expr::ArrayMap { .. } + | Expr::ArrayFilter { .. } + | Expr::ArraySort { .. } + | Expr::ArraySlice { .. } + | Expr::Array(_) + | Expr::ArrayFrom(_) + | Expr::ArrayFromArrayLikeHoley(_) + | Expr::StringSplit(_, _) + | Expr::ObjectKeys(_) + | Expr::ObjectValues(_) + ) { let mut it = args.into_iter(); let value_expr = it.next().unwrap(); let from_index = it.next().map(Box::new); @@ -948,30 +951,26 @@ pub(super) fn try_array_only_methods( // Fall through to string/generic dispatch. } else { let array_expr = lower_expr(ctx, &member.obj)?; - // Don't treat known string-valued properties as arrays. - let is_known_string_prop = matches!(&array_expr, - Expr::PropertyGet { property, .. } - if matches!( - property.as_str(), - "stack" | "message" | "name" | "sourceSQL" | "expandedSQL" - ) - ); - if !is_known_string_prop - && matches!( - &array_expr, - Expr::ArrayMap { .. } - | Expr::ArrayFilter { .. } - | Expr::ArraySort { .. } - | Expr::ArraySlice { .. } - | Expr::Array(_) - | Expr::ArrayFrom(_) - | Expr::ArrayFromArrayLikeHoley(_) - | Expr::StringSplit(_, _) - | Expr::ObjectKeys(_) - | Expr::ObjectValues(_) - | Expr::PropertyGet { .. } - ) - { + // #5195: see the `indexOf` arm above — a `PropertyGet` + // receiver is not necessarily an array (the property is + // often a string), so folding it to `ArrayIncludes` + // silently returned `false` on string properties. Only + // fold provably array-producing receivers; property + // accesses fall through to the runtime-type-checked + // generic dispatch. + if matches!( + &array_expr, + Expr::ArrayMap { .. } + | Expr::ArrayFilter { .. } + | Expr::ArraySort { .. } + | Expr::ArraySlice { .. } + | Expr::Array(_) + | Expr::ArrayFrom(_) + | Expr::ArrayFromArrayLikeHoley(_) + | Expr::StringSplit(_, _) + | Expr::ObjectKeys(_) + | Expr::ObjectValues(_) + ) { let mut it = args.into_iter(); let value_expr = it.next().unwrap(); let from_index = it.next().map(Box::new); diff --git a/crates/perry/tests/issue_5195_property_method_chain.rs b/crates/perry/tests/issue_5195_property_method_chain.rs new file mode 100644 index 000000000..e17dab047 --- /dev/null +++ b/crates/perry/tests/issue_5195_property_method_chain.rs @@ -0,0 +1,138 @@ +//! Regression tests for #5195 — an inline property-access chained into an +//! array/string-overlapping method call (`arr[i].id.indexOf(x)`, +//! `p.id.includes(x)`) miscompiled and returned the wrong result. +//! +//! Root cause: `lower/expr_call/array_only_methods.rs` folded a `PropertyGet` +//! receiver of `.indexOf` / `.includes` to `ArrayIndexOf` / `ArrayIncludes` +//! (an element-equality search) unless the property name happened to be in a +//! hardcoded `stack|message|name|sourceSQL|expandedSQL` allowlist. A genuine +//! string property (`id: string`) fell through that allowlist and was searched +//! as an array, so `arr[i].id.indexOf("PRO")` returned -1 (and the equivalent +//! `.filter(p => p.id.indexOf("PRO") >= 0)` predicate returned 0 matches) with +//! no error — silently breaking a StoreKit product lookup downstream. +//! +//! The fix only folds receivers that are *provably* array-producing +//! (`.map`/`.filter`/`.split`/`Object.keys`/array literals/…). Every property +//! access now falls through to `js_native_call_method`, which checks the +//! runtime value type and routes string- vs. array-`indexOf` correctly — the +//! same way the `slice` arm already behaved. + +use std::path::PathBuf; +use std::process::Command; + +fn perry_bin() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_perry")) +} + +fn compile_and_run(dir: &std::path::Path, entry: &std::path::Path) -> String { + let output = dir.join("main_bin"); + 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).output().expect("run compiled binary"); + assert!( + run.status.success(), + "compiled binary failed\nstatus: {:?}\nstdout:\n{}\nstderr:\n{}", + run.status, + String::from_utf8_lossy(&run.stdout), + String::from_utf8_lossy(&run.stderr) + ); + String::from_utf8_lossy(&run.stdout).to_string() +} + +/// The exact issue shape: a string property read inline and immediately +/// `.indexOf`/`.includes`'d, both in a loop and inside a `.filter` predicate. +/// Pre-fix every count was 0; they must match the extract-to-local form. +#[test] +fn string_property_indexof_includes_inline_match() { + let dir = tempfile::tempdir().expect("tempdir"); + let entry = dir.path().join("main.ts"); + std::fs::write( + &entry, + r#" +interface P { id: string } +const arr: P[] = [{ id: "GSC_PRO_MONTHLY" }, { id: "GSC_AGENCY_MONTHLY" }, { id: "GSC_PRO_ANNUAL" }] + +let inlineIdx = 0 +for (let i = 0; i < arr.length; i++) { if (arr[i].id.indexOf("PRO") >= 0) inlineIdx++ } +console.log("inlineIdx=" + inlineIdx) + +let inlineInc = 0 +for (let i = 0; i < arr.length; i++) { if (arr[i].id.includes("PRO")) inlineInc++ } +console.log("inlineInc=" + inlineInc) + +console.log("filter=" + arr.filter(function (p: P) { return p.id.indexOf("PRO") >= 0 }).length) +console.log("chain=" + arr[0].id.toLowerCase().indexOf("pro")) +"#, + ) + .expect("write entry"); + + let stdout = compile_and_run(dir.path(), &entry); + assert!( + stdout.contains("inlineIdx=2"), + "arr[i].id.indexOf(\"PRO\") must find 2 (got: {stdout})" + ); + assert!( + stdout.contains("inlineInc=2"), + "arr[i].id.includes(\"PRO\") must find 2 (got: {stdout})" + ); + assert!( + stdout.contains("filter=2"), + "filter(p => p.id.indexOf(\"PRO\") >= 0) must keep 2 (got: {stdout})" + ); + assert!( + stdout.contains("chain=4"), + "arr[0].id.toLowerCase().indexOf(\"pro\") must be 4 (got: {stdout})" + ); +} + +/// The fix must NOT regress a genuinely array-typed property: `.indexOf` / +/// `.includes` on an array field still search the array (via the runtime-typed +/// generic dispatch) and return the element index / membership. +#[test] +fn array_typed_property_indexof_includes_still_search_array() { + let dir = tempfile::tempdir().expect("tempdir"); + let entry = dir.path().join("main.ts"); + std::fs::write( + &entry, + r#" +class Box { tags: string[] = ["red", "green", "blue"] } +const b = new Box() +console.log("idx=" + b.tags.indexOf("green")) +console.log("inc=" + b.tags.includes("blue")) +console.log("miss=" + b.tags.indexOf("pink")) +const cfg = { nums: [10, 20, 30] } +console.log("nidx=" + cfg.nums.indexOf(20)) +"#, + ) + .expect("write entry"); + + let stdout = compile_and_run(dir.path(), &entry); + assert!( + stdout.contains("idx=1"), + "array-property indexOf must return the element index (got: {stdout})" + ); + assert!( + stdout.contains("inc=true"), + "array-property includes must return true for a member (got: {stdout})" + ); + assert!( + stdout.contains("miss=-1"), + "array-property indexOf must return -1 for a non-member (got: {stdout})" + ); + assert!( + stdout.contains("nidx=1"), + "numeric array-property indexOf must return the index (got: {stdout})" + ); +}