Fix Array.prototype.slice for high-index array-like values#411
Merged
Conversation
- Preserve slices that start above the 32-bit index range - Add tests for sparse array-like objects with lengths over 2**31
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthrough
Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
the
spec compliance
![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: 1
🤖 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 1725-1742: The current branch decision only uses RawK > MaxInt and
misses cases where StartIndex + Needed overflows the 32-bit Integer range;
update the branching so the 64-bit path (using View.HasIndex64 / View.Get64 /
ArrayCreateDataProperty with 64-bit indices) is chosen whenever RawK > MaxInt OR
the computed end index would exceed MaxInt (e.g. Int64(Trunc(RawK)) + Needed >
MaxInt) or when EndIndex would be greater than View.Len; compute the end as a
64-bit value to check bounds safely and, if it crosses the 32-bit boundary,
iterate via the 64-bit loop (mirroring the RawK > MaxInt branch) to avoid
integer overflow and missing high indices while still clamping against View.Len.
🪄 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: 60939614-c2b7-4008-8244-7ed8ab0129fa
📒 Files selected for processing (2)
source/units/Goccia.Values.ArrayValue.pastests/built-ins/Array/prototype/slice.js
Contributor
Benchmark Results386 benchmarks Interpreted: 🟢 12 improved · 🔴 350 regressed · 24 unchanged · avg -8.1% arraybuffer.js — Interp: 🔴 13, 1 unch. · avg -8.3% · Bytecode: 🔴 12, 2 unch. · avg -11.0%
arrays.js — Interp: 🔴 17, 2 unch. · avg -9.7% · Bytecode: 🔴 19 · avg -10.7%
async-await.js — Interp: 🔴 5, 1 unch. · avg -10.0% · Bytecode: 🔴 5, 1 unch. · avg -8.6%
base64.js — Interp: 🔴 9, 1 unch. · avg -11.0% · Bytecode: 🔴 10 · avg -9.7%
classes.js — Interp: 🔴 24, 7 unch. · avg -7.6% · Bytecode: 🔴 15, 16 unch. · avg -4.3%
closures.js — Interp: 🔴 11 · avg -11.0% · Bytecode: 🔴 11 · avg -9.6%
collections.js — Interp: 🔴 12 · avg -10.4% · Bytecode: 🔴 11, 1 unch. · avg -8.1%
csv.js — Interp: 🔴 13 · avg -10.5% · Bytecode: 🔴 13 · avg -9.3%
destructuring.js — Interp: 🔴 22 · avg -8.5% · Bytecode: 🔴 19, 3 unch. · avg -9.0%
fibonacci.js — Interp: 🔴 8 · avg -9.8% · Bytecode: 🔴 8 · avg -12.9%
float16array.js — Interp: 🟢 4, 🔴 26, 2 unch. · avg -3.8% · Bytecode: 🟢 6, 🔴 22, 4 unch. · avg -3.3%
for-of.js — Interp: 🔴 7 · avg -8.8% · Bytecode: 🔴 7 · avg -14.4%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🔴 39, 3 unch. · avg -6.6% · Bytecode: 🔴 39, 3 unch. · avg -10.1%
json.js — Interp: 🔴 20 · avg -10.8% · Bytecode: 🔴 19, 1 unch. · avg -7.7%
jsx.jsx — Interp: 🔴 21 · avg -10.2% · Bytecode: 🔴 21 · avg -7.5%
modules.js — Interp: 🔴 9 · avg -9.5% · Bytecode: 🔴 9 · avg -16.2%
numbers.js — Interp: 🔴 11 · avg -10.7% · Bytecode: 🔴 11 · avg -11.7%
objects.js — Interp: 🔴 7 · avg -8.3% · Bytecode: 🔴 7 · avg -7.8%
promises.js — Interp: 🔴 12 · avg -8.3% · Bytecode: 🔴 11, 1 unch. · avg -7.2%
regexp.js — Interp: 🔴 11 · avg -14.0% · Bytecode: 🔴 11 · avg -9.4%
strings.js — Interp: 🔴 18, 1 unch. · avg -15.7% · Bytecode: 🟢 1, 🔴 18 · avg -6.6%
tsv.js — Interp: 🔴 9 · avg -14.1% · Bytecode: 🔴 9 · avg -8.0%
typed-arrays.js — Interp: 🟢 3, 🔴 17, 2 unch. · avg -3.7% · Bytecode: 🟢 4, 🔴 14, 4 unch. · avg -4.3%
uint8array-encoding.js — Interp: 🟢 5, 🔴 9, 4 unch. · avg +7.6% · Bytecode: 🟢 9, 🔴 5, 4 unch. · avg +19.7%
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. |
The previous guard (RawK > MaxInt) missed the case where start equals MaxInt and the window crosses past it, causing an Integer overflow in StartIndex + Needed with overflow checks enabled. Branching on RawFinal instead catches all windows that touch indices beyond Integer range. Also derives EndIndex from RawFinal directly to avoid the addition. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/built-ins/Array/prototype/slice.js (1)
176-183: Test name is misleading — no negative end index is used.The test name says "high negative end" but the test uses positive indices (
highandhigh + 2). Consider renaming to reflect the actual scenario being tested.📝 Suggested name fix
-test("slice on generic array-like with high negative end", () => { +test("slice on generic array-like with multiple high indices", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Array/prototype/slice.js` around lines 176 - 183, The test title is misleading — it references a "high negative end" while the test actually calls Array.prototype.slice with large positive indices (variables high and high + 2); update the test description string in the test(...) call to accurately reflect the scenario (e.g., "slice on generic array-like with high positive indices" or "slice on generic array-like with large positive start/end") so the name matches the use of Array.prototype.slice and the variables high and high + 1.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/built-ins/Array/prototype/slice.js`:
- Around line 176-183: The test title is misleading — it references a "high
negative end" while the test actually calls Array.prototype.slice with large
positive indices (variables high and high + 2); update the test description
string in the test(...) call to accurately reflect the scenario (e.g., "slice on
generic array-like with high positive indices" or "slice on generic array-like
with large positive start/end") so the name matches the use of
Array.prototype.slice and the variables high and high + 1.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2f32b45c-d5f2-43e6-a5e3-6aabe0f5fbbc
📒 Files selected for processing (2)
source/units/Goccia.Values.ArrayValue.pastests/built-ins/Array/prototype/slice.js
- Clarify the test name for large positive start and end indices
- Cover `Array.prototype.slice` on generic array-like objects above `2**31` - Verify negative start and end positions preserve tail elements
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
Array.prototype.sliceso generic array-like objects with indices above2**31are handled without collapsing the slice window.MaxInt.Fixes #409