Swift: Dataflow through ?? and ? :

[geoffw0]

Adds dataflow through the nil coalescing operator (??) and the ternary operator (? :), along with more tests of optionals.

Flow through optional chaining (?.) was already working.

Flow through the second argument of ?? does not currently work, despite the code for this path (tested as far as I could). This appears to be because the Expr nodes inside the (typically) AutoClosureExpr there are not DataFlow::Nodes. Assuming @MathiasVP can't suggest a quick fix I will create a follow-up issue to address that.

[MathiasVP]

Flow through the second argument of ?? does not currently work, despite the code for this path (tested as far as I could). This appears to be because the Expr nodes inside the (typically) AutoClosureExpr there are not DataFlow::Nodes.
Assuming @MathiasVP can't suggest a quick fix I will create a follow-up issue to address that.

This is probably a CFG issue (since non-reachable code isn't given any DataFlow::Nodes). Let's address this as a follow-up.

I have a couple of stylistic comments, but otherwise the code LGTM!

[MathiasVP]

A couple of cleanup things. I think the second operand of ?? will always be an AutoClosureExpr. In that case, we can use the getReturn predicate on that class. There are two additional cleanups we can do here:

• Calling getStaticTarget on a BinaryExpr is a bit weird. Maybe we should have a synonym (like getOperator?) defined on BinaryExpr (and similarly for UnaryExprs). And maybe we should have a publically-exposed class for the ?? operator?
• AutoClosureExpr.getReturn() is necessary in CFG construction, but non-CFG code probably wants a way to get the expression being returned (instead of getting the ReturnStmt). Maybe we should have a getExpr predicate on AutoClosureExpr that's defined as this.getReturn().getResult()?

Suggested change

	exists(BinaryExpr nco |
	nco.getFunction().(DeclRefExpr).getDecl().(FreeFunctionDecl).getName() = "??(_:_:)" and
	(
	// value argument
	nodeFrom.asExpr() = nco.getAnOperand()
	or
	// unpack closure (the second argument is typically an `AutoClosureExpr` argument)
	nodeFrom.asExpr() =
	nco.getAnOperand().(AbstractClosureExpr).getBody().getAnElement().(ReturnStmt).getResult()
	) and
	nodeTo.asExpr() = nco
	)
	exists(BinaryExpr nco |
	nco.getStaticTarget().(FreeFunctionDecl).getName() = "??(_:_:)" and
	nodeTo.asExpr() = nco
	|
	// value argument
	nodeFrom.asExpr() = nco.getAnOperand()
	or
	// unpack closure (the second argument is typically an `AutoClosureExpr` argument)
	nodeFrom.asExpr() = nco.getAnOperand().(AutoClosureExpr).getReturn().getResult()
	)

[geoffw0]

I think the second operand of ?? will always be an AutoClosureExpr.

My understanding is that you can provide a manual closure, in which case it won't be an AutoClosureExpr. I could be wrong. The @autoclosure tag on the second argument just means (I think) that Swift is allowed to automatically turn an expression in this position into a closure (AutoClosureExpr). So you can write:

x ?? 0

rather than:

x ?? { return 0 }

or something like that.

---

I'll add a BinaryExpr.getOperator (or something similar, maybe .getOperation?).

---

I agree about AutoClosureExpr.getExpr but I don't think I want to use AutoClosureExpr in this PR.

[geoffw0]

My understanding is that you can provide a manual closure, in which case it won't be an AutoClosureExpr.

... every doc I read backs me up that @autoclosure "lets you omit braces" or similar phrasing, but swiftc is not happy if I leave them in. So it appears you're right, @autoclosure actually means you must omit the braces, and you presumably always get an AutoClosureExpr as a result.

[geoffw0]

All points addressed, I think. There's no test coverage for the unary expr predicates though.

[geoffw0]

Issue for the missing CFG: github/codeql-c-team#1384

[MathiasVP]

LGTM!