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

  • What is your evaluation of the proposal?

+1

  • Is the problem being addressed significant enough to warrant a change to Swift?

Yes

  • Does this proposal fit well with the feel and direction of Swift?

Yes

  • 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?

Quick reading

This proposal reeks of buy-one-get-one-free :slight_smile: two separate ideas not necessarily each worthwhile to propose on their own, bundled together to appear greater than the sum of their parts.

The existing compiler diagnostic is already clear. This proposal presents no real value add. Even its motivation is foolhardy.

execute {
    let foo = self.doFirstThing()
    performWork(with: self.bar)
    self.doSecondThing(with: foo)
    self.cleanup()
}

If self. is such an eyesore, which I'm willing to concede, then decompose it.

execute {
    self.doIt()
}

Your doIt() method has no access to local state at the call site, which is the entire point of using closures.

3 Likes

You can pass the state you need as parameters to the method. If they are properties on self then you don't need to which simplifies your code, too, without any risk of retain cycles.

To put your position another way: if you don't need any of the conveniences of a closure, don't use one.

That's not really a good response to a proposal to make closures easier to use.

4 Likes

It’s quite forceful, and foolhardy, to make something a one-off func because self may escape.

Things escape:

  • to other threads
  • to owner of self
  • to event handler

The closure may even escape back-and forth.

// do some work
background.async {
  // background work

  main.async {
    // clean up work
  }
}

At best, you could make a func for each part,

background.async {
  self.backgroundWork()

  main.async {
    self.cleanUp()
  }
}

Still, the situation does not improve for more complex hopping-around, which async libraries love to do. I’d even argue that it gets worse now that you have a multitude of functions just to express a single sequence of asynchronous operations (not to mention naming hell one’d need to go through).

Granted, it may make sense for event handler to be instance’s function, but I personally find it more of a hindrance and legacy from the era of target-action pattern.

It’s... lacking, to say the least. It could be quite common to apply fix-it without much though, in which case would easily incur ref-cycle.

Speaking of, @Jumhyn, do you think we could add unowned variation of fix-it as well? That’d force user to think about how they really want to capture it.

I do agree still, that this could be separated into 2 proposals.

1 Like

.. unless the proposal makes closures harder to use correctly, which is my point in the first place.

Please refrain from characterizing others' arguments because your words do not at all represent mine, Avi.

It does not appear that you have made that point anywhere. Can you please elaborate: in what ways would the proposal make closures harder to use?

3 Likes

Please refer to SE-0269: Increase availability of implicit self in @escaping closures when reference cycles are unlikely to occur - #30 by Ilias_Karim

Could you please elaborate on your response? “-1,” “no,” and “somewhat” isn’t much to work with for people to engage in an exchange of ideas with you. What makes the proposed changes “harder to use correctly”?

5 Likes

I don't have super strong feelings about this, and so would rather see it addressed as a follow-up depending on the outcome of this review. The specific fix-its offered (adding an unowned version, removing the self. one, etc.) can be considered with further detail once it's decided whether the behavioral changes are even acceptable.

Those that have commented on the fact that there appear to be two separable features here aren't wrong—this was pitch with just the "explicit capture of [self] enables implicit self, with the value type part implemented and added to the proposal at a later point—but the two seemed closely tied enough that it makes sense to me to pitch them together. I'd encourage anyone that finds themselves split on the proposal to elaborate as to which components of the proposal they do/don't want and why rather than just stating that it's difficult to decide. The Core Team is free to accept this proposal in part if desired; reviewers need not provide just a simple thumbs up/down!

To be clear, I think that the current diagnostic is ineffective at accomplishing its goals. For codebases that opt to not use implicit self, the diagnostic is completely useless since code which captures self does not differ visually at all from code which does not capture self. This proposal doesn't improve things in this regard. For codebases that do use implicit self the diagnostic and proposed fix-it introduce a stylistic inconsistency that is already used for an entirely different purpose (resolving shadowed names). Capturing self in the capture list explicitly is IMO a much better indication of intent.

The teachability of this nuance is, to me, a better argument about the fix-its we decide to offer rather than the implicit self behavior itself. It's a perfectly valid option to allow implicit self in closures where self is captured explicitly, but maintain the existing error and fix-it so that users aren't introduced to capture lists before they're "ready". I'd be opposed to such a modification of this proposal for the reasons outlined above (I'd sooner want to ditch the "insert self." fix-it), but I'm curious to know if those concerned about the increased nuance would feel any differently if it weren't exposed to users as part of the diagnostic logic.

From a clarity POV, I think this is the most important part of the proposal. It provides clarity and intent to an action that's currently invisible, magic, and incongruous with the way code is written everywhere else. My guess is if the feature were being written today, the proposed approach would be favored over the current approach.

4 Likes

BTW [x = self.x] in can be shortened to just [x] in

I think people are already too self.-happy and need to actually think about what they're doing before they add a self., since for structs, capturing single fields is often the correct choice. Even though it doesn't make reference cycles, it's still capturing more than you need. Even the stdlib makes this mistake, and while this case is unlikely to cause issues (lazy maps are usually inlined so the compiler would be able to clean up the mess this causes) I would still rather not make it even easier for people to do this. Ideally for me the fix-it when trying to use a struct property would offer both adding self. and capturing just the property as options.

Based on that, I'm +1 on [self] in, -1 on unmarked struct capture unless we follow Jumhyn's idea of making unmarked struct capture capture single fields

1 Like

The other side of this is to note that the compiler has no ability to analyze source code in a way that detects whether it actually causes a reference cycle. Instead, it applies a heuristic that involves a fairly simple heuristic to decide whether the source code pattern correlates with patterns that create reference cycles.

Given that the heuristic isn't very reliable in absolute terms, there's an argument that says the compiler should get out of the business of advising developers on avoiding reference cycles.

More reasonably, the argument from that side says that the compiler should only give advice in those cases where it has a pretty good chance of being right.

It's possible to see this SE-0269 as a proposal to get the compiler to stop making unreliable proposals.

1 Like

I strongly disagree with this approach. Currently, the compiler is guaranteed to at least call your attention to places where a cycle could occur. Backing off to only call attention to places where the compiler can be sure means, with very little doubt, failing to call attention to all places where you are making a cycle.

That isn't so, although to be fair I don't think that's what you mean. There are plenty of places where a cycle could occur but the compiler doesn't draw your attention to them.

In particular:

    self.completion = {
        self.someProperty = 0 // <-- this definitely creates a reference cycle
    }

but the compiler isn't going to tell you.

Of course, I know I'm cheating, because there isn't even an implicit self in that example, but that's kinda my point: making this about implicit self is a pretty approximate solution.

2 Likes

I like the first part of the proposal, however as it stands I'd like to offer the idea that the proposal is heavily biased towards those that use implicit self..

As @lukasa previously indicated, some of us (probably many) who use Swift, use explicit self. exclusively. There's a couple of solid arguments we always fall back on at my company as to why, however, those aren't necessary to diverge into. Functionally the language allows for explicit self., reality is people already use explicit self., and this proposal only adds any value to those who use implicit self. not explicit self. where it could! Because if you already use explicit self. everywhere, then it does not on its own indicate a capture in a closure to the developer (or is at least ambiguous).

What I would prefer is a much stronger proposal, that under any circumstances, if an escaping closer is capturing a reference to self at all then [self] must be indicated in the capture list. This will eliminate any ambiguity for those who prefer explicit self, that self is captured, and force the programmer to address whether or not self should be weak. This preferred proposal is more Swifty given that it provides "Clarity at the point of use" and acts as a forcing function towards correct behavior i.e. safer.

Though, this sort of change will be source breaking it could start as a warning and evolve into a hard requirement over time.

4 Likes

I meant more along the lines of

    self.completion = {
        someProperty = 0 // <-- this definitely creates a reference cycle (no explicit use of self)
    }

Places where you might create a reference cycle currently tend to have self nearby.

@rex-remind Such an idea is discussed in the Alternatives considered section, and was not pursued for exactly the reason that you note (it would be majorly source breaking for almost every decently sized codebase). Starting with this proposal does not close off that direction, and may even make it easier to adopt down the line if "capture [self] explicitly" becomes the default resolution strategy.

1 Like

I don't agree that the proposal presents no real value. Implicit self after explicitly declaring [self] is intuitive for me, and it is up to you to use it or not so there is no net loss.

My point is the that the second half of the proposal adds an extra cognitive burden, where you need to be aware of types so you can adjust which code you need to write.