Swift: Add variable-capture flow

[MathiasVP]

Commit-by-commit review recommended. I've kept most of Nora's commits, but note that many of the things added in those commits are actually changed by my own commits afterwards 😂.

DCA looks fine 🎉.

The QLDoc check fails because the shared consistency queries don't have any QLDoc. This will only fail this one time because this PR is introducing those consistency queries.

[aschackmull]

With the declared type of e2 being Expr, does this even work? IIRC this was a problem when we debugged this together and this disjunct should perhaps just be folded into the case above so we can step from Expr to Expr.

[MathiasVP]

Note that e2 is of type CaptureInput::Expr, and CaptureInput::Expr is actually AstNode (which includes Pattern).

[aschackmull]

Yes, that's why compilation doesn't fail. I'm asking whether Expr and Pattern might be sufficiently disjoint (even though both are subtypes of AstNode) that this will block flow?

[MathiasVP]

I think I'll leave this to another PR. I've discovered that the WriteDefinition::assigns predicate doesn't really do the right thing at all. For a variable declaration such as:

var x = 0

the WriteDefinition::assigns(value) predicate holds where value is the left-hand side of the declaration (i.e., the lvalue of x) instead of the right-hand side (i.e., 0). Once that's fixed (which I'll do in a later PR) we can remove this disjunct and have CaptureInput::Expr be Expr.

[geoffw0]

Thank you for your review @aschackmull . I've added a few comments but I think you have more insight here than I do.

I'm pleased to see these changes fix multiple [NOT DETECTED] and MISSING results from existing tests, which suggests things are working well enough to make a difference.

I didn't bother investigating captures through constructors in this PR

Is there an issue for / test showing this?

[geoffw0]

This result surprises me. Do you have any idea what's gone wrong here?

[MathiasVP]

Yes. We're missing call contexts for the call above:

forward(arg: source(), lambda: {
  (i: Int) -> Int in
    return 0
})

so we actually get flow like:

source() (i.e., the argument) -> arg (i.e., the parameter of `forward`) -> arg (i.e., the argument of lambda(arg)) -> i (the parameter of on line 123) -> i (the return on line 124) -> clean

There can be two explanations of this:

• Swift doesn't implement dispatch through call contexts (see mayBenefitFromCallContext and viableImplInCallContext in DataFlowDispatch).
• The call contexts exists, but is being thrown away because there's an issue with an enclosing callable. I actually believe this is the case based on @aschackmull's feedback here.

I'll look into this as well.

[MathiasVP]

This was fixed in 2465cc2 (the answer was indeed a version of case 2).

[geoffw0]

We've lost two good results in that change - and they look like fairly straightforward ones, unless I'm missing something?

[MathiasVP]

We're losing this one:

func setEscape() {
  let x = source("setEscape", 0)
  escape = {
    sink(x) // $ MISSING: hasValueFlow=setEscape
    return x + 1
  }
}

because we don't model global flow in Swift. So we don't manage to flow from the definition of escape to the place where it's actually called:

func callEscape() {
  setEscape()
  sink(escape?()) // $ MISSING: hasTaintFlow=setEscape
}

In an earlier commit we accidentially got this flow because we flowed from let x = source("setEscape", 0) and straight into the closure (which violates all kinds of dataflow invariants), but when I fixed that in 2465cc2 the result disappears again, and we're only gonna get it back if we implement global flow in Swift.

For this one:

func captureList() {
  let y: Int = source("captureList", 123);
  { [x = hello()] () in
     sink(x) // $ MISSING: hasValueFlow=hello
     sink(y) // $ MISSING: hasValueFlow=captureList
  }()
}

I feel like we should be able to catch sink(y) (but for some reason we still don't). We don't catch the sink(x) one because we're missing an SSA definition for x = hello() and that work is blocked on #14567 (which I've promised Paolo to look at after this is merged).

[MathiasVP]

Yep I'm comfortable calling the the above an issue with capture lists. I've added a version of this test in 65e13aa without a capture list, and that works fine. So I think we should delay further work on this until #14567 is merged.

[MathiasVP]

I didn't bother investigating captures through constructors in this PR

Is there an issue for / test showing this?

For sure not an issue. I haven't checked if we have a test for it (I suspect not, but I'll add one before this PR is merged).

[geoffw0]

Forgot to add, DCA LGTM as well.

[MathiasVP]

For sure not an issue. I haven't checked if we have a test for it (I suspect not, but I'll add one before this PR is merged).

I've now investigated this. It seems like this is simply irrelevant for Swift because of fantastic language design choices 😂 (see b41ec37 for more details).

[MathiasVP]

@geoffw0 @aschackmull I believe I've addressed all review comments. There are a few unresolved ones (#14577 (comment) and #14577 (comment)) that are blocked on work that I don't want to include in this PR (but which we now have issues/other ongoing PRs for).

[aschackmull]

SGTM. I'll leave approval to @geoffw0 as I haven't looked at the tests at all.

[geoffw0]

Yes I'm happy. There's quite a bit of follow-up work but I agree it's time to get this merged and start on incremental improvements.

[geoffw0]

🎉