Ruby: Summarized type-tracking stores should target post-update nodes #14628
Conversation
| // Relating nodes to summaries | ||
| /** Gets a dataflow node respresenting the argument of `call` indicated by `arg`. */ | ||
| Node argumentOf(Node call, SummaryComponent arg); | ||
| Node argumentOf(Node call, SummaryComponent arg, boolean isOutput); |
There was a problem hiding this comment.
Could we explain the role of isOutput in the comment?
| result = I::parameterOf(prev, head) and | ||
| isOutput0 = false and | ||
| isOutput = true | ||
| or | ||
| result = I::returnOf(prev, head) | ||
| result = I::returnOf(prev, head) and | ||
| isOutput0 = false and | ||
| isOutput = false | ||
| or | ||
| componentLevelStep(head) and | ||
| result = prev | ||
| result = prev and | ||
| isOutput = isOutput0 |
There was a problem hiding this comment.
I am struggling to reconstruct the logic here. isOutput, in the base case is only true for returns. Here we preclude taking the parameter of an output and we claim that a return is not an output. Could you add a comment with the guiding principle?
There was a problem hiding this comment.
The first case for when there is output that goes into a callback: output = Argument[0].Parameter[1]; in this case the Argument[0] bit should resolve to the normal argument node and not the post-update node.
The second case is for when there is input from a callback: input = Argument[0].ReturnValue, where we again don't want Argument[0] to give us the post-update node.
There was a problem hiding this comment.
Should isOutput (not-0) be true in the second case, then?
There was a problem hiding this comment.
No, because it is for cases like input = Argument[0].ReturnValue
| nodeFrom = evaluateSummaryComponentStackLocal(callable, call, input) and | ||
| nodeTo = evaluateSummaryComponentStackLocal(callable, call, output) | ||
| nodeFrom = evaluateSummaryComponentStackLocal(callable, call, input, false) and | ||
| nodeTo = evaluateSummaryComponentStackLocal(callable, call, output, true) | ||
| ) | ||
| } |
There was a problem hiding this comment.
This predicate is not a store step, should it have the same treatment? (The commit title indicates maybe not.)
There was a problem hiding this comment.
Yes, it should have the same treatment, good question though.
| result = I::parameterOf(prev, head) and | ||
| isOutput0 = false and | ||
| isOutput = true | ||
| or | ||
| // `ReturnValue` is only allowed int the input of flow summaries (hence `isOutput = false`), |
There was a problem hiding this comment.
| // `ReturnValue` is only allowed int the input of flow summaries (hence `isOutput = false`), | |
| // `ReturnValue` is only allowed in the input of flow summaries (hence `isOutput = false`), |
| result = I::parameterOf(prev, head) and | ||
| isOutput0 = false and | ||
| isOutput = true | ||
| or | ||
| // `ReturnValue` is only allowed int the input of flow summaries (hence `isOutput = false`), |
There was a problem hiding this comment.
| // `ReturnValue` is only allowed int the input of flow summaries (hence `isOutput = false`), | |
| // `ReturnValue` is only allowed in the input of flow summaries (hence `isOutput = false`), |
hvitved
force-pushed
the
ruby/type-tracking-store-post-update
branch
from
a68278b to
0c5b528
Compare
No description provided.