Ruby: Add case string comparison barrier guard #11114
Conversation
This recognises barriers of the form
STRINGS = ["foo", "bar"]
case foo
when "some string literal"
foo
when *["other", "strings"]
foo
when *STRINGS
foo
end
where the reads of `foo` inside each `when` are guarded by the comparison
of `foo` with the string literals.
We don't yet recognise this construct:
case foo
when "foo", "bar"
foo
end
This is due to a limitation in the shared barrier guard logic.
|
|
||
| final override SplatExpr getExpr() { result = super.getExpr() } | ||
|
|
||
| final ExprCfgNode getOperand() { result.getExpr() = e.(SplatExpr).getOperand() } |
There was a problem hiding this comment.
Please add a class SplatExprChildMapping and change this to
| final ExprCfgNode getOperand() { result.getExpr() = e.(SplatExpr).getOperand() } | |
| final ExprCfgNode getLhs() { e.hasCfgChild(e.getOperand(), this, result) } |
There was a problem hiding this comment.
Sure, but why (the extra class)?
There was a problem hiding this comment.
Well regardless of why, I think that's fixed the issue below with SplatExprCfgNode.getAnOperand() not giving results for array literals. Still, in the absence of any documentation comments, it would be nice to know why.
There was a problem hiding this comment.
There may be multiple CFG nodes for the same AST node due to control flow graph splitting. The e.hasCfgChild predicate picks the CfgNode that matches the AstNode child node and is connected to the current CfgNode.
| branch = true and | ||
| exists(CfgNodes::ExprNodes::CaseExprCfgNode case | | ||
| case.getValue() = testedNode and | ||
| exists(CfgNodes::ExprNodes::WhenClauseCfgNode branchNode | |
There was a problem hiding this comment.
This doesn't handle in clauses. It looks much harder to be sure that an expression is guarded by an in pattern, because the patterns can be complex. I guess we'd have to check that there are no new variable bindings in the pattern or something. Anyway, I think that's something to tackle in the future.
There was a problem hiding this comment.
I've pushed a commit that handles very simple in clauses where the pattern is a single string literal, such as
case foo
in "foo"
foo
end| ) { | ||
| branch = true and | ||
| exists(CfgNodes::ExprNodes::CaseExprCfgNode case | | ||
| case.getValue() = testedNode and |
There was a problem hiding this comment.
Should we also handle the case where getValue does not exist?
case
when foo == "foo" then ...
end
There was a problem hiding this comment.
Weird, I didn't know you could do this. This appears to already be covered by our existing StringConstCompareBarrier. I'll add a test for it.
| pattern instanceof ExprNodes::StringLiteralCfgNode | ||
| or | ||
| // array literals behave weirdly in the CFG so we need to drop down to the AST level for this bit | ||
| // specifically: `SplatExprCfgNode.getOperand()` does not return results for array literals |
There was a problem hiding this comment.
I'm a bit surprised by this comment. It could be that there is a bug in the CFG library, but other than that getOperand should return an array literal.
There was a problem hiding this comment.
Your suggested change for SplatExprCfgNode appears to have fixed this. I've pushed an update.
| foo | ||
| end | ||
|
|
||
| if foo == "foo" or foo == "bar" # not recognised |
There was a problem hiding this comment.
I think this should be fixed by #11153
|
@hmac I trivially fixed all merge conflicts in the test cases through the GitHub UI. You probably need to refresh the expected output, though. |
This recognises barriers of the form
where the reads of
fooinside eachwhenare guarded by the comparisonof
foowith the string literals.We don't yet recognise this construct:
This is due to a limitation in the shared barrier guard logic.