Add option for unneeded_override to affect initializers

[leonardosrodrigues0]

Changes

These changes add an affect_initializers option to unneeded_override rule . Closes #5265.

When the option is enabled, the overridden initializers are checked in the same way methods are. In addition, failable (init?) and implicitly unwrapped (init!) initializers never trigger the rule, as it is allowed to override one with the other and vice versa. Finally, private overrides also don't trigger the rule, as they can be used to hide the super initializer.

Implementation

A new OverridableDecl protocol was built to allow both FunctionDeclSyntax and InitializerDeclSyntax to be checked using the same function. The check for init name on the member access expression was implemented by comparing directly to the "init" string, as it is done in ExplicitInitRule, NSNumberInitAsFunctionReferenceRule and ReduceIntoRule.

[SwiftLintBot]

	16 Messages
📖	Linting Aerial with this PR took 1.19s vs 1.19s on main (0% slower)
📖	Linting Alamofire with this PR took 1.65s vs 1.62s on main (1% slower)
📖	Linting Brave with this PR took 9.07s vs 9.08s on main (0% faster)
📖	Linting DuckDuckGo with this PR took 4.69s vs 4.73s on main (0% faster)
📖	Linting Firefox with this PR took 10.51s vs 10.59s on main (0% faster)
📖	Linting Kickstarter with this PR took 11.05s vs 11.05s on main (0% slower)
📖	Linting Moya with this PR took 0.62s vs 0.62s on main (0% slower)
📖	Linting NetNewsWire with this PR took 3.26s vs 3.29s on main (0% faster)
📖	Linting Nimble with this PR took 0.81s vs 0.82s on main (1% faster)
📖	Linting PocketCasts with this PR took 8.87s vs 8.95s on main (0% faster)
📖	Linting Quick with this PR took 0.39s vs 0.39s on main (0% slower)
📖	Linting Realm with this PR took 11.68s vs 11.7s on main (0% faster)
📖	Linting Sourcery with this PR took 2.84s vs 2.82s on main (0% slower)
📖	Linting VLC with this PR took 1.58s vs 1.58s on main (0% slower)
📖	Linting Wire with this PR took 19.53s vs 19.69s on main (0% faster)
📖	Linting WordPress with this PR took 13.3s vs 13.37s on main (0% faster)

Generated by 🚫 Danger

[leonardosrodrigues0]

Any thoughts on this?

[leonardosrodrigues0]

@SimplyDanny Could you review this enhancement? This would be my first contribution here and any suggestion are welcome.

[SimplyDanny]

Very well done! Not much to say. 👍

To check the rule against a few OSS project, please set the new option to true by default. With the report produced by the build we are able to judge on the option's impact. Typically we find some special cases that need to be considered in the implementation. After the report is checked, you can turn off the option again.

[leonardosrodrigues0]

Thank you for reviewing this!

Typically we find some special cases that need to be considered in the implementation.

I made the change and checked the report. It seems like the differences found were the expected.

Cases with overrides that change the visibility of the init caught my attention, but those could also happen with a method and it seems impossible to me to catch them while linting.

There are also the required inheritance of inits that do not need the override keyword, but if the subclass implement another initializer, then it is mandatory to implement the required init even if it only calls the super, so I don't think we can trigger in that case.

After the report is checked, you can turn off the option again.

I was going for off by default, but now I'm thinking that it would be expected for this to be on by default...

[SimplyDanny]

override init!(arg: ...) {
    super.init(arg: arg)
}

and

override init?(arg: ...) {
    super.init(arg: arg)
}

should not trigger. It's allowed to override a failable initializer with an initializer that's implicitly unwrapped and vice versa.

The private keyword in

private override init() {
    super.init()
}

is an indicator that this override exists to hide the initializer. If the parent was private as well, the override would be prohibited by the compiler.

I was going for off by default, but now I'm thinking that it would be expected for this to be on by default...

The thing is that folks might already have adopted the rule and it's always surprising if the same rule suddenly triggers on more cases. Sure, then they can set the option to false. But just as much they can activate it intentionally.

[leonardosrodrigues0]

Thanks for the insights on failable, implicitly unwrapped and private initializers, I'm fixing those.

I found a solution to not expose the private protocol contents while conforming the types to it:

class Foo {}

private protocol Bar {
    var value: String { get }
}

private extension Foo {
    var value: String { "bar" }
}

extension Foo: Bar {}

I'm thinking this should be good for us here, let me know if you judge otherwise.

[SimplyDanny]

I'm thinking this should be good for us here, let me know if you judge otherwise.

It's good enough in the sense of a workaround. But is it really easy to follow? If it doesn't bother you too much, I'd still prefer the extra name argument. Not elegant but easy to comprehend.

[leonardosrodrigues0]

It's good enough in the sense of a workaround. But is it really easy to follow? If it doesn't bother you too much, I'd still prefer the extra name argument. Not elegant but easy to comprehend.

Interesting point, I agree... I just changed it and added the checks for failable, implicitly unwrapped and private initializers.

[SimplyDanny]

Thanks, @leonardosrodrigues0! That's a great first contribution. 👍

[leonardosrodrigues0]

Thank you @SimplyDanny for the reviews and the support!!