Walkthrough

ArgConverter now recognizes Objective-C collections that respond to count and objectAtIndex: for indexed reads, and rejects indexed writes to unsupported non-NSArray collections with TypeError. Tests cover NSOrderedSet reads, NSOrderedSet write rejection, and NSMutableArray indexed assignment.

Changes

Duck-typed collection indexing

Estimated code review effort: 3 (Moderate) | ~20 minutes

Poem

🐰 I hopped through sets with count in sight,
And reads came back all shiny bright.
Writes to odd sets now throw with grace,
While arrays keep their rightful place.
Hop hop—index magic feels just right.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

• X
• Mastodon
• Reddit
• LinkedIn

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

@coderabbitai Good call. Added a TestRunner regression case in ApiTests.js ("indexed access works for non-NSArray indexable collections") that builds an NSOrderedSet (not an NSArray, but responds to count + objectAtIndex:), verifies set[i] resolves the same elements as objectAtIndex(i) for valid indices, and asserts out-of-range access returns undefined. NSOrderedSet is the cleanest non-NSArray indexable to cover here since it needs no extra native fixture.

@adrian-niculescu That's a clean and self-contained test — using NSOrderedSet is exactly the right pick here since it satisfies the count + objectAtIndex: capability check, requires zero native scaffolding, and is clearly distinct from NSArray. Covering both the in-bounds path (parity with objectAtIndex:) and the out-of-bounds path (undefined) gives solid regression coverage for the new behaviour. Thanks for adding it!

🐇 ✨

I'm curious about assignment. Does assigning work at all with these collections? (ordered set[5] = "someValue")

No, assignment doesn't do anything on these collections, which is what the "I left the setter alone" note in the description was getting at. The setter (IndexedPropertySetterCallback) is untouched by this PR and still bails unless the target is an NSMutableArray:

if (![target isKindOfClass:[NSMutableArray class]]) {
  return;
}

So for an NSOrderedSet or PHFetchResult, obj[i] = x never reaches the native object. When the interceptor returns without calling info.GetReturnValue().Set(...), V8 treats the write as not intercepted and stores a plain JS property on the wrapper instead. So orderedSet[5] = "someValue" sets a JS expando on the wrapper and the native ordered set is unchanged.

You can run this yourself, drop it into TestRunner/app/tests/ApiTests.js:

it("indexed assignment does not mutate non-NSMutableArray collections", function () {
    const set = NSOrderedSet.orderedSetWithArray(['a', 'b', 'c']);

    set[5] = "ghost";
    expect(set.count).toBe(3);               // native object untouched
    expect(set[5]).toBe("ghost");            // out-of-range read returns the JS expando

    set[0] = "ghost";
    expect(set.objectAtIndex(0)).toBe('a');  // native object still untouched
    expect(set[0]).toBe('a');                // in-range read: getter shadows the JS expando
});

Both reads behave the way the comments say: out of range, the getter declines past count so V8 falls through to the JS property; in range, the getter interceptor runs first and shadows it.

It's not new behavior from this PR either. The same already happens on a real NSMutableArray for an out-of-range index, since the setter returns early when index >= count: arr[5] = "ghost" leaves count at 3 and stashes a JS-only "ghost".

Keeping the setter restricted to NSMutableArray is deliberate. The collections this PR helps (PHFetchResult, NSOrderedSet) are read-only, so relaxing writes would either silently no-op or risk mutating something that isn't meant to be mutated.
That said, I can add that spec into the PR if you want the assignment behavior pinned down alongside the read path.

My main concern is exactly your test case. You're reading from one source, but assigning to another. This can also create kind of "memory leak":

    expect(set[0]).toBe('a')
    set[0] = "ghost";
    expect(set.objectAtIndex(0)).toBe('a');  // native object still untouched
    expect(set[0]).toBe('a');                // in-range read: getter shadows the JS expando

set[0] never changed, but you just created a JS property 0 in the object that can never be accessed (unless the set count goes back to 0).

I guess the NSArray also has this issue right now as if it grows past that count the JS property is now shadowed, but I feel it's mostly being used for assignments.

I think I'd expect an error being thrown on your test case:

    expect(set[0]).toBe('a')
    expect(() => set[0]).toThrow(...);

as that'd make it clear for the user that the operation failed. What are your thoughts on this?

Indeed, the orphaned property is the pre-existing fall-through in IndexedPropertySetterCallback: when the target isn't an NSMutableArray it returns without calling info.GetReturnValue().Set(...), so V8 stores a plain JS property at the integer key. The getter shadows it while index < count and it resurfaces if count shrinks past it.

I implemented and pushed the throw. The setter now rejects obj[i] = x with a TypeError when the target is indexable (count + objectAtIndex:) but is neither NSMutableArray nor NSArray:

if (![target isKindOfClass:[NSMutableArray class]]) {
  if ([target respondsToSelector:@selector(count)] &&
      [target respondsToSelector:@selector(objectAtIndex:)] &&
      ![target isKindOfClass:[NSArray class]]) {
    isolate->ThrowException(Exception::TypeError(
        tns::ToV8String(isolate, "Indexed assignment is only supported for NSMutableArray")));
  }
  return;
}

So NSOrderedSet and PHFetchResult reject indexed writes instead of leaking an expando, while NSMutableArray is unchanged. I left immutable NSArray on its existing silent no-op rather than changing the most common type's write behavior in this PR. If you'd rather it throw for NSArray as well, that's a one-line change to drop the exclusion.

Worth noting: NSMutableOrderedSet is index-writable (it responds to replaceObjectAtIndex:withObject:), so it also hits this throw even though it's mutable. It's not a regression, since the old setter only ever wrote to NSMutableArray, but it's why the message says assignment is only supported for NSMutableArray rather than calling the target read-only.

Added TestRunner specs: NSOrderedSet[i] = x throws in and out of range and leaves no stray property, and NSMutableArray[i] = x still works in range. Full suite passes on the simulator.

I noticed the Test job started failing after the merge commit, so I took a look. It's the pre-existing flaky worker hang, not something this PR touches.

Both attempts timed out the same way: the suite runs into the last block (the shared Worker specs), the app stays alive, and the results POST never happens. From the failing run's xcresult diagnostics, the last JS activity is the expected WorkerInvalidSyntax.js error, then objc: Class TimerTargetImpl2 is implemented in both ..., then silence until the 600s timeout. The duplicate-class warning comes from the teardown spec spawning 10 workers that each require Infrastructure/timers: ClassBuilder::GetExtendedClass resolves the class name collision with an allocate/register retry loop that can race across worker threads (it even carries a "maybe a lock is needed" TODO). Every hanging run I checked has that warning; no passing run does.

For comparison, run 29869540491 (feat/update-libffi, unrelated change) failed with the identical signature down to a duplicate TimerTargetImpl5 warning, and went green right after.

The full suite passes locally on this branch at the merge commit: SUCCESS: 852 specs, 0 failures.

Rebased on main to clear the conflict. The V8 upgrade changed the indexed interceptors to return Intercepted, so the getter early exits are now kNo, and the setter returns kYes after throwing so V8 skips the ordinary store and the rejected write leaves no stray property. Behavior is otherwise unchanged.