[compiler] InferReferenceEffects outputs a disjoint set of aliases #30974
Conversation
Test Plan: For the purposes of a pass added later in this stack, the InferReferenceEffects now outputs a DisjointSet of identifier ids representing aliases. We add this to InferReferenceEffects because this pass already does its own alias analysis, so making it available to later passes is convenient. This entails implementing copy() and equals() methods on DIsjointSets so that the InferenceState copy and merge methods can handle alias sets. [ghstack-poisoned]
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
… aliases" Test Plan: For the purposes of a pass added later in this stack, the InferReferenceEffects now outputs a DisjointSet of identifier ids representing aliases. We add this to InferReferenceEffects because this pass already does its own alias analysis, so making it available to later passes is convenient. This entails implementing copy() and equals() methods on DIsjointSets so that the InferenceState copy and merge methods can handle alias sets. [ghstack-poisoned]
There was a problem hiding this comment.
Nit: It might be helpful to be explicit about what this alias map represents with a comment / docblock . iiuc InferReferenceEffects aliases are conservative (no false positives, maybe false negatives). InferReactiveScopeVariables on the other hand uses maybe-aliases (e.g. pessimistic on function calls)
Also had a qq: reading through InferReferenceEffects, I notice that we currently alias each destructuring target of a Destructure instruction to source place.
It seems that all other alias callsites are for known / precise aliases (LoadLocal and StoreLocals, evaluation of a PropertyStore). Similarly, we don't alias PropertyLoads. What's the difference between Destructure and PropertyLoad here?
… aliases" Test Plan: For the purposes of a pass added later in this stack, the InferReferenceEffects now outputs a DisjointSet of identifier ids representing aliases. We add this to InferReferenceEffects because this pass already does its own alias analysis, so making it available to later passes is convenient. This entails implementing copy() and equals() methods on DIsjointSets so that the InferenceState copy and merge methods can handle alias sets. [ghstack-poisoned]
There was a problem hiding this comment.
Seems fine to return the alias sets (though i agree w @mofeiZ about adding some comments for what it represents). But I think the equals() method might have a bug, can you add some extra tests (jest tests, not normal snap tests) to double-check?
| equals(other: DisjointSet<T>): boolean { | ||
| if (this.size !== other.size) { | ||
| return false; | ||
| } | ||
| const rootMap = new Map<T, T>(); | ||
| for (const item of this.#entries.keys()) { | ||
| const otherRoot = other.find(item); | ||
| if (otherRoot === null) { | ||
| return false; | ||
| } | ||
| const thisRoot = this.find(item); | ||
| CompilerError.invariant(thisRoot != null, { | ||
| reason: 'Expected item to be in set', | ||
| loc: null, | ||
| }); | ||
| if (rootMap.get(thisRoot) == null) { | ||
| rootMap.set(thisRoot, otherRoot); | ||
| } else if (rootMap.get(thisRoot) !== otherRoot) { | ||
| return false; | ||
| } | ||
| } | ||
| for (const item of other.#entries.keys()) { | ||
| const thisRoot = this.find(item); | ||
| if (thisRoot === null) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
can you add some tests for this?
imagine two Sets:
thishas groups{a},{b}otherhas one group,{a, b}
we'd iterate this:
a. otherRoot = a or b, thisRoot = a, rootMap doesn't have a so we continueb. otherRoot = a or b, thisRoot = b, rootMap doesn't have b so we continue
now we iterate other:
a, thisRoot = a which is non-null so continueb, thisRoot = b which is non-null so continue
So i think this example ends up incorrectly returning true (equals) when they're not equal.
… aliases" Test Plan: For the purposes of a pass added later in this stack, the InferReferenceEffects now outputs a DisjointSet of identifier ids representing aliases. We add this to InferReferenceEffects because this pass already does its own alias analysis, so making it available to later passes is convenient. This entails implementing copy() and equals() methods on DIsjointSets so that the InferenceState copy and merge methods can handle alias sets. [ghstack-poisoned]
… aliases" Test Plan: For the purposes of a pass added later in this stack, the InferReferenceEffects now outputs a DisjointSet of identifier ids representing aliases. We add this to InferReferenceEffects because this pass already does its own alias analysis, so making it available to later passes is convenient. This entails implementing copy() and equals() methods on DIsjointSets so that the InferenceState copy and merge methods can handle alias sets. [ghstack-poisoned]
|
Fixed, thanks for the catch! Now build up the rootMap before comparing the keys. Added tests as well. |
Stack from ghstack (oldest at bottom):
Test Plan:
For the purposes of a pass added later in this stack, the InferReferenceEffects now outputs a DisjointSet of identifier ids representing aliases. We add this to InferReferenceEffects because this pass already does its own alias analysis, so making it available to later passes is convenient.
This entails implementing copy() and equals() methods on DIsjointSets so that the InferenceState copy and merge methods can handle alias sets.