fix: detect Map and Set mutation methods - #354
Open
electrohyun wants to merge 1 commit into
Open
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
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.
What is the purpose of this pull request?
This PR fixes an issue where
getStaticValue()statically evaluates.sizebased on the initial state without detecting mutating method calls on aSetorMap.In summary:
const set = new Set(); set.add("a"); set.size, the actual value ofset.sizeis1.set.add()as a mutation. Therefore, it returns{ value: 0 }.getStaticValue()should returnnull.What changes did you make? (Give an overview)
I kept the existing detection of mutating array methods unchanged and added the
SetandMapmutation checks as a separate condition.I changed
hasMutationInProperty()to receive the static value already evaluated from the variable's initializer. This allows it to detect each mutating method only when the method's receiver is the variable itself and its static initial value is an actualSetorMap.Set:add,delete,clearMap:set,delete,clearAs a result, when a direct mutating method call on a
SetorMapis detected,getStaticValue()returnsnullinstead of statically evaluating the current value from the initial state alone.I added regression tests that evaluate
.sizeafter calls toSet.prototype.add(),Set.prototype.delete(),Set.prototype.clear(),Map.prototype.set(),Map.prototype.delete(), andMap.prototype.clear().I also added tests to ensure that ordinary objects with non-mutating methods named
add,clear,delete, orsetcontinue to be statically evaluated as before. The tests verify that a mutatingSetmethod called through a statically evaluated computed property is detected, while a non-mutating method on an ordinary object called in the same way is not. They also verify that existing static evaluation of non-mutating methods such asSet.prototype.has()is preserved.I have summarized below why the
SetandMapmutation checks do not rely on method names alone. A name-only check could incorrectly treat ordinary objects with methods of the same name as mutated.Why the checks do not rely on method names alone
I initially considered generalizing the existing array mutation method check and adding
add,clear,delete, andsetto a name-based list of mutating methods. However, this approach does not check the receiver's actual type, so it would also treat an ordinary object like the following as mutated.object.set(2)does not mutateobject. The existing implementation evaluatesobject.valueas{ value: 1 }, but checking only the method name would treatsetas a mutation and returnnull. To avoid this unnecessary behavior change, I kept the existing array detection unchanged and limited the new mutation detection to cases where the static initial value is an actualSetorMapand the variable itself is the direct receiver.Related Issues
Fixes #353
Is there anything you'd like reviewers to focus on?
This change only handles cases where the static initial value is an actual
SetorMapand the variable itself is the direct receiver. Indirect mutations through an alias, a function argument, or calls such asSet.prototype.add.call()are outside the scope of this change.Disclosure: I'm a participant of open source contribution program OSSCA