Allow implicit self in escaping closures when self usage is unlikely to cause cycle

[Jumhyn]

Update the lookup logic for closures to allow resolution of implicit self to resolve to any direct captures of self (e.g. { [self] in ... }) in closures. Also introduce new diagnostic logic to suggest such a capture as a fix-it for implicit self in an escaping closure.

This is the implementation for an incoming Swift Evolution proposal.

Resolves SR-10218.

[gregomni]

This is awesome, just a few suggestions.

[Jumhyn]

Hi @gregomni, thanks for the comments! I’ll have a chance to make changes re: your feedback this weekend.

[beccadax]

This looks pretty good! I have some minor suggestions, but they're mostly stylistic, plus a couple of edge cases.

[beccadax]

If you fix the merge conflict and @ me, I'll kick off tests.

[Jumhyn]

@brentdax Implementation updated and conflicts resolved. Made some new changes in lib/Parse/Lexer.cpp to better handle the bracket cases, so might want to give some thought there as to whether it will affect anything that I didn't consider. I think the change should be safe, and tests all run locally for me, but it's a member function with a lot of clients so there may be an edge case that I'm not thinking of.

[beccadax]

Very minor suggestions, but I'm happy with this overall!

[Jumhyn]

@brentdax Thanks for the comments! Sorry it took me a bit to get back to you; I had to update the implementation to integrate with the new AST scope lookup system. I have a working implementation now, so once I polish that up I’ll push it up and add the tests you’ve called out!

[theblixguy]

To be honest I just created it because that's what Ted/Doug were trying to do. You can use it in Xcode to check if everything works and behaves as expected I guess.

That's weird - I don't get such an error (I am using Xcode 11.3 on macOS 10.15.1). The toolchain needs to be placed in /Library/Developer/Toolchains. Is that where you’ve placed the xctoolchain file?

[Jumhyn]

Ah, gotcha. @tkremenek @DougGregor this is passing CI now and is ready to merge on my end!

[beccadax]

It looks like you haven't run full tests recently, so just to be conservative, I'll do a test-and-merge.

@swift-ci please test and merge

[Jumhyn]

@brentdax Should the test-and-merge job be reflected in the check statuses below? I'm not seeing anything change (still just the smoke test and build toolchain jobs).

[theblixguy]

It will update soon, it's running: https://ci.swift.org/view/Pull%20Request/job/swift-PR-merge/2194/

[theblixguy]

Infrastructure problems... let’s try again:

@swift-ci please test

[theblixguy]

@swift-ci please test Linux

[theblixguy]

@swift-ci please test macOS

[Jumhyn]

@brentdax Looks like the tests @theblixguy kicked off succeeded finally, but this still needs one last nudge to actually merge it :)

[theblixguy]

Let's run source compatibility tests as well, just to be extra sure. If that passes too, we can merge this:

@swift-ci please test source compatibility

[Jumhyn]

Thank you @theblixguy! For future reference, are these all steps that I could have run myself (i.e., was I bothering you two needlessly)?

[theblixguy]

You need commit access to run tests on CI, but I am happy to run them for you if needed! For testing locally, there's a doc: https://github.com/apple/swift/blob/master/docs/Testing.md (instructions for running source compat suite locally is here: https://github.com/apple/swift-source-compat-suite).

[theblixguy]

@swift-ci please test source compatibility debug

[theblixguy]

@swift-ci please test source compatibility release

[theblixguy]

Okay, all tests are passing so let's . Thank you very much for your contribution @Jumhyn! Could you please mark the JIRA ticket as resolved? Also, I think it needs to be added to the changelog.

@brentdax can this be cherry-picked to 5.2 branch or is it too late?

[phlegmaticprogrammer]

Oh it would be great if this could end up in 5.2. I have a DSL that would be much much nicer to use with this.

[theblixguy]

@Jumhyn Do you mind adding this to the changelog and also update the proposal as "Implemented" on Swift evolution repo?

[Jumhyn]

@theblixguy Sure thing. Do you know what heading it should be put under in the changelog since it missed 5.2?

[theblixguy]

It should be under “Swift Next”. Also, I’m sorry for suggesting to update the proposal, since we don’t know which version of Swift this will end up in, its best to keep the current status as-is.

[Jumhyn]

Great, done. Once this makes it into a Swift release I'll update the swift-evolution PR with the version number so that it can be merged in.