[Async Refactoring] Support Obj-C style bool flag checks

[hamishknight]

Add an additional case to CallbackCondition to support boolean checks, and add the necessary classification logic to determine whether a bool flag check is for a success or failure path.

The logic currently first checks to see if the async alternative has a convention that specifies which parameter the flag is, and whether it's a success or failure flag. If so, it classifies the flag check accordingly. If there's no async convention specified, we use a heuristic to see if the error parameter is force unwrapped in the failure block. If so, we treat that as the error path. If there's no force unwrap of the error, we leave the condition unhandled.

rdar://74063899

[bnbarham]

Thanks for the cleanup as usual 🙇. No major comments

[bnbarham]

See comment above - is there a good reason not to handle this case? Ie. if the force unwrap is used at all in the scope (nested or not), treat it as the error condition?

[hamishknight]

The problem is that would count for things like this:

if !success {
  func doThings() {
    print(err!)
  }
  if err != nil {
    doThings()
  }
}

Where the !success condition doesn't appear to be relevant to the force unwrap. We could probably perform a more in-depth heuristic that also looks for other references to err, but I'm not sure whether or not that's worth it. What do you think?

[bnbarham]

Makes sense, I wouldn't expect that case to be super common though. Another would be returning multiples bools and checking each both (nested). Though again, I assume that's uncommon. Erring on the side of leaving it and adding placeholders is probably okay for now though 👍

[bnbarham]

I agree kind of silly, but this sort of implies that success is actually error. We could handle it as such... Thoughts?

[hamishknight]

Hmm, the problem is that we're only classifying it as a success flag to begin with because of the force unwrap of the error in the body, so even if we classify both checks as success paths, the code still doesn't make much sense. We could apply a more general heuristic that allows for bool condition checks if they're also with other conditions that are able to classify as success or failure, but I don't really have a good sense of how often that comes up.

On a related note, I've just noticed that if the force unwrap of the error in this example was in an else block here, we would still classify the condition as the same, which doesn't seem right. It seems we should adjust the classification logic so we make the determination of whether the flag is a success check or failure check based on the branch in which the force unwrap of the error occurs rather than the other way around, which would let us classify a flag as an error check if we then force unwrap the error.

[bnbarham]

Probably a couple of problems here really. Yes, ideally if the force unwrap was in the else we would then consider success to be a flag. Could get a bit tricky when the else can itself be another if though.

Another problem is that if we don't classify the success as being a flag then there are unhandled conditions and so we don't remove the if at all. Ideally we'd classify the whole if as being in the success block and then remove the && err == nil. But that's an existing issue so... 🤷

[hamishknight]

Updated to handle the unwrap case, and changed the flag classification logic to consider the branch the force unwrap happens in, rather than assuming the flag always denotes success (and with that change, it didn't seem like we really needed the check to discard the flag if it disagrees with the rest of the conditions). Also noticed we were missing a couple of getSemanticsProvidingExpr calls, added those in.

[bnbarham]

One final comment, but we can do that in another PR if we want it.

[bnbarham]

For both a and f we could just assume that if there's a force unwrap, that == the error branch (only where the conditions aren't explicitly success/error and involve known params). I'm happy with this as is though, just thought I'd mention it.

[hamishknight]

Yeah it's possible we'll want to generalise the heuristic to better handle cases like these, though I'm not really sure how often they'll come up in practice as it seems a little odd to e.g check a success param for non-nil and then assume you're on an error path without looking at the success param again. I think for now at least I'd rather tend on the side on keeping the heuristic a little more limited and seeing what kind of feedback we get from users.

[hamishknight]

@swift-ci please smoke test