Allow indexed access for non-NSArray collections

[adrian-niculescu]

Right now obj[i] from JS only works when the native object is an NSArray, because the indexed getter checks isKindOfClass:[NSArray class]. Plenty of other ordered collections are just as indexable but get left out. The one that bit me was PHFetchResult from the Photos framework.

This swaps that NSArray check in the getter for a capability check: if the target responds to both count and objectAtIndex:, treat it as indexable. NSArray keeps behaving exactly like before, and things like PHFetchResult and NSOrderedSet start working. The bounds check is unchanged, so out of range still returns nothing.

The setter uses the same capability check. It still writes only to NSMutableArray, but instead of silently dropping obj[i] = x for other indexable collections (which lets V8 stash an unreachable JS property on the wrapper), it now throws a TypeError when the target is indexable and not an NSArray. Immutable NSArray keeps its existing silent no-op.

A few notes:

• Requiring both selectors keeps it tight. NSSet and NSDictionary respond to count but not objectAtIndex:, so they're unaffected, and NSPointerArray uses pointerAtIndex: rather than objectAtIndex:, so it gets skipped instead of crashing.
• NSMutableOrderedSet is index-writable but isn't handled by the setter, so it throws on assignment too. That's the same as the old behavior of no-op'ing anything that wasn't an NSMutableArray, now made explicit.

TestRunner covers bracket reads on NSOrderedSet (in range matches objectAtIndex:, out of range returns undefined) and indexed assignment throwing without leaving a stray property, with NSMutableArray writes still working.

Summary by CodeRabbit

• Bug Fixes
  • Broadened array-style indexing support: Objective-C objects that expose both count and objectAtIndex: can now be read with bracket notation, with correct bounds behavior.
  • Tightened indexed assignment: bracket-based writes are now rejected with a TypeError for indexable non-NSArray collections, avoiding silent no-ops.
• Tests
  • Added coverage for bracket reads on non-NSArray indexable collections (e.g., ordered sets).
  • Added coverage ensuring indexed assignment throws and does not create stray properties; confirmed indexed assignment still works for mutable arrays.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 579f9877-40af-42e1-a82b-16f731792bdd

📥 Commits

Reviewing files that changed from the base of the PR and between 7e5cbb5 and e621baa.

📒 Files selected for processing (2)

• NativeScript/runtime/ArgConverter.mm
• TestRunner/app/tests/ApiTests.js

🚧 Files skipped from review as they are similar to previous changes (2)

• NativeScript/runtime/ArgConverter.mm
• TestRunner/app/tests/ApiTests.js

---

📝 Walkthrough

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

Layer / File(s)	Summary
Indexed getter generalization and read validation NativeScript/runtime/ArgConverter.mm, TestRunner/app/tests/ApiTests.js	IndexedPropertyGetterCallback now uses respondsToSelector: for count and objectAtIndex:, checks bounds with [target count], retrieves with [target objectAtIndex:index], and test coverage verifies NSOrderedSet bracket reads and out-of-range undefined.
Indexed setter error handling for unsupported collections NativeScript/runtime/ArgConverter.mm, TestRunner/app/tests/ApiTests.js	IndexedPropertySetterCallback now throws TypeError for indexed writes to indexable-but-unsupported collections, and tests verify NSOrderedSet write rejection plus unchanged contents and NSMutableArray indexed assignment behavior.

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.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: enabling indexed access for non-NSArray Objective-C collections.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

🧹 Nitpick comments (1)

NativeScript/runtime/ArgConverter.mm (1)

886-897: ⚡ Quick win

Add regression tests for new duck-typed indexable collections.

Line 886 introduces a broader capability check (count + objectAtIndex:), but this behavior change is not protected by tests in this PR. Please add a TestRunner case that verifies obj[i] on at least NSOrderedSet (and ideally another non-NSArray type) including out-of-range behavior.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@NativeScript/runtime/ArgConverter.mm` around lines 886 - 897, Add regression
tests exercising the new duck-typed indexable behavior introduced in
ArgConverter.mm (the count + objectAtIndex: check) by adding a TestRunner case
that (1) creates an NSOrderedSet and at least one other non-NSArray indexable
(e.g., PHFetchResult / custom object that implements count and objectAtIndex:),
(2) verifies that Objective-C indexing via obj[i] returns the same elements as
NSArray-style access for valid indices, and (3) verifies out-of-range behavior
(confirm it returns nil or the same error/behavior as NSArray in the runtime)
for indices >= count; place assertions in the TestRunner so this new behavior is
covered by the test suite.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@NativeScript/runtime/ArgConverter.mm`:
- Around line 886-897: Add regression tests exercising the new duck-typed
indexable behavior introduced in ArgConverter.mm (the count + objectAtIndex:
check) by adding a TestRunner case that (1) creates an NSOrderedSet and at least
one other non-NSArray indexable (e.g., PHFetchResult / custom object that
implements count and objectAtIndex:), (2) verifies that Objective-C indexing via
obj[i] returns the same elements as NSArray-style access for valid indices, and
(3) verifies out-of-range behavior (confirm it returns nil or the same
error/behavior as NSArray in the runtime) for indices >= count; place assertions
in the TestRunner so this new behavior is covered by the test suite.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: a76db5ad-c7e4-4afe-8a22-8c1226e52930

📥 Commits

Reviewing files that changed from the base of the PR and between 05224d3 and c210aa5.

📒 Files selected for processing (1)

• NativeScript/runtime/ArgConverter.mm

[adrian-niculescu]

@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.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@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!

🐇 ✨

[edusperoni]

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

[adrian-niculescu]

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.

[edusperoni]

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?

[adrian-niculescu]

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.