Guarded closures


(Matthew Johnson) #5

@Maik639 I addressed this use case in the draft I wrote by introducing a ? prefix sigil for instance methods of classes.


(Matthew Johnson) #6

@gwendal.roue I agree. If we had to choose one or the other I would also choose rebinding. I don’t feel too strongly about guarded closures anymore but there was enough interest in the topic that it deserves a dedicated thread. I see the basic proposal as syntactic sugar that builds on top of self rebinding.

The direction I had been exploring goes a bit further and attempts to solve related problems that self rebinding does not address. As I mentioned in the opening topic, I am less confident in that being worthwhile than I was a year ago.


(Matthew Johnson) #7

I agree that the decision about capturing should happen at the point of use. Nothing discussed here changes that. I don’t have a strong opinion on the topic but do want to make it clear that usage sites would remain in full control of every capture.

It is relatively unusual to want to extend the lifetime of an object by capturing a strong reference when using many async APIs. Accidentally doing so is a common source of memory leaks and bugs in Swift code. It may be easier for some programmers to avoid such bugs if such APIs were able to invert the “default” capture mode. The inverted default would be visible at the usage site in order to ensure readers understand what is happening. An example of an API that might choose to use this feature is an API for performing network requests.


(Tony Allevato) #8

In general I’m not opposed to optimizing for common cases, but if we move forward with self rebinding, I have trouble rationalizing guarded closures. My biggest concerns with the concept are:

  • A syntax like [guard self] or some similar attribute combines two very different concepts—memory ownership and control flow—into a single keyword. That’s an oddly specific and heavy weight to add to the language.
  • This feature only works for a subset of possible closures—those which return Void (or possibly Optional, as mentioned above).

That’s quite a bit of magic to avoid writing guard let self = self else { return }.


(Boris Triebel) #9

I also think that a) self rebinding is a preferable solution to a part of the described problem and b) using the keyword guard in two places in different ways works against the consistency of the language.


(Andrés Cecilia Luque) #10

IMHO here we are mixing two different proposals that solve different issues, and there is no need for opting for one or the other (I would love to have both):

a) Allowing shadowing and rebinding of self [recent discussion, old evolution proposal, recently merged implementation] (please note that this proposal is not being discussed here, I am just considering it in order to explain why I think it is different): it intents to modify the usage of the self keyword. This is an important change for the language, and not syntactic sugar, because (as I understood) it basically proposes to stop treating self as a reserved keyword and start treating it as any other identifier. This allows to unwrap self by reasigning it, instead of requiring a new identifier:

Before:
    let closure: () -> Void = { [weak self] in
        if let strongSelf = self {
            strongSelf.found = true
        } else {
            strongSelf.found = false
        }
    }

After:
    let closure: () -> Void = { [weak self] in
        if let self = self {
            self.found = true
        } else {
            self.found = false
        }
    }

b) Introducing a guard keyword in a capture list [recent discussion, Unofficial evolution proposal]: this introduces a guard keyword as part of the capture list of a closure, which is not only intended to be useful when capturing self, but can also be used to capture any other identifiers:

Before:
    let closure: () -> Void = { [weak self] in
        guard let strongSelf = self else {
          return
        }
        strongSelf.intProperty += 1
    }

After:
    let closure: () -> Void = { [guard self] in
        self.intProperty += 1
    }

This is syntactic sugar for a specific closure usage case, but it is worth considering because this usage case appears a lot. In order to be eligible to use guard in a capture list, the closure has to satisfy that:

  1. The closure intents to capture one or several properties weakly.
  2. The code inside the closure is only intended to be executed if all the weakly captured properties are not nil.
  3. The closure returns Void.

The rationale I have in mind for this proposal is extremely similar to the rationale given for the recently accepted toggle() proposal:

  • commonality: the need to unwrap a weakly captured identifier is a common one across multiple different problem domains

  • readability: the addition of this command can significantly improve readability for the cases in which it is possible to use it (which are many, despite the requirements that the closure has to comply with)

  • consistency: while capturing weakly and then unwrapping is fairly trivial, it is good to have a consistent way of doing this across all swift codebases, rather than having multiple variants (with multiple variants, I mean that the identifier given when unwrapping may be different in every case. For example, when unwrapping self, developers assign the new strong reference to indentifiers such as weakSelf, strongSelf, ss, sSelf, wSelf...)

  • correctness: being able to give a different identifier for the unwrapped and weakly (optional) captured object, can lead to this not-so-correct code (which eventually can lead to bugs):

      let closure: () -> Void = { [weak self] in
          guard let strongSelf = self else {
            return
          }
          strongSelf.intProperty += 1
          self?.anotherIntProperty += 1
      }

The Future Of [weak self] Rebinding
(Xiaodi Wu) #11

A few issues:

  • First, your arguments about inconsistency across code bases now go away because guard let self = self else { ... } is now possible; no one needs to decide anymore between strongSelf, ss, etc.
  • As has been said before regarding other shorthand sugar pitched for guard, eliding the entire else block is not considered a win for correctness. Instead, being explicit about what happens when the condition is not met can be important for reasoning about code. Silently doing nothing is not always correct--it may be that you really want to halt execution with fatalError(), for example--and we should not be creating syntax that makes silently doing nothing the default behavior.
  • The bar for adding new syntactic sugar is higher than the bar for adding new methods; the same criteria are a good starting point but wouldn't be sufficient.

This topic is now very old. I think that, if you have new concrete use cases that can strongly motivate the addition of new syntax in the face of recent changes that enable guard let self = self else { ... }, it would be worth launching a new discussion. However, in the absence of new evidence, if anything the need for new syntax is less rather than greater now, in my view, especially seeing how some of the arguments you're raising in favor are now superseded by the recent change.


(Nikita Leonov) #12

It makes simple to write a code similar to if without else and end up with an unpredictable / undesired behavior. An existing weak self jump through the hoops always expect you to provide else where you should do all the handling, logging etc. [guard self] makes it normal to not handle a faulty scenario, this will enforce a bad habits for less experiences engineers.


(Rod Brown) #13

Has this been confirmed? I thought this was still considered a bug...


(Xiaodi Wu) #14

The fix has been merged into master.


(Andrés Cecilia Luque) #15

My first point was to get clear that the two proposal solve different issues. Thus, there is no need to choose one or another. As I see it, discussing if we want [guard self] is independent of the guard let self = self else { ... } proposal. Can we agree on that?

About the rest:

In case guard let self = self else { ... } is confirmed, you are right about the consistency argument.

If you want to halt execution with fatalError(), then you do not use [guard self]. As explained, [guard self] is syntactic sugar for a very concrete scenario, where you dont want to execute the code in the closure if any of the identifiers in the capture list is nil. And despite being a very concrete case, in my opinion the [guard self] proposal makes sense due to the high amount of times this pattern appears on the code.

I understand. As I see it, this toggle() proposal that was recently accepted is adding syntactic sugar that is really useful only in a very concrete case: when in larger nested structs. If you are not in an scenario with a large nested struct, then the advantage for using it is not so much.
If such a proposal was accepted, I wonder why not this syntactic sugar proposal that is being discussed and requested during years would not be accepted, or at least seriously considered as a proposal.
I think the acceptance and rationale of the toggle() proposal is a good starting point for, at least, making an official [guard self] proposal, and end up the recurring discussions about it.

Yes, this topic is very old, but even after discussing it a lot, I have been reading all the information I could find about it and the result is:
a) This was never oficially proposed in swift evolution.
b) I could not find a proper rationale of why there has never been an official proposal, or why this syntactic sugar is undesired. Seems to me that when the discussion was happening, it was not the right time for this, or the people supporting the discussion gave up.
c) Because there has never been an official proposal, there is no official rationale from the core team for or against it.
d) From the discussion and the comments in here, seems to me that there is still interest on this, or alternatively, on a solution for the capturing weakly and then unwrapping commonly repeated scenario.

I see your point. Maybe guard is not the right keyword for this. It seems to me the right one, but of course having a similar implementation with another keyword that fits better is an alternative.

Well, I think what less experiences engineers do is not use the capture list, thus creating reference cycles. And, at the moment they realise that when using closures almost everything should be captured weakly and then unwrapped, is when you start wondering: is there any alternative for this commonly repeated code [weak self] in guard let strongSelf = self else { return }?
Also, if you are using [guard self] you should be aware of what you are doing. Using the features of a language without even reading what they are intended for is a bad habit. Using features of a language that allow you to write clearer, more readable code, I think is a good habit.


(Xiaodi Wu) #16

In the Swift Evolution process, proposals are reviewable when the author creates a document that achieves some amount of consensus during the pitch phase. As you can see from the preceding conversation here, members of the community have doubts that this is the most desired solution.

This may fine as a general principle when you're supervising junior developers, but from the perspective of language design you can't wave away the potential of creating sharp edges in the language by saying, "you should know what you're doing." I'm not aware of any place in the language that has hidden control flow like this.

In any case, it can be interesting to visit old topics when something new changes the context. To drive the conversation forward, do you have concrete use cases that haven't been previously considered?


(Tino) #17

btw:
Has it ever been discussed to make weak the default for closures?
It's probably to late for such a change, but besides compatibility, I think it would be better if you could omit [weak self] and had to explicitly state when you want a strong reference.


(Andrés Cecilia Luque) #18

No. I am bringing back the discussion because:
a) It is being mentioned lately during the discussion of the "Allowing shadowing and rebinding of self" proposal (with people for and against it), and I see it keeps getting support here.
b) I could not find a rationale with purely technical reasons agains it (most of the reasons agains it that I have red are very subjective to the developer talking).
c) Considering that I could not find purely technical reasons agains it, I looked for a subjective reason against it coming from the core team. I could not find it either.
d) I saw the recently approved toggle() proposal, and I think the rationale for accepting it has several similarities with the rationale I would give to this [guard self] proposal.
e) Due to point d), and considering that this discussion has happened already several times without a clear acceptance/rejection outcome, I think it would be interesting to push a bit more for it, or even make an official proposal in order to get more visibility, and eventually an acceptance/rejection rationale, making things clear and preventing the discussion from happening in the future.

This discussion being repeated over an over is an indicator to me that several developers are seeking for an alternative for [weak self] in guard let strongSelf = self else { return }. Having a clear rationale for or againt it would help.

As an example, the "Allowing shadowing and rebinding of self" proposal has been accepted in September 2016, but an implementation of it did not happen until March 2018, even though the context did not change from 2016 to 2018. And I believe that the reason for this is that in 2016, the focus of the community was on other more important topics, but now in 2018 the focus is shifting thowards this not so important - but still possible to improve - syntactic issues that we are seeing lately.


(Andrés Cecilia Luque) #19

@Tino agree, but that would be another discussion thread, in my opinion. Gather some old threads and discussion about it, and go for it ;P


(Xiaodi Wu) #20

The Swift Evolution process doesn't really work this way. An idea needs to achieve a certain amount of uptake and consensus before review. When people raise reasons why they are not in favor of the proposal, these need to be addressed or answered in some way and not dismissed as "subjective." Most ideas are brought up and fade out because it never gets to a point where it's ready for review, and pushing an idea forward after extensive discussion requires more than "+1."


(Andrés Cecilia Luque) #21

@xwu I know how the Swift Evolution process work, and that is why we are having this discussion.

I answered your concerns here. If you think I was not clear enough, ask again.

If you find other arguments against this proposal I would happily read them, because as I said I could not find them in the old threads, where usually the thread was just dying or diverging the discussion topic.


(Xiaodi Wu) #22

I think we are misunderstanding each other. I don't mean that you haven't replied to my post. To address or answer a concern would involve changing the proposed idea such that the concern becomes inapplicable.

The posts above mine in this thread lay out a variety of concerns, none of which have been addressed to date. Forward movement would involve giving deep consideration to these concerns and coming up with something new that resolves the difficulties raised. It appears that you're dismissing all of these comments as "very subjective," which is not conducive to having a discussion.


(Andrés Cecilia Luque) #23

@xwu You are welcome to come with another keyword proposal instead of guard. As I said, guard makes sense for me. That is the only thing I can come up with in order to accomodate your concerns.

About the other concern you raised, I think is out of the scope of the proposal, as [guard self] (the way I see it) is trying to address a very concrete case: capturing weakly + not executing the code inside the closure if any of the captured identifiers are nil. Doing fatalError() does not fall in that category, because to halt with fatalError() you need to execute the code in the closure.


(Andrés Cecilia Luque) #24

@xwu The creator of the more complex draft said that the increased complexity added by his proposal is not worth, going back to the original proposal of [guard self].

As him, I believe in introducing small proposals one after another, instead of making a big proposal that tries to fix all our concerns. The [guard self] proposal is a scoped change solving a concrete problem when using closures.

Some of the other posts before were presenting guard let self = self else { return } as a solution. I already explained here why the guard let self = self else { return } proposal seems different and independant of the [guard self] proposal to me, and I asked you if you agreed with me on that.

Other post said that:

I agree with that, but still believe that the benefits of it pays off the slightly increase of complexity.

About:

I agree. And I also commented that:

And the last post saying:

Aligns with your concern about the keyword guard, and the possibility of finding another keyword that fits better.