Thanks for your continued effort on this, @suyashsrijan!
I'm a bit apprehensive about the combination of the error diagnostic and the @ignoresSuper attribute. IMO, if violation of an @requiresSuper directive is severe enough to be considered an error, we shouldn't offer a way for a client to unilaterally silence the diagnostic. OTOH, if we think that it is, in some cases, perfectly valid for a client to ignore an @requiresSuper directive, then it has no business being an error at all, and the silence-able warning is sufficient.
As I've mentioned up-thread, I come down on the side of scrapping @ignoresSuper altogether, but the "silence-able error" middle ground is less desirable to me than either alternative. Is there precedent in the language for this sort of error already?
I don't think we have precedent for an error which is suppressible using an attribute, but we do have @discardableResult which can suppress an unused function result warning.
I think we have the following options:
Warns by default
Errors by default
Warns if @requiresSuper
Errors if @requiresSuper
Escape hatch (@ignoresSuper)
Y
N
N
N
Y
N
Y
N
N
Y
N
N
Y
N
N
N
N
N
Y
N
N
N
Y
N
Y
N
N
N
Y
Y
(Note that some of these are more source-breaking than others)
I think your concern is reasonable. Given that I am proposing a "opt-in" system, it seems that not providing an escape hatch is probably fine, because the expectation would be that an API author would not "opt-in" and use this attribute, if they don't really need it. I have also explained this in the API resilience section, that adding this attribute to a public API method is a breaking change if any overrides of such a method doesn't already call super.
The only remaining question is - should this be an error for imported ObjC methods (annotated with objc_requires_super) as well? Or should it be downgraded to a warning instead (and perhaps upgrade it back to an error in Swift 6)?
The problem I see coming if there's no escape hatch is it's going to break useful patterns. I'm probably not wrong saying this won't work for instance:
autoreleasepool {
...
super.call()
...
}
All patterns wrapping the call to super inside of another function call (for scoped access, grouping undos and logs or other effects, etc.) are going to become impossible.
Perhaps if we had @once for closures it'd allow us to limit false positives and be more strict. But as things stand now I think the escape hatch is still needed and @requiresSuper should be a warning.
That's my thinking as wellâthis feature sits in a similar spot in my mind as access control. We don't provide a way for clients to arbitrarily overrule a library authors' stated access level via some @makePublic(otherModuleObject.someInternalProperty), even though we could do this in theory when the library source is available (as @testable shows). Clients can either work around the constraints laid out by the library author, or fork the library and change it as they wish.
Makes sense. Despite being an API break, is adding @requiresSuper ABI compatible for libraries compiled in evolution mode?
It would be a source break to suddenly start raising an error on imported Objective-C declarations with the equivalent attribute, so we would either have to not offer any diagnostic at all pre-Swift-6, or offer a warning.
I'd advocate for a (un-silence-able) warning for imported Objective-C declarations rather than no diagnostic at all.
As for upgrading to an error, I'm somewhat ambivalent and I think the decision should be data-driven. Whenever the implementation is ready, it would be great to do a run on the source compatibility suite to see if the source break is likely to occur in practice or not.
Importantly, these are both attributes that are applied by the API authors rather than clients. As a client, if I have a non-escaping closure that I want to use in an escaping context, I have to use withoutActuallyEscaping which IMO is much more awkward than just slapping an attribute on a declaration (and importantly, not offered as a fix-it).
I don't think it should affect ABI because it's just requiring users of the method to call super in their body. I would need to ask someone more experienced to authoritatively confirm this though.
Yeah, offering a warning seems like a sensible thing to do than no warning at all.
I will confirm this with a test run soon. When I turned on the warning by default (rather than using an attribute), I saw tons of warnings in stdlib, so I think there might be code in some projects in the source compatibility suite using imported ObjC APIs in a way which might also cause a diagnostic to be emitted.
I've wanted something like this for some time. I apologize for what is basically bikeshedding but It would be great if we captured whether or not the call should be pre|post|wherever in the subclass. I'm not sure about enforcing it but capturing it would be great for diagnostics and possibly automatically inserting in pre/post cases.
I was worried about errors/warnings that would break patterns, but if those things pass the scrutiny of the compiler then I'll be worried about how easy it is to fool instead. I mean, it can still be useful, but I would disapprove of it being an error on the ground that it'd make it looks like it is actually enforcing something while it's effectively just glancing at the code superficially to emit a reminder if it looks like there is no call to super.
I believe the option for the API author to specify a message with @requiresSuper is intended to address this request. For reasons noted up-thread, trying to enforce a specific location of the super call is probably going to result in a lot of false positives:
I was pretty careful to separate out capturing the intent and enforcing it but I'll try to be more clear.
Baking in the ability to specify pre|post|anywhere EVEN IF WE AREN'T GOING TO ENFORCE IT would be good, in my opinion. It ensures that a decision about ordering is made and documented in a way that doesn't rely on a message which can have any information whatsoever in any format whatsoever.
It would be possible for the implementation to skip inner closures or functions for example, but that would break some valid code. Path-sensitive checking is a little harder (and impossible in some scenarios). This is not to say that the implementation cannot be improved incrementally from the status quo.
The rule is that the overridden method calls the super method with the same signature, so this is not valid:
class A {
func foo(a: Int) {}
func foo(b: String) {}
}
class B: A {
override func foo(a: Int) { // error: method override is missing 'super.foo(a:)' call
super.foo(b: a.description)
}
}
This is also not valid as local type declarations are skipped:
class A {
@requiresSuper
func foo() {}
}
class B: A {
override func foo() { // error: method override is missing 'super.foo()' call
class A1 {
func foo() {}
}
class B1: A1 {
override func foo() { super.foo() }
}
}
}
I will add them to the test suite to ensure nothing regresses.
You can document it in the attribute if you want, in the form of a message that will be shown alongside the diagnostic. For example, @requiresSuper("call super before anything else"). You can assume that the lack of a message basically implies "anywhere". I think it would be quite uncommon for an author to specify the order, but if needed, the message seems like a good solution to me and can provide additional context/information if necessary.
And here I was under the impression it was purely syntactic. But if it checks types and overloads there's some semantics involved, right? You had me worried for a bit.
