Rule(s) Request unnecessary_open and missing_final

[mildm8nnered]

New Issue Checklist

• Updated SwiftLint to the latest version
• I searched for existing GitHub issues

New rule request

The idea of unnecessary_open is that it would warn about uses of open where it is not required, because the class, var, or function is never overridden outside its own module.

missing_final would warn whenever a class, var, or method was never overridden either inside or outside its own module, and can be safely declared as final.

Presumably these would need to be analyzer rules. Because the logic required is quite similar, it might be easier to combine them into one rule.

1. Why should this rule be added?

If open and final are strictly enforced, then it becomes possible to tell from the declaration whether and where a class is ever subclassed, or a var of fun overridden, as opposed to whether they could be subclassed or overridden in theory.

final - never overridden
internal or public - overridden, but only within the same module
open - overridden outside their defining module.

Because unnecessary open's and missing finals are not currently detected, in our own codebase, we were able to tighten these up very considerably when we did this analysis by hand.

3. Provide several examples of what would and wouldn't trigger violations.

Triggering Examples

For unnecessary_open:

↓open class Foo {
    // Foo is never subclassed outside it's defining module
}

open class Foo {
    // bar is never overridden outside it's defining module
    ↓open var bar: Int = 0
}

For missing_final:

↓public class Foo {
    // Foo is never subclassed at all
}

public final class Foo {
    ↓public var bar: Int = 0
    // bar is never overridden
}

Non-Triggering Examples:

For unnecessary_open:

open class Foo {
    // Foo is subclassed outside its defining module
}

open class Foo {
    // bar is overridden outside its defining module
    open var bar: Int = 0
}

For missing_final:

public class Foo {
    // Foo is subclassed within its defining module
}

public final class Foo {
    public var bar: Int = 0
    // bar is overridden within its defining module
}

5. Should the rule be configurable, if so what parameters should be configurable?

If combined into one rule, it would probably be good to allow them to be selectively enabled.

For libraries, where it is kind of expected that you will advertise your API, but not necessarily call it yourself, this could be a pain, but you can always disable it.

7. Should the rule be opt-in or enabled by default? Why?

As an analyzer rule, by definition this would be opt-in.

Other notes:

Not entirely sure how you would unit test a rule like this that requires different compilation modules.

[SimplyDanny]

Do you know of Periphery? Even though it does not yet provide the requested features (I guess), it could (more easily) be extended to do so. SwiftLint would have to reimplement parts of what Periphery already provides.

[mildm8nnered]

Do you know of Periphery? Even though it does not yet provide the requested features (I guess), it could (more easily) be extended to do so. SwiftLint would have to reimplement parts of what Periphery already provides.

I had never seen Periphery, but I will definitely grab it and have a play now.

I tried stubbing out unnecessary_open, based on unused_declaration, and I don't think this would be actually be too hard (famous last words), but let me see what Periphery can do.

[mildm8nnered]

Oooh - I like the guided setup - that is quite nice.

% periphery scan --setup
Welcome to Periphery!
This guided setup will help you select the appropriate configuration for your project.

* Inspecting project...

Select build targets to analyze:
? Delimit choices with a single space, e.g: 1 2 3, or 'all' to select all options
 1 Foo
 2 Bar
 3 ...
> all

Select the schemes necessary to build your chosen targets:
? Delimit choices with a single space, e.g: 1 2 3
1 Foo
2 Bar
> 1 2

Assume Objective-C accessible declarations are in use?
? Declarations exposed to the Objective-C runtime explicitly with @objc, or implicitly by inheriting NSObject will be assumed to be in use. Choose 'No' if your project is pure Swift.
(Y)es/(N)o > Y

Assume all 'public' declarations are in use?
? You should choose 'Yes' here if your public interfaces are not used by any selected build target, as may be the case for a framework/library project.
(Y)es/(N)o > N

Save configuration to .periphery.yml?
(Y)es/(N)o > Y

* Executing command:
periphery scan