Super call enforcement

ebg · July 6, 2020, 3:43pm

But just calling super may still be a bug, if it's called in the wrong order. I don't see the benefit of adding one without the other.

Avi · July 6, 2020, 6:48pm

There's still value because getting the programmer to add a call should also prompt him to think of where the call should be in his code. Any bugs are entirely his fault at that point; the compiler has done what it can.

Jean-Daniel · July 6, 2020, 7:06pm

How would this be a bug ?

fun foo() {
  log("will call foo")
  super.foo()
  log("did call foo")
}

fun foo(arg) {
  withSomeContextSensitiveValue { value in
    super.foo(value)
  }
}

Forcing super at start or at end prevent writing perfectly valid code.

ebg · July 6, 2020, 7:21pm

It's not a bug in that example. However subclassing UI components can often lead to bugs. Look at this headerdoc for Apple UIKit method updateConstraints():

Important
Call [super updateConstraints] as the final step in your implementation.

suyashsrijan · July 6, 2020, 7:40pm

Right, but one can write:

override func updateConstraints() {
  // Update constraints here
  super.updateConstraints()
  Logger.debugLog(“Finished updating constraints”)
}

which is a correct implementation even if the super call is not strictly the last call in the method body. Enforcing that the super call must appear in some place in a body (such as the start or the end) is not compatible with various forms of completely legal code.

The discussion about call placement has already happened in the past and the conclusion is that it is not feasible to add it. I think it would be be great to avoid rehashing the same discussion in this thread.

I think the most useful and valuable thing is the compiler warning you about the missing super call, rather than also about where the call should go, how many times the call should happen and other details.

mayoff · July 6, 2020, 8:09pm

Example 0:

class C0 {
    @requiresSuper
    func m() { }
}

class C1: C0 {
    override func m() { super.m() }
}

class C2: C1 {
    override func m() { }
}

Does C2 provoke a warning? The proposal should specify that it does.

Example 1:

class C0 {
    @requiresSuper
    func m() { }
}

class C1: C0 {
    @ignoresSuper
    override func m() { }
}

class C2: C1 {
    override func m() { }
}

Does C2 provoke a warning? The proposal should specify that it doesn't.

Example 2:

class C0 {
    @requiresSuper
    func m() { }
}

class C1: C0 {
    @ignoresSuper
    @requiresSuper
    override func m() { }
}

class C2: C1 {
    override func m() { }
}

Is declaration C1 permitted? The proposal should specify that it is permitted, and that C2 provokes a warning.

drumnkyle · July 11, 2020, 1:40am

I was the first one to ever bring this up and submitted a proposal back a long time ago that was closed here:

Here is the last thread I commented on about this:

michelf · July 11, 2020, 11:02am

Without really enforcing where the call to super happens, knowing if the call is expected happen at the start or at the end could enable correct fix-it suggestions when there is a diagnostic about a missing call to super.

michelf · July 11, 2020, 11:10am

I wouldn't expect @ignoreSuper to have an effect outside of a specific function body. And because of this I somewhat doubt putting it in the signature is the right place. I'd rather see it in the body, a bit like this:

class C1: C0 {
    override func m() {
        // ignore super
    }
}

This is where I'd document not calling super is a deliberate choice. It'd be a bit novel though for a comment to silence a warning, so maybe this is better:

class C1: C0 {
    override func m() {
        super._ // ignore call to super
    }
}

Tino · July 11, 2020, 3:44pm

Imho the feature is definitely useful, especially (but not only) in the context of Cocoa with its various methods that require chaining (and those where you should not call super at all).
It just touches some parts of Swift which have been a big source for frustration in the past (access control), and it's hard to get this one right.
It starts with the name:
There's a precedent in Objective-C, but it should actually rather be enforceSuperCallInSublcasses than requiresSuper, which breaks with the meaning of super.
My favorite would be extendable, as you should never replace the annotated method completely...

I don't think it's any good to enforce the position of the call to super, but it could still be useful to add that information; it could also allow to generate the super call automatically (if it doesn't happen in the overridden method).

Other questions that imho need discussion:

Should it be allowed to call super conditionally?
Should it be enforced that the call to super is done with the same parameters as the original call?

suyashsrijan · July 11, 2020, 4:12pm

I don't think the fix-it would be correct in all situations though. For example, if updateConstraints wants the super call to go in the end and one has written a function like:

override func updateConstraints() {
  // Update constraints here
  Logger.debugLog(“Finished updating constraints”)
}

inserting the fix-it after the call to debugLog wouldn't be correct from the user's point of view. You can add some heuristics, for example to insert it before any user code, but that too could change the meaning of the code or cause errors. This is why I think it would be better to let the user decide where the call should go.

suyashsrijan · July 11, 2020, 4:20pm

Thanks, I will add it to the proposal. Although, the way it works now means the attributes are not transitive. For example:

class C0 {
    @requiresSuper
    func m() { }
}

class C1: C0 {
    override func m() { super.m() }
}

class C2: C1 {
    override func m() { }
}

The declaration C2.m() will not lead to a warning because C1.m() does not implicitly inherit @requiresSuper from C0.m(). Do you think it makes sense to do that?

michelf · July 11, 2020, 4:24pm

It'd be nice if it could be documented at the right place in the control flow, such as:

if something {
   super.call()
} else {
   super._ // ignore call to super
}

This way the compiler can warn about any path with an unclear intention.

A common reason functions are overridden is to transform the parameters before calling super. But sometime it's probably a bad idea to change them. Is this worth another flag though?

suyashsrijan · July 11, 2020, 4:25pm

I think the only requirement should be that there is a super call somewhere in the body.

I think the super call must be made to the same method that is annotated with @requiresSuper, although I don't think there should be restrictions enforced on what arguments are passed as long as it's valid i.e. you can do super.viewDidAppear(false) if you want and you don't necessarily have to do super.viewDidAppear(animated). The fix-it to insert the super call could use placeholders where the arguments go, which can be filled in by the user.

mayoff · July 11, 2020, 5:57pm

Yes, I think requiresSuper should be inherited unless the subclass declares ignoresSuper.

C0 annotates m with @requiresSuper because C0 calls self.m() to maintain some invariant. A more concrete example:

open class StyledText {
    // Make `boldness(at:)` return true for every point in `range`.
    public func embolden(in range: Range<String.Iterator>) {
        ... implementation details ...
        self.styleDidChange(in: range)
    }

    // When NOT inside a call to `embolden(in:)`, the returned
    // `extent` is the largest range that contains point and in which
    // every character's boldness is `isBold`
    public func boldness(at point: String.Iterator) -> (isBold: Bool, extent: Range<String.Iterator>) {
        ... implementation details ...
    }

    @requiresSuper
    public func styleDidChange(in range: Range<String.Iterator>) {
        // Coalesce adjacent bold ranges to simplify the implementation
        // of `boldness(at:)`.
        ... implementation details ...
    }
}

Here, StyledText's implementation of embolden(in): relies on the behavior of styleDidChange(in:) to ensure that boldness(at:) returns the correct extent of boldness.

Let's add a subclass of StyledText:

open class VeryStyledText: StyledText {
    public override func styleDidChange(in range: Range<String.Iterator>) {
        super.styleDidChange(in: range)
        ... implementation details ...
    }
}

VeryStyledText calls super, so StyledText can maintain its own correct behavior.

Now we have a subclass of VeryStyledText:

open class ExtremelyStyledText: VeryStyledText {
    public override func styleDidChange(in range: Range<String.Iterator>) {
        // Does Swift warn if I don't call super.styleDidChange(in:) here?
    }
}

If ExtremelyStyledText doesn't call super.styleDidChange(in:), then StyledText cannot maintain its own correct behavior.

In other words, StyledText wants @requiresSuper to inform all subclasses that they need to call StyledText. It doesn't matter whether the subclass is a direct child of StyledText (like VeryStyledText) or a more distant descendant (like ExtremelyStyledText). They all need to call super in styleDidChange(in:). So @requiresSuper should be inherited by default.

If VeryStyledText really knows what it is doing (which should almost always require it to come from the same vendor as StyledText), then it can declare @ignoresSuper to override @requiresSuper. In such a case, the inheritance of @requiresSuper stops at VeryStyledText unless VeryStyledText also declares @requiresSuper.

rlziii · July 16, 2020, 6:35pm

I certainly agree with this statement. Trying to enforcement the placement of super becomes a huge mess. Perhaps attaching an optional guidance message to the annotation that could show up as part of the warning would make sense to direct the programmer? For example:

@requiresSuper("Call super.updateConstraints() as the final step in your implementation.")
func updateConstraints() {
    // ...
}

And then later the warning message would be something along the lines of:

override func updateConstraints() {} // warning: method override is missing 'super.updateConstraints()' call; Call super.updateConstraints() as the final step in your implementation.

suyashsrijan · July 16, 2020, 6:43pm

I think it would be reasonable to allow specifying an optional message.

suyashsrijan · July 27, 2020, 3:37pm

I have adjusted the implementation to incorporate some feedback:

I have upgraded the warning to an error, because the attribute implies a strong requirement and hence an error is more suitable than a warning.
I have added the ability to specify a message on the @requiresSuper attribute, which will be shown along with the missing 'super' call diagnostic, as suggested by @rlziii.
The @requiresSuper attribute will be implicitly inherited by any overrides of a function which has been annotated with that attribute (as suggested by @mayoff) according to the following rule:
(1) the override is not annotated with @ignoresSuper.
(2) the override is not in a final class.

A macOS and Linux toolchain is now available for testing purposes.

Alejandro · July 27, 2020, 3:38pm

I think this link points to a different post

suyashsrijan · July 27, 2020, 3:38pm

Oops, I'll fix it. Not sure what happened there!

Edit: Fixed :)