Fixed an issue with ReduceLabel flows spoiling node reachability cache
#61265
Conversation
| @@ -28178,7 +28192,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| const target = (flow as FlowReduceLabel).node.target; | |||
| 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 temporarily 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
Andarist
force-pushed
the
fix/reduce-label-spoiling-reachability-cache
branch
from
2a860e8 to
4552c20
Compare
fixes #61259