Fix property redefinition rules for objects and arrays#381
Merged
Conversation
- validate non-configurable descriptors before redefining - handle array length and indexed defineProperty updates - add coverage for Object.defineProperty and Reflect.defineProperty
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughImplements ES spec-compliant property descriptor validation for non-configurable property redefinitions. Adds array-specific Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/built-ins/Object/defineProperty-array.js (1)
1-61: Good test coverage for array-specific defineProperty behavior.The tests comprehensively cover:
- Length truncation, extension, and zeroing
- Numeric index updates within and beyond current bounds
- Non-index property handling
Consider adding edge-case tests for invalid length values (NaN, Infinity, non-integer, negative) to validate proper RangeError handling.
Optional: Add edge-case tests for invalid lengths
test("defineProperty on array length with NaN throws", () => { const arr = [1, 2, 3]; expect(() => { Object.defineProperty(arr, "length", { value: NaN }); }).toThrow(); }); test("defineProperty on array length with negative throws", () => { const arr = [1, 2, 3]; expect(() => { Object.defineProperty(arr, "length", { value: -1 }); }).toThrow(); }); test("defineProperty on array length with non-integer throws", () => { const arr = [1, 2, 3]; expect(() => { Object.defineProperty(arr, "length", { value: 1.5 }); }).toThrow(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Object/defineProperty-array.js` around lines 1 - 61, Add edge-case tests to validate that Object.defineProperty on an array "length" correctly throws RangeError for invalid length values; specifically add tests using Object.defineProperty(arr, "length", { value: NaN }), value: Infinity, value: -1, and a non-integer like 1.5 (or similar) and assert they throw (or throw RangeError) to complement existing tests in tests/built-ins/Object/defineProperty-array.js; reference the existing tests by pattern/name (e.g., the "defineProperty on array length ..." tests) and ensure each new test follows the same test(...) structure and expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/units/Goccia.Values.ArrayValue.pas`:
- Around line 1783-1790: The current TryDefineProperty branch that handles
TGocciaPropertyDescriptorData only checks NewLen < 0; update it to mirror the
constructor's ES2026 §10.4.2.4 validation: obtain the numeric Len from
TGocciaPropertyDescriptorData(ADescriptor).Value.ToNumberLiteral.Value into
NewLen, then reject and return False if NewLen is NaN or infinite, if NewLen <>
Trunc(NewLen) (non-integer), or if NewLen > 4294967295 (overflow); when
rejecting free ADescriptor and Exit(False) as already done for negative values
so the behavior matches the constructor's validation.
In `@source/units/Goccia.Values.ObjectValue.pas`:
- Around line 716-717: The branch that handles adding a property when the object
is not extensible uses the wrong error token; update the ThrowTypeError call in
the block checking FExtensible to use SErrorCannotAddPropertyNotExtensible
instead of SErrorCannotRedefineNonConfigurable (keep the same Format([AName])
and suggestion SSuggestCannotDeleteNonConfigurable). Locate the code path where
FExtensible is tested (the else if not FExtensible branch) and replace the error
identifier so the thrown error correctly indicates adding a property to a
non‑extensible object.
- Around line 981-982: The error raised when adding a new symbol property on a
non-extensible object uses the wrong message constant; in the else branch that
checks FExtensible and calls
ThrowTypeError(Format(SErrorCannotRedefineNonConfigurable,
[ASymbol.ToDisplayString.Value]), SSuggestCannotDeleteNonConfigurable) replace
the message constant with the symbol-specific one (e.g.,
SErrorCannotAddSymbolNotExtensible) and adjust the suggestion constant if needed
so the thrown error refers to adding a symbol on a non-extensible object (refer
to FExtensible, ThrowTypeError, ASymbol.ToDisplayString.Value,
SErrorCannotRedefineNonConfigurable).
---
Nitpick comments:
In `@tests/built-ins/Object/defineProperty-array.js`:
- Around line 1-61: Add edge-case tests to validate that Object.defineProperty
on an array "length" correctly throws RangeError for invalid length values;
specifically add tests using Object.defineProperty(arr, "length", { value: NaN
}), value: Infinity, value: -1, and a non-integer like 1.5 (or similar) and
assert they throw (or throw RangeError) to complement existing tests in
tests/built-ins/Object/defineProperty-array.js; reference the existing tests by
pattern/name (e.g., the "defineProperty on array length ..." tests) and ensure
each new test follows the same test(...) structure and expectations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: be76dd75-0b9a-4e61-9cc0-ef875b7a80d5
📒 Files selected for processing (4)
source/units/Goccia.Values.ArrayValue.passource/units/Goccia.Values.ObjectValue.pastests/built-ins/Object/defineProperty-array.jstests/built-ins/Object/defineProperty-redefinition.js
Contributor
Benchmark Results386 benchmarks Interpreted: 🟢 15 improved · 🔴 324 regressed · 47 unchanged · avg -7.2% arraybuffer.js — Interp: 🔴 10, 4 unch. · avg -8.0% · Bytecode: 🔴 2, 12 unch. · avg +0.3%
arrays.js — Interp: 🔴 18, 1 unch. · avg -11.6% · Bytecode: 🟢 3, 🔴 3, 13 unch. · avg +1.0%
async-await.js — Interp: 🔴 5, 1 unch. · avg -12.2% · Bytecode: 🟢 1, 🔴 1, 4 unch. · avg -0.3%
base64.js — Interp: 🔴 10 · avg -11.1% · Bytecode: 🟢 1, 🔴 1, 8 unch. · avg -0.6%
classes.js — Interp: 🔴 29, 2 unch. · avg -7.5% · Bytecode: 🟢 2, 🔴 1, 28 unch. · avg +0.3%
closures.js — Interp: 🔴 10, 1 unch. · avg -9.0% · Bytecode: 11 unch. · avg -0.6%
collections.js — Interp: 🔴 12 · avg -10.9% · Bytecode: 🟢 1, 🔴 3, 8 unch. · avg -0.6%
csv.js — Interp: 🔴 13 · avg -9.7% · Bytecode: 🟢 1, 🔴 2, 10 unch. · avg -0.8%
destructuring.js — Interp: 🔴 22 · avg -8.0% · Bytecode: 🔴 3, 19 unch. · avg -0.2%
fibonacci.js — Interp: 🔴 8 · avg -9.7% · Bytecode: 🟢 3, 5 unch. · avg +1.2%
float16array.js — Interp: 🟢 4, 🔴 20, 8 unch. · avg -3.9% · Bytecode: 🟢 15, 🔴 3, 14 unch. · avg +2.8%
for-of.js — Interp: 🔴 5, 2 unch. · avg -5.6% · Bytecode: 🟢 3, 4 unch. · avg +2.2%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🔴 35, 7 unch. · avg -6.5% · Bytecode: 🟢 6, 🔴 4, 32 unch. · avg +0.1%
json.js — Interp: 🔴 19, 1 unch. · avg -8.7% · Bytecode: 🟢 3, 🔴 7, 10 unch. · avg -0.7%
jsx.jsx — Interp: 🔴 19, 2 unch. · avg -7.4% · Bytecode: 🟢 6, 🔴 1, 14 unch. · avg +0.6%
modules.js — Interp: 🔴 9 · avg -9.4% · Bytecode: 🟢 4, 5 unch. · avg +2.5%
numbers.js — Interp: 🔴 11 · avg -12.8% · Bytecode: 🟢 3, 🔴 2, 6 unch. · avg +0.8%
objects.js — Interp: 🔴 3, 4 unch. · avg -2.0% · Bytecode: 🟢 1, 🔴 1, 5 unch. · avg +1.6%
promises.js — Interp: 🔴 12 · avg -7.9% · Bytecode: 🟢 1, 🔴 5, 6 unch. · avg -2.0%
regexp.js — Interp: 🔴 10, 1 unch. · avg -6.5% · Bytecode: 🟢 5, 6 unch. · avg +1.2%
strings.js — Interp: 🟢 6, 🔴 11, 2 unch. · avg +9.0% · Bytecode: 🟢 6, 🔴 10, 3 unch. · avg -9.4%
tsv.js — Interp: 🔴 6, 3 unch. · avg -6.7% · Bytecode: 🟢 1, 8 unch. · avg +0.2%
typed-arrays.js — Interp: 🟢 2, 🔴 17, 3 unch. · avg -5.3% · Bytecode: 🟢 9, 🔴 3, 10 unch. · avg +1.1%
uint8array-encoding.js — Interp: 🟢 3, 🔴 10, 5 unch. · avg -13.2% · Bytecode: 🟢 10, 8 unch. · avg +18.9%
Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
- Fix VM FinalizeEnumValue to initialize symbols before PreventExtensions (matches interpreted mode order, fixes 30 bytecode enum test failures) - Use correct error messages for non-extensible objects in DefineProperty (SErrorCannotAddPropertyNotExtensible / SErrorCannotAddSymbolNotExtensible) - Add array length validation for NaN, non-integer, and overflow values - Consolidate tests into single defineProperty.js file Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 30, 2026
4 tasks
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
1 participant
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.
Summary
Object.definePropertyandReflect.definePropertyhandling for non-configurable properties to follow ES2026 compatibility rules.definePropertybehavior forlengthand numeric indices, including truncation, विस्तार, and hole filling.Reflect.definePropertyboolean results.Fixes #376