Swift: collection/tuple content for dictionary flow #13947
Conversation
FTR, this is also how it's done in C# 👍 (though instead of |
|
That's good to know. Swift has had first-class tuples from the start, so they just alias |
| subscript.getArgument(0).getExpr() = node1.asExpr() and | ||
| node2.(DictionarySubscriptNode).getExpr() = subscript and | ||
| c.isSingleton(any(Content::TupleContent tc | tc.getIndex() = 1)) | ||
| or | ||
| assign.getSource() = node1.asExpr() and | ||
| node2.(DictionarySubscriptNode).getExpr() = subscript and | ||
| c.isSingleton(any(Content::TupleContent tc | tc.getIndex() = 1)) | ||
| or | ||
| node1.(DictionarySubscriptNode) = node1 and | ||
| node2.asExpr() = subscript and | ||
| c.isSingleton(any(Content::CollectionContent cc)) |
There was a problem hiding this comment.
So now we do:
source -> TupleContent -> CollectionContent
in two steps (which makes sense), and I gather that the last of these three cases is the CollectionContent step. But why do we need two cases to cover the first step? I would have thought that only the middle cases was needed, and the first case would happen through reverse reads?
There was a problem hiding this comment.
I think there are two steps to assemble the tuple because one is for the key and one is for the value.
There was a problem hiding this comment.
The first case is the step from the key in dict[key] = value - I typoed and it should be tc.getIndex() = 0 there.
swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll
Outdated
Show resolved
Hide resolved
swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll
Outdated
Show resolved
Hide resolved
| subscript.getArgument(0).getExpr() = node1.asExpr() and | ||
| node2.(DictionarySubscriptNode).getExpr() = subscript and | ||
| c.isSingleton(any(Content::TupleContent tc | tc.getIndex() = 1)) | ||
| or | ||
| assign.getSource() = node1.asExpr() and | ||
| node2.(DictionarySubscriptNode).getExpr() = subscript and | ||
| c.isSingleton(any(Content::TupleContent tc | tc.getIndex() = 1)) | ||
| or | ||
| node1.(DictionarySubscriptNode) = node1 and | ||
| node2.asExpr() = subscript and | ||
| c.isSingleton(any(Content::CollectionContent cc)) |
There was a problem hiding this comment.
I think there are two steps to assemble the tuple because one is for the key and one is for the value.
| ( | ||
| c.isSingleton(any(Content::FieldContent fc)) or | ||
| c.isSingleton(any(Content::TupleContent tc)) | ||
| ) |
There was a problem hiding this comment.
Why this restriction? Do we not want to clear CollectionContent and ArrayContent?
There was a problem hiding this comment.
We don't want to clear CollectionContent and ArrayContent at a post-update node because they may have been only partially overwritten - consider the case here. It shouldn't clear the content from line 779.
I think we do want to clear EnumContent, though. Good catch.
I think so, yes. I'd still like to see a DCA run on this PR, though 🙏. In addition, it would be nice if we could have a testcase that demonstrated the impact of 3ee3eab (although this can be done in a future PR if you prefer). |
|
Possible analysis performance regression showing in the DCA runs, particularly the second one. Not sure if it's real or wobble. A small real regression would be acceptable.
I agree. |
|
I haven't been able to find any obvious performance issues locally, and the worst cases do look like wobble (very high standard deviations for the slower run). I wonder if we should implement type pruning and see if that fixes performance... |
|
If you can't reproduce it then it's probably fine. But let's do another DCA run after the merge-conflict has been resolved just to be safe 🤞. |
|
Timings in the latest DCA run look reasonable |
|
Indeed, this LGTM! Now we just need to:
and then I think this PR is good to go 🎉 |
This PR models dictionary content as being
TupleContentnested withinCollectionContent, and adds a newDictionarySubscriptNodeto represent the intermediate step, where only theTupleContenthas been stored, or only theCollectionContenthas been read. This means models for the genericCollectionprotocol will work for dictionaries without modification.