SE-0269: Increase availability of implicit self in @escaping closures when reference cycles are unlikely to occur

Evolution Proposal Reviews
61

Indeed, should have read that section. I fully agree with your assessment, and now that I’ve read that section I +1.

  • What is your evaluation of the proposal?
    +1
  • Is the problem being addressed significant enough to warrant a change to Swift?
    Yes, and I am absolutely in favor of a follow on that enforces captured self universally for references in escaping closures.
  • Does this proposal fit well with the feel and direction of Swift?
    Yes, clarity at point of use.
    Additionally I am far more strongly in favor of the first than second section. +.1 on the second section.
  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
    N/a
  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
    Read the proposal, read all the forum threads.
62

Even though it doesn't make reference cycles, it's still capturing more than you need.

I'm not sure it works as you think. Whether you write self or not the compiler will synthesize that anyway if you are accessing properties and methods.

63

This seems to work as I thought, what do you think this should have done?

class DeallocationDetector {
	var i: Int
	init(_ i: Int) { self.i = i }
	deinit { print("Detector \(i) deallocated!") }
}

struct Test {
	var detector: DeallocationDetector
	var variable: Int
	func selfCapture() -> () -> () {
		return { print(self.variable) }
	}
	func varCapture() -> () -> () {
		return { [variable] in print(variable) }
	}
}

do {
	var a: () -> ()
	var b: () -> ()
	do {
		let test1 = Test(detector: .init(1), variable: 5)
		let test2 = Test(detector: .init(2), variable: 6)
		a = test1.selfCapture()
		b = test2.varCapture()
	}
	a()
	b()
}

Prints:

Detector 2 deallocated!
5
6
Detector 1 deallocated!

Assuming you aren't relying on delaying deinits to demo things, you probably want to do varCapture but everyone's favorite compiler fix-it does selfCapture

64

Ah, sorry I thought you were talking about self. usage inside closures, not the capture list.

65

Boohoo, wish I hadn't read this proposal. Now I want to just capture { [self] and profit from implicit self everywhere!

I follow much of the analysis performed by @Jumhyn. I agree with others that the first part of the proposal is - by far - the strongest. It's actually brilliant. Once you see it, you can't forget it. I do not disagree with the second part, but its impact on the UX of Swift is much weaker.

7 Likes
66

IME the amount of times I've needed to capture a copy of a Type's Property without the full Type is relatively rare, and the times it would matter are even less common.

The main reason being IMO that kind of operation is better expressed as a "pure" function that doesn't directly involve a Type's instance in the first place.

struct Test {
    var variable: Int
    var printer: () -> () { Self.makePrinter(for: variable) }

    // static to allow use by others and ensure internal use doesn't depend on `self`
    static func makePrinter(for value: Int) -> () -> () {
        return { print(value) }
}
67

yes, we agree

68

+1 for the first part of the proposal: enable omitting self. when it is in the capture list. Personally, I think a scenario where explicit self. feels too unpleasing is when it has to be used with property-wrapper projections. Projecting a wrapped property within a closure requires doing self.$property.

I am also concerned about omitting explicit self with all value types, regardless of whether they are trivial types i.e., types that do not store any references, or not. However, the compiler does know whether a type is trivial or not. So perhaps the compiler could stop emitting errors if a trivial self is implicitly captured within a closure. This can be progressively relaxed to support known COW types like Arrays, strings, dictionary etc.

In essence, this gives people who are developing embedded DSLs or the like some freedom and leaves it more of a choice for others.

3 Likes
69

I think this is a big positive.

Anytime you are trying to bring a user's awareness to an issue, it is an extremely fragile effect. Over-warning is almost worse than no warning at all because it causes people to blindly apply the fix without thinking about it, and actually start ignoring real warnings. Boy who cried wolf.

Ideally, it should only require self. in cases where a cycle could actually occur.

70

Proposal Accepted

This proposal has been accepted. The proposal discussion roughly divided into two parts:

  1. Discussion around hoisting the explicit mention of self into the closure capture list as the alternative to repeatedly writing explicit self in a closure body (and having the compiler fix-it encourage that style)

  2. The expansion of implicit self around the specific case where the type of self is a value type

The discussion on both topics was deeply constructive and productive. The core team wants to express their deep thanks to everyone who contributed to this discussion. Some really fantastic insights were made from different perspectives.

In the end, the core team felt that the hoisting self in the closure capture list provided a better experience where explicit self will still be encouraged as it (a) more clearly captures the intent of explicitly mentioning self and (b) syntactically will be cleaner in the cases where self is uttered multiple times.

The discussion around the expansion of implicit self was a bit more fragmented on the review, but ultimately the core team sided with expanding the use of implicit self as proposed in SE-0269.

Thank you to everyone who participated in this review!

14 Likes
71

Just curious, shouldn’t the long version of rationale be in the separated thread? (This being much longer threw me off a little bit)

2 Likes
72

We’ve been inconsistent about this. I personally think the rationale should be where the actual review conversation took place. I’ll discuss the consistency of our process here with the rest of the core team.

4 Likes
73

I had been writing some packages on top of NIO using 5.3 snapshots and today I configured CI which had Swift 5.2 on it. I realized that some of the packages I wrote were accidentally incompatible with 5.2 because I (unintentionally) missed self. inside closures. I probably should have setup the CI sooner but I was wondering if this change should be gated with Swift language version to avoid the ecosystem impact this change might end up causing.

74

We historically haven't had an equivalent of Clang's -Wc++98-compat to warn you when you're using a Swift-5.3-only feature, even when it's a relaxing of existing rules rather than an introduction of new syntax. The thought here has been that there's no reason to have one codebase work with multiple Swift compilers because of forward source compatibility and the ease of updating, but it's come up repeatedly in practice because of upgrade periods (such as Xcode betas) and because keeping an older version of Swift around is a pain. I don't think this specific proposal should be changed, but maybe a best-effort "warn on incompatible features" flag is worth designing.

5 Likes
75

@Jumhyn

As I see in the proposal you wrote, in the detailed design section you mention this case

execute { [self] in
    execute { // Fix-it: capture 'self' explicitly to enable implicit 'self' in this closure.
        x += 1 // Error: reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit. Fix-it: reference 'self.' explicitly.
    }
}

But now I put the self inside the capture list of a PromiseKit call, the second PromiseKit call which is inside the first one (as the example above) doesn't warn me that I have to add .self.

Is this working for any closure inside the first closure with self in the capture list?

Great suggestion by the way!

1 Like
76

Ha! Great catch, @mvorisis. This code was definitely intended to produce an error, but I can confirm that the exact code from the proposal compiles just fine. Could have sworn that it was working at the time of acceptance, so maybe this bug crept in in the months since... in any case, should have added a test. :man_facepalming:

I've filed SR-14120 to track this issue, but at this point it would be a source-breaking change so I'm uncertain whether concern about potential cycles here will rise to the level of justifying the source break. Thank you for calling this out!

2 Likes
77

Ok what about using it like this?

execute { [self] in
    execute { [self]
        x += 1 
    }
}
78

There should be an "in" after the second [self] but yes, this is the form that the proposal was intended to enforce for nested escaping closures.

2 Likes