Fixed an issue with ReduceLabel flows spoiling node reachability cache#61265
Fixed an issue with ReduceLabel flows spoiling node reachability cache#61265Andarist wants to merge 4 commits intomicrosoft:mainfrom
ReduceLabel flows spoiling node reachability cache#61265Conversation
src/compiler/checker.ts
Outdated
| const saveAntecedents = target.antecedent; | ||
| target.antecedent = (flow as FlowReduceLabel).node.antecedents; | ||
| const result = isReachableFlowNodeWorker((flow as FlowReduceLabel).antecedent, /*noCacheCheck*/ false); | ||
| const result = isReachableFlowNodeWorker((flow as FlowReduceLabel).antecedent, FlowNodeReachableCacheFlags.None); |
There was a problem hiding this comment.
as the comment in this block says:
// Cache is unreliable once we start adjusting labelsWe can't let this isReachableFlowNodeWorker call here to populate the cache as that would create entries based on the temporary antecedent(s). Anything computed below this call can't be preserved, at least not without including the id of this ReduceLabel flow in the cache key or smth.
So this disables cache writes. As later on that would be used on the same shared flow but with "correct" antecedents (and that's the only result the compiler should cache and trust).
There was a problem hiding this comment.
I'm still considering 2 things here:
- should this code maybe save & restore
flowNodeReachable[getFlowNodeId(target)]? - should reads be permanently disabled here? this would make the previous consideration obsolete
I have not been able to produce a test case that would prove either to be required so far but I have only spent a little bit of time on it.
There was a problem hiding this comment.
cc @ahejlsberg who has been thinking about this sort of thing recently and I believe had ideas on how to simplify the flow node logic here
There was a problem hiding this comment.
fwiw, Corsa still suffers from this (checked at microsoft/typescript-go@a92eff4 )
Andarist
force-pushed
the
fix/reduce-label-spoiling-reachability-cache
branch
from
2a860e8 to
4552c20
Compare
…ling-reachability-cache
|
With 6.0 out as the final release vehicle for this codebase, we're closing all PRs that don't fit the merge criteria for post-6.0 patches. If you think this was a mistake and this PR fits the post-6.0 patch criteria, please post to the 6.0 iteration issue with details (specifically, which PR and which patch criteria it satisfies). Next steps for PRs:
|
3 participants
fixes #61259
fixes #63004