Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rule Request: Prefer static over final class #5471

Closed
SimplyDanny opened this issue Feb 23, 2024 · 9 comments · Fixed by #5487
Closed

Rule Request: Prefer static over final class #5471

SimplyDanny opened this issue Feb 23, 2024 · 9 comments · Fixed by #5487

Comments

@SimplyDanny
Copy link
Collaborator

The modifiers final and class on a method/property have the same meaning as static. The rule should flag appearances of final class on the same declaration and replace them by static.

I don't think there is any need for configuration. The rule could be enabled by default, but this can be decided in review when browsing through the reported OSS findings.

Triggering:

class C {
    final class func f() {}
}

final class C {
    class func f() {}
}

Non-triggering:

class C {
    static func f() {}
}

class C {
    class func f() {}
}

final class C {}
@omelics
Copy link

omelics commented May 16, 2024

The rule conflicts with nested classes. For example:

extension A {
    final class B { }
}

This obviously cannot be replaced with the static keyword and will lead to a compilation error.

@SimplyDanny
Copy link
Collaborator Author

The rule conflicts with nested classes. For example:

extension A {
    final class B { }
}

This obviously cannot be replaced with the static keyword and will lead to a compilation error.

Which part is replaced by the rule in this example?

@omelics
Copy link

omelics commented May 16, 2024

The rule conflicts with nested classes. For example:

extension A {
    final class B { }
}

This obviously cannot be replaced with the static keyword and will lead to a compilation error.

Which part is replaced by the rule in this example?

Sorry, the previous example was incorrect.

The following code is triggered by the rule, while it should not, I believe:

final class A: UIView {
    override class var layerClass: AnyClass {
        CALayer.self
    }
}

Seems like it happens because of combination of a final class and a class property override. Swift do allow replacing class keyword with static in this scenario, but Xcode suggests using override class var by default.

@SimplyDanny
Copy link
Collaborator Author

Seems like it happens because of combination of a final class and a class property override. Swift do allow replacing class keyword with static in this scenario, but Xcode suggests using override class var by default.

Just checked with

class B {
    class var a: Int { 1 }
}

final class A: B {
    override static var a: Int { 2 }
}

which compiles fine in Swift 5.10.

@omelics
Copy link

omelics commented May 16, 2024

Seems like it happens because of combination of a final class and a class property override. Swift do allow replacing class keyword with static in this scenario, but Xcode suggests using override class var by default.

Just checked with

class B {
    class var a: Int { 1 }
}

final class A: B {
    override static var a: Int { 2 }
}

which compiles fine in Swift 5.10.

This compiles fine, you are right. But the following does not.

final class A: B {
    override class var a: Int { 2 }
}

This is why I highlighted the default Xcode suggestion behavior in this case.

And, by the way, should the rule even been triggered by this kind of code? It has no sequential combination of final class keywords inside the class. It does have a separate final class and a separate class / static property (not class or func) instead, isn't it?

@SimplyDanny
Copy link
Collaborator Author

This is compiles fine, you are right. But the following does not.

final class A: B {
    override class var a: Int { 2 }
}

This is why I highlighted the default Xcode suggestion behavior in this case.

What is the error in this example? Both variants seem to be fine for me.

And, by the way, should the rule even been triggered by this kind of code? It has no sequential combination of final class keywords inside the class. It does have a separate final class and a separate class / static property (not class or func) instead, isn't it?

This is exactly the idea of this rule. It mimics the behavior of the compiler when you use open in a final class. Using class instead of static in a final class is the same "violation" but for class properties or functions. The purpose of this rule is to serve the same warning in the "static" case.

@omelics
Copy link

omelics commented May 16, 2024

This is exactly the idea of this rule. It mimics the behavior of the compiler when you use open in a final class. Using class instead of static in a final class is the same "violation" but for class properties or functions. The purpose of this rule is to serve the same warning in the "static" case.

So let me clarify a little.

Let's say there is the same base class B.

class B {
    class var a: Int { 1 }
}
  1. The following code will FAIL to pass the rule, because the final class A should not contain a class property, and it should be replaced with a static one:
final class A: B {
    override class var a: Int { 2 }
}
  1. Thus the following code will PASS the rule as expected:
final class A: B {
    override static var a: Int { 2 }
}
  1. Both of the following blocks of code will PASS the rule, because the class A is not marked as final, so it can contain any property you want, class or static:
class A: B {
    override class var a: Int { 2 }
}
class A: B {
    override static var a: Int { 2 }
}

Is this the intended behavior of the rule?

@SimplyDanny
Copy link
Collaborator Author

Is this the intended behavior of the rule?

Yes, once a class comes with final you should not state that something can be overridden by using class. Without the final it's your decision to put static or class.

@omelics
Copy link

omelics commented May 16, 2024

Ok, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants