Swift: Flow through OpenExistentialExpr

[geoffw0]

Add flow through OpenExistentialExpr. We sometimes identify flow sources at calls whose qualifier has a protocol type, rather than an actual class type. Swift inserts an OpenExistentialExpr around these calls marking the conversion, and we weren't propagating taint through that - leading to a disappointing lack of results from these sources.

[MathiasVP]

Should we maybe hide OpenExistentialExpr from the AST instead? As you said yourself it behaves like an implicit conversion (which we already hide)

[geoffw0]

Should we maybe hide OpenExistentialExpr from the AST instead?

That sounds reasonable, I'm not fully confident about how to do it though. Can you point me towards a similar change that was made perhaps?

[MathiasVP]

That sounds reasonable, I'm not fully confident about how to do it though. Can you point me towards a similar change that was made perhaps?

Here's an example where we decided to hide InOutExpr from the AST: bf5ba37#diff-69ca0d5fe96a318dd0849377f54a1564f3416dcb9518dd8b751a93f1fff17156. You'd want to override the same predicate (i.e., convertsFrom) for OpenExistentialExpr.

Here's a PR that hides LValueTypes from the AST: #11566.

I hope that helps. Otherwise, feel free to ask for more input!

[geoffw0]

(note to self: probably not worth fixing the merge conflicts until after #13980 is merged)

[geoffw0]

Merge fixed. Ready to merge assuming the checks still pass.

[geoffw0]

... realized I hadn't addressed the suggestion to hide OpenExistentialExpr in the AST rather than implementing flow for it ...

I'm trying to understand the examples you linked a bit better. I can see that an InOutExpr is being modelled as a conversion (ConversionOrIdentityTree) with one child. Where does the actual hiding take place? Is the ConversionOrIdentityTree class special in any way (I think not)? Does the existence of convertsFrom(e) imply the e subtree is hidden (I think so)? Should I be overriding getResolveStep() directly instead?

Notably OpenExistentialExpr has two children, so it doesn't map to a conversion cleanly.

[MathiasVP]

I'm trying to understand the examples you linked a bit better. I can see that an InOutExpr is being modelled as a conversion (ConversionOrIdentityTree) with one child. Where does the actual hiding take place? Is the ConversionOrIdentityTree class special in any way (I think not)? Does the existence of convertsFrom(e) imply the e subtree is hidden (I think so)? Should I be overriding getResolveStep() directly instead?

Yes, the existence of convertsFrom (or getResolveStep) implies that it's hidden from the main AST. So if you implement something like:

override predicate convertsFrom(Expr e) { e = this.getImmediateSubExpr() }

on OpenExistentialExpr then the sub expression of the OpenExistentialExpr will be the expression that's "returned" by all the getX predicates on AST classes (instead of the OpenExistentialExpr as we're getting now).

[geoffw0]

I've just pushed the solution as discussed, along with extended tests...

• dataflow looks OK.
• in the CFG, we have disconnected the elements under getExistential, such as the call to getMyProtocol() in the third test case; this feels wrong. I could perhaps carefully define OpenExistentialTree to explore both the getExistential and the getSubExpr without ever landing on itself, which would resemble the non-OpenExistentialExpr CFG.
• in the AST we have the same things missing, e.g. call to getMyProtocol(), except that I don't know how to fix it. The non-OpenExistentialExpr case has that call as the base for the MethodLookupExpr, but in the OpenExistentialExpr case the base is just an OpaqueValueExpr.

[MathiasVP]

Alright, I can see that this may not be as straightforward as I was hoping then. Maybe we should just go back to your original proposed solution. Then we can always go back and do the AST hiding if this comes back to bite us (i.e., if we run into more instances where the OpenExistentialExpr surprised us).

Would that be okay with you @geoffw0? I know you spent a lot of time getting my suggestion to work (which I really appreciate), but I don't want to drag this on for too long since I know you've got a lot on your plate already.

[geoffw0]

I'll do that then, but will keep the new tests in case we decide to come back to this.

In principle I agree it would be better hidden, but looking at it now I think if that's possible at all it would require a bit of a reshuffle of the AST, not just hiding one element.

[geoffw0]

I've reverted the commit with the switch to AST hiding, merged with main, fixed some conflicts (in the tests).

[geoffw0]

DCA LGTM. There's supposedly a 15.4% analysis speedup, but I don't see how that related to the changes here. Probably just a rather large wobble.

[MathiasVP]

LGTM!

[geoffw0]

Fixed merge conflicts.

[MathiasVP]

LGTM!