Skip to content

Skip for-in properties shadowed by non-enumerable properties - #2557

Open
Thanatos9404 wants to merge 1 commit into
mozilla:masterfrom
Thanatos9404:fix-for-in-non-enumerable-shadowing
Open

Thanatos9404 wants to merge 1 commit into
mozilla:masterfrom
Thanatos9404:fix-for-in-non-enumerable-shadowing

Conversation

@Thanatos9404

Copy link
Copy Markdown

Fixes #1394.

for-in visited a prototype's enumerable property even when an object earlier in the chain had a non-enumerable property of the same name:

Object.prototype.length = 42;
for (var k in Function) print(k); // printed "length"

enumChangeObject added only the ids from getIds(), the enumerable ones, to the set of ids to skip. Objects without enumerable properties, such as Function and Function.prototype here, were passed over without adding anything. In EnumerateObjectProperties, a non-enumerable property of an object visited earlier hides a prototype's property of the same name.

The enumeration now keeps the ScriptableObjects it has left. Before it enumerates the next object that has enumerable properties, it adds their non-enumerable ids (getAllIds() minus getIds()) to that set.

  • Why it waits: a for-in over a large array, whose prototypes have no enumerable properties, never collects the array's indexes.
  • Only non-enumerable ids: enumerable properties added to an object during the loop must still not hide the prototype's, as before.
  • Proxies are skipped: asking again would call the ownKeys trap twice. With a trap, getIds() already returns every key it lists. Without one, a proxy behaves as before.
  • Other Scriptables: they behave as before.
  • Typed arrays: their getIds() lists only indexes, so their expando properties now count as non-enumerable here and hide same-named prototype properties. for-in already skips those expandos.

Tests:

  • six cases in ForTest:

    • the issue's example;
    • a direct shadow;
    • a shadowing object with no enumerable properties;
    • an array's length;
    • a property added during the loop;
    • the proxy ownKeys call count.

    The first four fail on master.

  • 12.6.4-2.js and order-enumerable-shadowed.js now pass, so I removed them from test262.properties.

./gradlew check passes. The one exception is JLineConsoleTest.barePrintlnWritesOnlyLineSeparator, which also fails on master on my Windows machine.

for-in only remembered the enumerable ids of the objects it had
already visited, and passed over objects without enumerable properties
entirely, so a non-enumerable property did not hide an enumerable
property of the same name further up the prototype chain. Before
enumerating the next object with enumerable properties, add the
non-enumerable ids of the objects already left to the ids to skip.

Fixes mozilla#1394
@gbrail

gbrail commented Nov 6, 2026

Copy link
Copy Markdown
Collaborator

The challenge with this PR is that it introduces a performance regression in some cases because we have to populate and manage a HashSet when enumerable and non-enumerable properties are mixed on the prototype chain. We need to follow the spec, so at some point some amount of overhead is going to be necessary, but we need to see if there are other opportunities. The AI I threw at this problem ran some performance regressions and came up with some ways to minimize the overhead in some cases -- perhaps that's something that you can do as well.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ES5 Enumerable properties can be shadowed by non-enumerables

2 participants