Repository navigation
Skip for-in properties shadowed by non-enumerable properties - #2557
Open
Thanatos9404 wants to merge 1 commit into
Open
Thanatos9404 wants to merge 1 commit into
Thanatos9404 wants to merge 1 commit into
Conversation
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
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
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
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.
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:
enumChangeObjectadded only the ids fromgetIds(), the enumerable ones, to the set of ids to skip. Objects without enumerable properties, such asFunctionandFunction.prototypehere, 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()minusgetIds()) to that set.ownKeystrap twice. With a trap,getIds()already returns every key it lists. Without one, a proxy behaves as before.Scriptables: they behave as before.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:length;ownKeyscall count.The first four fail on master.
12.6.4-2.jsandorder-enumerable-shadowed.jsnow pass, so I removed them fromtest262.properties../gradlew checkpasses. The one exception isJLineConsoleTest.barePrintlnWritesOnlyLineSeparator, which also fails on master on my Windows machine.