Ruby: More splat flow (alternative) #14090
Conversation
Allow flow from a splat argument to a positional parameter in cases
where there are positional arguments left of the splat. For example:
def foo(x, y, z); end
foo(1, *[2, 3])
There was a problem hiding this comment.
This aligns with how I envisioned it in my comment #13974 (review). A few comments.
| exists(c.asCallable()) and // exclude library callables | ||
| isParameterNode(_, c, any(ParameterPosition p | p.isSplat(_))) | ||
| exists(c.asCallable()) // exclude library callables | ||
| // and isParameterNode(_, c, any(ParameterPosition p | p.isSplat(_))) |
| // since that won't (probably) won't reach the parameters of the callable. | ||
| // This saves a node per call. | ||
| n >= 0 and | ||
| node1.asExpr().(Argument).isArgumentOf(call, any(ArgumentPosition p | p.isSplat(splatPos))) and |
There was a problem hiding this comment.
Shouldn't this be limited to splatPos > 0?
| exists(int n, int splatPos, CfgNodes::ExprNodes::CallCfgNode call | | ||
| // Don't propagate taint on the `self` element of the splat | ||
| // since that won't (probably) won't reach the parameters of the callable. | ||
| // This saves a node per call. |
There was a problem hiding this comment.
I'm not sure I understand this comment.
There was a problem hiding this comment.
This was a misunderstanding on my part that content at index -1 represented the value of the container itself. That's why I added the n >= 0 constraint. I've now realised this isn't true and that positional content is only defined for n in [0..10] so I'll remove the constraint and this comment.
| // Don't propagate taint on the `self` element of the splat | ||
| // since that won't (probably) won't reach the parameters of the callable. | ||
| // This saves a node per call. | ||
| n >= 0 and |
| node2.getPosition() = n + splatPos and | ||
| ( | ||
| c = getPositionalContent(n) or | ||
| c.isSingleton(TUnknownElementContent()) |
There was a problem hiding this comment.
n is not bound in this case; in this case it should store into a version of SynthSplatArgumentElementNode that represents unknown content (you could abuse TSynthSplatArgumentElementNode(-1) to represent unknown).
| } | ||
|
|
||
| class TSourceParameterNode = | ||
| TNormalParameterNode or TBlockParameterNode or TSelfParameterNode or | ||
| TSynthHashSplatParameterNode or TSynthSplatParameterNode or TSynthSplatArgParameterNode; | ||
| TSynthHashSplatParameterNode or TSynthSplatParameterNode or TSynthSplatArgParameterNode or | ||
| TSynthSplatArgumentElementNode; |
| TSynthSplatArgumentNode(CfgNodes::ExprNodes::CallCfgNode c) or | ||
| TSynthSplatArgumentElementNode(CfgNodes::ExprNodes::CallCfgNode c, int n) { | ||
| n in [0 .. 10] and | ||
| exists(Argument arg, ArgumentPosition pos | pos.isSplat(_) and arg.isArgumentOf(c, pos)) |
There was a problem hiding this comment.
I believe this should be limited to pos.isSplat(any(int splatPos | splatPos > 0))?
There was a problem hiding this comment.
If we do that, then I think it breaks cases like
def f(x, y); end
f(*[taint, 2])because we no longer match the explicit splat arg to the synth splat parameter. If we re-add ppos.isSynthSplat() and apos.isSplat(0) to parameterMatch, then we still have a failure in cases like
def g(x, *y); end
g(*[1, taint])
No description provided.