slice helper should check for the ES5array or Proxy in the protototype #3415
Conversation
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
Few array apis (reverse, shift, unshift and sort) do fill from prototype and still take the faster path to complete the respective logic. It is possible that FillFromPrototype will mutate the current array, in that case taking faster path is wrong and give you wrong results. However as you already done FillFromPrototype you already mutated the current object so now you cannot take the slower path as it will create further deviations. In order to fix those I have put a precheck which actually looks the prototype chain and find out if there is any ES5Array or Proxy of any sort. If we don't have any of those we cannot change the current array's length or type (I am not talking about native array to var array). With these check we can safely decide which way we can go. Added few tests cases to excercise the scenario, where previously we were taking the faster path and leading to wrong result (we have compliant gap).
Since the unshift expand to new length. It is possible that the current array does not have any missing item but it still has accessor on the proto chain. That setter will not be fired as we were taking the no-side effect path. Fixed that by forcing to check the proto chain for the unshift case. Added test case for more understanding of the scenario.
|
|
This way we can identify if the array's content can be affected. If there is sideeffect expected we can take the slower to finish the logic.
chakrabot
pushed a commit
that referenced
this pull request
Jul 25, 2017
…Proxy in the protototype Merge pull request #3415 from akroshg:nosideeffect This way we can identify if the array's content can be affected. If there is sideeffect expected we can take the slower path to finish the logic.
chakrabot
pushed a commit
that referenced
this pull request
Jul 25, 2017
…5array or Proxy in the protototype Merge pull request #3415 from akroshg:nosideeffect This way we can identify if the array's content can be affected. If there is sideeffect expected we can take the slower path to finish the logic.
chakrabot
pushed a commit
that referenced
this pull request
Jul 25, 2017
…ck for the ES5array or Proxy in the protototype Merge pull request #3415 from akroshg:nosideeffect This way we can identify if the array's content can be affected. If there is sideeffect expected we can take the slower path to finish the logic.
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.
This way we can identify if the array's content can be affected. If there is sideeffect expected we can take the slower
to finish the logic.