[SE-0365] Allow implicit self for `weak self` captures #40702

calda · 2021-12-24T16:21:38Z

This PR adds support for implicit self for weak self captures, as long as self has been unwrapped.

This implements SE-0365.

For example:

{ [weak self] in
    guard let self else { return }
    dismiss() // equivalent to `self.dismiss()`
}

…wrapped

…ted as an invalid implicit self

…escaping () -> String = String()'

…t self from an outer closure

…ermit implicit self

test/expr/closure/closures.swift

lib/Sema/PreCheckExpr.cpp

lib/Sema/MiscDiagnostics.cpp

test/expr/closure/closures.swift

compnerd · 2022-07-18T17:03:57Z

@swift-ci please smoke test

compnerd · 2022-07-18T17:04:04Z

@swift-ci please build toolchain Windows platform

xedin

LGTM! I have left two tiny comments but they can be addressed in a follow-up as well.

xedin · 2022-09-29T17:39:22Z

lib/Sema/TypeCheckNameLookup.cpp

@@ -327,7 +332,7 @@ LookupResult TypeChecker::lookupMember(DeclContext *dc,
  dc->lookupQualified(type, name, subOptions, lookupResults);

  for (auto found : lookupResults)
-    builder.add(found, nullptr, type, /*isOuter=*/false);
+    builder.add(found, nullptr, nullptr, type, /*isOuter=*/false);


Nit: Please add /*baseDecl=*/ before new nullptr.

xedin · 2022-09-29T17:39:58Z

lib/Sema/MiscDiagnostics.cpp

+        }
+      }
+
+      if (conditionalStmt == nullptr) {


Nit: if (!conditionalStmt)

xedin · 2022-09-29T17:40:59Z

@swift-ci please test

xedin · 2022-09-30T00:33:20Z

@swift-ci please test Linux platform

xedin · 2022-09-30T17:31:24Z

Thank you for the great contribution, @calda!

calda · 2022-09-30T17:33:45Z

Thank you for the quick reviews and thorough feedback @xedin! Really appreciate it. This was my first non-trivial compiler change so your input was really helpful.

DougGregor · 2022-10-04T20:52:57Z

CHANGELOG.md

@@ -3,6 +3,48 @@ CHANGELOG

 _**Note:** This is in reverse chronological order, so newer entries are added to the top._

+## Swift 6.0


Minor nit: this should go under Swift 5.8, since part of the feature is there, and we aren't release "Swift 6.0" any time soon.

xedin · 2022-10-04T21:48:26Z

@calda The language workgroup discussed the problem and came to the conclusion that the compiler should:

Keep existing lookup behavior in Swift 5 (so no changes there);
Produce a tailored warning about guard let and if let statements that introduce self would be used in Swift 6 mode but isn't in Swift 5 - it has to describe the issue and avoid the fix-it to remove the statement. (We can brainstorm on the actual text).
Produce a new warning in < Swift 6 modes that detects an incorrect use of implicit self in non-escaping closures and suggests to add guard let statement if there isn't one.

Would you be interested in working on changes?

calda · 2022-10-04T21:51:09Z

Sure! I can work on these changes

calda · 2022-10-05T01:27:42Z

Produce a tailored warning about guard let and if let statements that introduce self would be used in Swift 6 mode but isn't in Swift 5

In what situations should this warning be emitted, and how would the warning be silenced?

xedin · 2022-10-05T04:03:08Z

In what situations should this warning be emitted, and how would the warning be silenced?

The same situations where the warning about "unused self" had to be suppressed in Swift 5 mode because it picks self from [weak self] instead of guard let/if let statements.

calda · 2022-10-05T04:13:17Z

If we want a warning and this case:

closure { [weak self] in
  // warning: self is not unwrapped, add `guard let self else { return }`
  method() 
}

and this case:

closure { [weak self] in
  // warning: guard condition is unused in Swift 5, but used in Swift 6
  guard let self else { return }
  method() 
}

then how do you write a weak self closure without triggering a warning?

It seems like we'd want a way to suppress the first warning without introducing a new warning. Thoughts?

xedin · 2022-10-05T04:14:22Z

Probably the only way is to write explicit self. in this case?

calda · 2022-10-05T04:20:23Z

Should the fix-it suggest using explicit self instead of adding a guard statement?

xedin · 2022-10-05T04:22:24Z

As far as I understand the guard/if let statements have to stay to produce self unwrap for the Swift 6 mode, so all the fix-it would do is to add self. which is going to silence the warning in Swift 5 mode and not going to break code in Swift 6 mode. Does that make sense?

calda · 2022-10-05T04:34:51Z

Do we need a warning at all in this case, since the code is well-formed in Swift 6?

closure { [weak self] in
  guard let self else { return }
  method() 
}

Allowing this without a warning may be better than having multiple warnings that needs to be suppressed in stages.

xedin · 2022-10-05T04:37:09Z

This warning should be emitted only for language modes < 6 because in 6 and on it would pickup the right self :) We just need to warn people that okay previous the behavior here wasn't ideal and we are going to change it.

calda · 2022-10-05T04:46:27Z

Sorry, I just want to make sure any new warning we add here has an actionable fix-it that silences it.

For the warning in this example, what fixit would we use?

closure { [weak self] in
  // Warning: guard statement condition is unused in Swift 5 language mode
  guard let self else { return }
  method()
  otherMethod()
}

Would we emit the warning any time there's an implicit self reference that had its lookup behavior changed by Swift 6? e.g. would this case emit a warning?

closure { [weak self] in
  // warning?
  guard let self else { return }
  self.method()
  otherMethod() // still implicit
}

(the guard statement is not unused, but there's still an implicit self call that has its behavior changed in Swift 6)

xedin · 2022-10-05T04:50:48Z

Would we emit the warning any time there's an implicit self reference that had its lookup behavior changed by Swift 6?

Yes, guard statement gets one warning and both method() and otherMethod() would emit a warning and ask to add explicit self.. In other later case only otherMethod() would get a warning.

xedin · 2022-10-05T04:56:21Z

We could also go in a direction where guard let self and if let self trigger a special diagnostic walker that only emits a single warning attached to the statement and a bunch of notes with fix-its per incorrect use of self, so your first example would be diagnosed something like this:

error: declaring self in guard statement has to effect on implicit self use in this closure, this behavior is an error in Swift 6
guard let self else { return }
          ^~~

note: use of weak implicit 'self'
   method()
   ^
   self.

note: use of weak implicit 'self'
   ohterMethod()
   ^
  self.

calda · 2022-10-05T04:56:55Z

Ok, this makes sense to me -- we emit a warning for each these implicit self calls that will have their behavior changed with a fix-it to use explicit self. This also silences the "unused guard statement" warning, since that is no longer unused after applying the fixits. Does that sound right?

xedin · 2022-10-05T04:57:56Z

Take a look at my previous comment (i might have posted it while you where writing this message), you might find that a more interesting approach which reduces number of warnings produced.

calda · 2022-10-05T05:06:53Z

Thanks for the in-depth discussion! I think your suggestion of having a single warning with multiple notes would work really nicely here.

xedin · 2022-10-05T05:07:35Z

Absolutely! Let's try to make it happen then, if it turns out too hard there is always a fallback to multiple warnings.

calda · 2022-10-09T19:22:15Z

Here you go: #61513 -- turned out to be pretty straightforward!

calda added 6 commits December 23, 2021 11:01

Unconditionally permit implicit self in weak closures

4eee6f8

Fix issue where implicit self wasn't rejected before self is unwrapped

2292e38

Fix edge cases

30f07e1

Improve diagnostic for invalid use of implicit self before self is un…

b1ebd95

…wrapped

Update tests

d909365

Fix crash

5f8e567

calda mentioned this pull request Dec 24, 2021

Add proposal for allowing implicit self for weak self captures swiftlang/swift-evolution#1506

Merged

calda changed the title ~~Support implicit self for weak self captures~~ Allow implicit self for weak self captures Dec 24, 2021

calda added 9 commits December 24, 2021 16:34

Fix some test failures

72074b3

Fix issue where '@autoclosure () -> String = String()' would be rejec…

854f024

…ted as an invalid implicit self

Fix more cases, write more test

a5b3312

Reject normal ValueDecls but allow ConstructorDecl from '@autoclosure @…

e21b4d7

…escaping () -> String = String()'

More test fixes

66c89f1

Fix another test failure, from a defer FuncDecl

6c682b7

Always emit an error for invalid inner closures trying to use implici…

6fe7dc5

…t self from an outer closure

Make more tests pass

c6b4a20

Fix issue where 'let = MyCls()' in function body would unexpectedly p…

0cd9f19

…ermit implicit self

CodaFi reviewed Dec 26, 2021

View reviewed changes

test/expr/closure/closures.swift Outdated Show resolved Hide resolved

CodaFi reviewed Dec 26, 2021

View reviewed changes

lib/Sema/PreCheckExpr.cpp Outdated Show resolved Hide resolved

lib/Sema/MiscDiagnostics.cpp Outdated Show resolved Hide resolved

test/expr/closure/closures.swift Outdated Show resolved Hide resolved

calda added 7 commits December 26, 2021 08:47

Fix incorrect DeclContext check

0a5e4e0

Tests pass, except for 'capture 'y' was never used' issue

20ed27a

Use LabeledConditionalStmt instead of just IfStmt / GuardStmt

c03950b

Move weak self errors to their own file

ccdf807

Put back unused capture warnings

369e20f

Update comments

0f33636

Fix last test failure

367b112

xwu added the swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review label Jan 6, 2022

Merge branch 'main' into cal--implicit-weak-self

778a2fc

xedin approved these changes Sep 29, 2022

View reviewed changes

xedin merged commit 1a79d93 into swiftlang:main Sep 30, 2022

DougGregor reviewed Oct 4, 2022

View reviewed changes

calda added a commit to calda/swift that referenced this pull request Oct 9, 2022

Implement PR feedback from swiftlang#40702

8cbff9e

calda added a commit to calda/swift that referenced this pull request Oct 9, 2022

Implement PR feedback from swiftlang#40702

0a60e56

calda mentioned this pull request Oct 9, 2022

Warn on incorrect usage of implicit self in non-escaping closure that captures self weakly #61513

Closed

calda mentioned this pull request Oct 10, 2022

Enable SE-0365 behavior in Swift 5 mode #61520

Merged

calda mentioned this pull request Oct 22, 2022

Update SE-0365 to discuss source compatibility implications swiftlang/swift-evolution#1819

Closed

		@@ -3,6 +3,48 @@ CHANGELOG

		_Note: This is in reverse chronological order, so newer entries are added to the top._

		## Swift 6.0

[SE-0365] Allow implicit self for weak self captures #40702

[SE-0365] Allow implicit self for weak self captures #40702

Uh oh!

Conversation

calda commented Dec 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

compnerd commented Jul 18, 2022

Uh oh!

compnerd commented Jul 18, 2022

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

xedin Sep 29, 2022

Choose a reason for hiding this comment

Uh oh!

xedin Sep 29, 2022

Choose a reason for hiding this comment

Uh oh!

xedin commented Sep 29, 2022

Uh oh!

xedin commented Sep 30, 2022

Uh oh!

xedin commented Sep 30, 2022

Uh oh!

calda commented Sep 30, 2022

Uh oh!

DougGregor Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

xedin commented Oct 4, 2022

Uh oh!

calda commented Oct 4, 2022

Uh oh!

calda commented Oct 5, 2022

Uh oh!

xedin commented Oct 5, 2022

Uh oh!

calda commented Oct 5, 2022

Uh oh!

xedin commented Oct 5, 2022

Uh oh!

calda commented Oct 5, 2022

Uh oh!

xedin commented Oct 5, 2022

Uh oh!

calda commented Oct 5, 2022

Uh oh!

xedin commented Oct 5, 2022

Uh oh!

calda commented Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xedin commented Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xedin commented Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

calda commented Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xedin commented Oct 5, 2022

Uh oh!

calda commented Oct 5, 2022

Uh oh!

xedin commented Oct 5, 2022

Uh oh!

calda commented Oct 9, 2022

Uh oh!

Uh oh!

[SE-0365] Allow implicit self for `weak self` captures #40702

[SE-0365] Allow implicit self for `weak self` captures #40702

calda commented Dec 24, 2021 •

edited

Loading

calda commented Oct 5, 2022 •

edited

Loading

xedin commented Oct 5, 2022 •

edited

Loading

xedin commented Oct 5, 2022 •

edited

Loading

calda commented Oct 5, 2022 •

edited

Loading