Skip to content

fix: functional-correctness batch 1 (semver.sort, immer hasOwnProperty, ctor return-override, jwt crypto keys)#5386

Merged
proggeramlug merged 4 commits into
mainfrom
fix/functional-correctness-batch1
Jun 18, 2026
Merged

fix: functional-correctness batch 1 (semver.sort, immer hasOwnProperty, ctor return-override, jwt crypto keys)#5386
proggeramlug merged 4 commits into
mainfrom
fix/functional-correctness-batch1

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Functional-correctness batch 1

A new differential harness (secret-tests/corpus/functional/) byte-diffs real package APIs between node --experimental-strip-types and perry-source-compiled drivers. This PR fixes four DISTINCT functional-correctness roots surfaced by it, each with a regression test. The packages whose root I fixed advance past their original failure (some then hit further independent bugs, noted below).

Fixed (root cause per bug)

  1. semver.sortarray_only_methods.rs / imported_array_methods.rs (+ local_array_methods.rs) unconditionally folded a .sort(list) method call into the Expr::ArraySort intrinsic even when the receiver was a class instance or an imported module-exports object. semver re-exports sort = (list) => list.sort(cmp) and the driver calls semver.sort(list); the fold mis-routed the single list arg into the comparator slot → "The comparison function must be either a function or undefined". Fix: gate the sort fold on a real-array receiver (mirrors the existing map/filter/join !recv_is_class / Array-return-type guards). semver now sorts correctly; its driver still DIFFs on a separate satisfies/maxSatisfying Range-test bug (not addressed).

  2. immer O.hasOwnProperty.call(...)Object is a function, so member reads resolve up its prototype chain: Object.hasOwnProperty IS Object.prototype.hasOwnProperty (callable). The reified Object constructor value only had its static methods installed, so const O = Object; O.hasOwnProperty returned undefined.call(...) threw "Function.prototype.call on non-function". Fix: install the inherited Object.prototype methods (hasOwnProperty / isPrototypeOf / propertyIsEnumerable / toString / toLocaleString / valueOf) on the reified ctor, reusing the existing object_prototype_*_thunk impls. immer's hasOwnProperty path fixed; its driver then hits a separate reading 'slice' bug.

  3. chalk constructor return-override (codegen, new path) — ECMAScript: a constructor that explicitly returns an object/function makes new C() evaluate to that value. The default new codegen calls a shared standalone <class>_constructor symbol and DISCARDED its return value, always yielding the empty default instance. chalk's class Chalk { constructor(o){ return chalkFactory(o); } } produced the default instance instead of the factory function. Fix: capture the standalone ctor's return value and run it through js_ctor_return_override (the same helper the inline path already used; undefined/primitive maps back to this, so ordinary ctors are unaffected). Verified by fixtures (object- and function-returning ctors); chalk itself still needs further prototype-getter machinery, so its driver still DIFFs.

  4. jsonwebtoken createPrivateKey/createPublicKey (crypto) — Node throws on input that is not valid key material; perry returned the raw string as a type: undefined key. jsonwebtoken does try { createPrivateKey(secret) } catch { createSecretKey(...) }, so a string HMAC secret must make those throw to reach the symmetric-key fallback — otherwise HS256 sign reported "secretOrPrivateKey must be a symmetric key" and verify reported "invalid algorithm". Fix: throw (ERR_OSSL_PEM_NO_START_LINE) unless the input classifies as a real key surrogate or carries a PEM -----BEGIN ... KEY----- header. Fixes the symmetric-key + invalid-algorithm errors; jwt then hits a further reading 'prototype' bug in the jws layer.

Characterized only (not fixed)

  • zodz.string()core._string(ZodString, …)new Class(…) throws "undefined is not a constructor" because ZodString (a const = core.$constructor("ZodString", …)) is undefined at the call site. A module-init-order / ≥300-const-init scale issue (known hard zod blocker class, cf. perry-codegen: Effect — (number).slice is not a function during Schema.ts__init (#680 follow-up, ~310th init) #684/tests: harness for dynamic class extension / mixins #806). Isolated fixtures of the $constructor/new Cls(param) pattern work; the failure is the cross-module const value being undefined.
  • joiinternals.generate reads schema._definition.args where schema._definition is undefined: the type schema object's _definition field isn't initialized. Deep joi internals.
  • markedmarked.parse(md) returns an opaque object (no keys, no ctor, not a Promise) instead of an HTML string, through marked's nested class-field-closure parse chain. Not localized to a single root.
  • lodash_.get(obj, "a.b.c") throws "reading 'size'" inside lodash's MapCache.set (getMapData(this, key) returns undefined). Faithful standalone MapCache fixtures work, so the trigger is more subtle (likely the cached-native-Map/ListCache selection).
  • minimatchnew Minimatch(p, o) instances have no match method and no set field (prototype methods / this.make()-set fields missing). Class-method/field resolution gap for this cross-module class.
  • cheeriocompile-time stack overflow in the "Generating code" (codegen) phase over cheerio's 222-module graph, on a rayon worker thread (RUST_MIN_STACK ineffective). Unbounded recursion in a codegen lowering/type pass; specific trigger construct not yet isolated.
  • ajv — SIGSEGV at runtime (not investigated in depth this round).
  • chalk-template — shares chalk's root; needs chalk's prototype-getter chain complete first.

Results

  • functional harness OK(==node): 25 → 25 (the four fixed packages each advance past their original root but hit further independent bugs, so their driver lines don't yet fully match node end-to-end — no functional driver newly turns fully green, but four genuine correctness roots are eliminated and four packages' first-failure moves materially downstream). No functional regressions.
  • import-smoke corpus: 59/59 (unchanged).
  • cargo test perry-hir / perry-codegen / perry-stdlib(crypto): all green. fmt/file-size/gc-inventory/addr-class lints clean.
  • 4 new tests/test_*.sh regression tests.

Version / changelog

Per the batch instructions, this PR does not bump Cargo.toml/CLAUDE.md version or touch CHANGELOG.md — maintainer folds metadata at merge.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed constructor return override behavior to properly apply ECMAScript semantics for classes with explicit return values.
    • Resolved incorrect lowering of user-defined sort methods into array intrinsics, preventing argument mis-routing.
    • Enabled access to Object.prototype methods (hasOwnProperty, isPrototypeOf, etc.) via the Object constructor.
    • Improved error handling in crypto key creation functions to throw proper exceptions for invalid input.

Ralph Küpper added 4 commits June 18, 2026 08:32
A method named `sort` on a class instance or an imported module-exports
object (semver re-exports `sort = (list) => list.sort(cmp)`, called as
`semver.sort(list)`) was unconditionally folded into `Expr::ArraySort`,
mis-routing the single `list` argument into the comparator slot. The
runtime comparator validator then saw the array (not a function) and threw
"The comparison function must be either a function or undefined: ...".

Gate the `sort` fold on the receiver being a known array, mirroring the
existing map/filter/join guards:
- array_only_methods.rs: require !recv_is_class for the `sort` arm, and
  bail for imported-binding receivers (treated like namespaces).
- imported_array_methods.rs: only fold when the imported binding's
  return_type is statically Array (mirrors the #420 `join` fix).
- local_array_methods.rs: skip the fold for known class-instance receivers.
… constructor

`Object` is a function whose member reads resolve up the prototype chain, so
`Object.hasOwnProperty` IS `Object.prototype.hasOwnProperty` (a callable).
immer's `isPlainObject` does `O.hasOwnProperty.call(proto, CONSTRUCTOR)` with
`const O = Object`; the reified Object constructor value had only its static
methods installed, so reading `hasOwnProperty` returned `undefined` and `.call`
threw "Function.prototype.call was called on a value that is not a function".

Install the Object.prototype methods reachable on the constructor by
inheritance (hasOwnProperty / isPrototypeOf / propertyIsEnumerable / toString /
toLocaleString / valueOf) as statics on the reified ctor, reusing the existing
`object_prototype_*_thunk` impls (which read the receiver from IMPLICIT_THIS,
so `.call(receiver, ...)` binds correctly).
…bol new path

ECMAScript: a constructor that explicitly returns an object (or function)
makes `new C()` evaluate to that value, not the freshly-allocated `this`. The
default codegen path calls a shared standalone `<class>_constructor` symbol
(the inline path is opt-in via PERRY_INLINE_CTOR=1); that path discarded the
ctor's return value and always returned `obj_box`, so a constructor like
chalk's `class Chalk { constructor(o){ return chalkFactory(o); } }`
(no-constructor-return) produced the empty default instance instead of the
returned factory function.

call_local_constructor_symbol now returns the standalone ctor's call result;
the new site feeds it through js_ctor_return_override (the same helper the
inline path already used). The standalone ctor returns `undefined` for an
ordinary body, and js_ctor_return_override maps `undefined`/primitive back to
`obj_box`, so ordinary constructors are unaffected (verified: import-smoke
59/59, functional OK count unchanged).
…terial

Node throws when createPrivateKey/createPublicKey receive input that is not
valid key material (e.g. a plain non-PEM string) instead of producing a
KeyObject with `type === undefined`. perry returned the raw string as a key,
so its `.type` was undefined. jsonwebtoken depends on the throw:
sign()/verify() do `try { createPrivateKey(secret) } catch { createSecretKey(...) }`,
so a string HMAC secret must make createPrivateKey/createPublicKey throw to
reach the symmetric-key fallback — otherwise HS256 signing reported
"secretOrPrivateKey must be a symmetric key" and verify reported "invalid
algorithm".

js_crypto_create_private_key_value / js_crypto_create_public_key_value now
throw (ERR_OSSL_PEM_NO_START_LINE) unless the input classifies as a real
key surrogate or carries a PEM `-----BEGIN ... KEY-----` header. The
legitimate createSecretKey(Buffer) path is unaffected (still type 'secret').
No functional/import-smoke regressions.
@coderabbitai

coderabbitai Bot commented Jun 18, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

Four independent correctness fixes: (1) the standalone <class>_constructor codegen path now propagates the raw constructor return value and applies ECMAScript return-override semantics via js_ctor_return_override; (2) three HIR lowering passes are guarded against mis-folding user-defined or re-exported sort methods into Expr::ArraySort; (3) six Object.prototype methods are installed as callable bindings on the reified Object constructor value; (4) createPrivateKey/createPublicKey now throw Node-style errors on invalid PEM input. Each fix is accompanied by a new integration test.

Changes

Constructor Return Override (Standalone Path)

Layer / File(s) Summary
Return propagation and override semantics in lower_new
crates/perry-codegen/src/lower_call/new.rs
call_local_constructor_symbol is changed to return Option<String>, and lower_new uses the captured value with js_ctor_return_override (plus derived-class detection) instead of unconditionally returning obj_box.
Integration test for standalone constructor return override
tests/test_ctor_return_override_standalone.sh
New Bash test runs a TypeScript snippet with constructors that explicitly return objects/functions, asserting the standalone codegen path honors new return override.

ArraySort Mis-folding Fixes

Layer / File(s) Summary
receiver_is_class_instance helper and local sort guard
crates/perry-hir/src/lower/expr_call/local_array_methods.rs
Introduces receiver_is_class_instance (excluding TypedArray/Array generics) and threads it into the local .sort fast-path to prevent argument mis-routing.
Return-type guard in imported array sort folding
crates/perry-hir/src/lower/expr_call/imported_array_methods.rs
The imported "sort" branch now folds to Expr::ArraySort only when the binding's static return_type is Type::Array(_); non-array-typed bindings fall through to generic call dispatch.
Imported-function bailout and recv_is_class guard in array-only dispatch
crates/perry-hir/src/lower/expr_call/array_only_methods.rs
Adds an early Err(args) return for Ident receivers that are imported function bindings without a local binding; tightens the "sort" match arm with !recv_is_class.
End-to-end test for user-defined sort method
tests/test_user_method_named_sort.sh
New Bash test verifies a class sort method delegates to list.sort(comparator) with correct runtime output and no incorrect Expr::ArraySort folding.

Object Constructor Prototype Method Reification

Layer / File(s) Summary
Install Object.prototype methods on reified Object constructor
crates/perry-runtime/src/object/global_this.rs
The Object branch in install_builtin_constructor_statics adds prototype-thunk bindings for hasOwnProperty, isPrototypeOf, propertyIsEnumerable, toString, toLocaleString, and valueOf.
Integration test for Object constructor inherited prototype methods
tests/test_object_ctor_inherited_proto_methods.sh
New Bash test asserts hasOwnProperty and related methods are callable on the Object constructor value and that Object.hasOwnProperty.call(...) works against a prototype chain result.

Crypto Key Creation Error Throwing

Layer / File(s) Summary
Error throwing in createPrivateKey and createPublicKey
crates/perry-stdlib/src/crypto/sign.rs
Both key-creation functions throw ERR_INVALID_ARG_TYPE when PEM material cannot be derived and ERR_OSSL_PEM_NO_START_LINE when the PEM header is unrecognized, replacing silent null returns.
Integration test for invalid key input throwing
tests/test_crypto_create_key_invalid_throws.sh
New Bash test exercises createPrivateKey and createPublicKey with non-PEM symmetric key material and asserts both throw while createSecretKey still returns type === "secret".

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • PerryTS/perry#5194: Both PRs modify the same HIR array-method lowering logic in array_only_methods.rs and receiver classification to prevent incorrect folding into Expr::ArraySort under non-array receivers.
  • PerryTS/perry#5304: Both PRs modify crates/perry-codegen/src/lower_call/new.rs around the standalone <Class>_constructor symbol path in lower_new, with this PR extending that path to enforce constructor return-override semantics.

Poem

🐇 Four bugs once lurked in the warren so deep,
The sort method folded where it shouldn't creep,
Object.prototype methods hid from view,
Crypto threw null when errors were due,
And new ignored the value you'd return—
Now each is fixed, the rabbit's hard-won earn! 🥕

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title directly summarizes the four functional-correctness fixes (semver.sort, immer hasOwnProperty, chalk constructor return-override, JWT crypto keys) and accurately reflects the main substance of the changeset.
Description check ✅ Passed The description comprehensively documents all four fixes with root causes, implementation details, test results, and adherence to version/changelog guidelines. All required template sections are present and complete.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/functional-correctness-batch1

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant