[Fix] avoid relying on indexOf's HasProperty+Get semantics, unlike includes's bare Get - #54
Conversation
…ike `includes`'s bare `Get` The fast path delegated to $indexOf.apply(this, arguments) whenever searchElement wasn't NaN/undefined and fromIndex was finite. For ordinary arrays this is equivalent to the spec's own includes algorithm, but indexOf's per-index step is "HasProperty, then Get only if present" while includes's is a bare Get -- so for an exotic object whose has trap disagrees with its get trap (e.g. a Proxy), delegating to indexOf can silently miss an element that includes must find. Removed the optimization; the existing manual loop already implements the correct includes algorithm (bare Get, SameValueZero) and was only being skipped by this shortcut.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #54 +/- ##
==========================================
+ Coverage 96.36% 97.95% +1.59%
==========================================
Files 5 5
Lines 55 49 -6
Branches 8 7 -1
==========================================
- Hits 53 48 -5
+ Misses 2 1 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
The failing Node matrix appears unrelated to this change: all 15 failures stop during dependency setup before the test command runs. The shared workflow's determine-node-version step upgrades to npm 12.0.2, then |
c7d26a3 to
70ef118
Compare
ljharb
left a comment
There was a problem hiding this comment.
I've rebased this after pushing up fixes to the test suite.
|
Thanks for rebasing this and pushing the test-suite fixes. I’ve pulled the updated branch context on my side — please let me know if you want any additional adjustments from me. |
Summary
The fast path delegates to
$indexOf.apply(this, arguments)wheneversearchElementisn't NaN/undefined andfromIndexis finite. For ordinary arrays this matches the spec's ownincludesalgorithm, butindexOf's per-index step is HasProperty, then Get only if present, whileincludes's is a bareGet— per spec (Array.prototype.includesexplicitly notes it "does not skip missing array elements, instead treating them as undefined", unlikeindexOf).For an exotic object whose
hastrap disagrees with itsgettrap, that difference is observable:Changes
implementation.js: removed theindexOf-based fast path (and its now-unused$isNaN/$isFinite/$indexOfrequires). The existing manual loop already implements the correctincludesalgorithm (bareGetviaO[k],SameValueZero) and was only being skipped by this shortcut — so removing it is enough, no new logic needed.test/tests.js: one new case (guarded bytypeof Proxy === 'function', matching the pattern inis-callable's own tests) confirming a lyinghastrap doesn't hide an element that's still reachable viaget.Testing
implementation.jspath is actually exercised by the bug, sinceindex.js/shimmed.jsprefer the native method when available).eslintreports the same pre-existinglinebreak-style/no-magic-numbersfindings with and without this change (Windows checkout CRLF artifact + pre-existing style, unrelated) — no new lint issues, and one magic-number warning (-1) actually goes away since that line is removed.