realm / SwiftLint

A tool to enforce Swift style and conventions.
https://realm.github.io/SwiftLint
MIT License
18.46k stars 2.2k forks source link

0.55.0: Static Over Final Class warning occurs on overridable class property #5570

Closed amcinnis closed 1 month ago

amcinnis commented 1 month ago

New Issue Checklist

Describe the bug

In Xcode's boilerplate UITests launch tests code, they override a XCTest class property. This triggers a static over final class violation warning in SwiftLint 0.55.0.

final class StaticOverFinalUITestsLaunchTests: XCTestCase {

    // This triggers a static_over_final_class violation:
    override class var runsForEachTargetApplicationUIConfiguration: Bool {
        true
    }

   ...
   ..
   .
}

The Static Over Final Class documentation mentions the warning should only populate for non-overridable declarations. Is this a bug?

tonyarnold commented 1 month ago

I'm seeing the same in my project, in other NSObject-derived classes.

SimplyDanny commented 1 month ago

The intention of the "non-overridable" in the documentation is like "elements that cannot be overridden further should be marked static instead of final class".

In your example, static instead of class should be valid. Or are you seeing any compilation issues after the change?

amcinnis commented 1 month ago

Ah, thanks for that clarification Danny. If I replace override class var with override static var then everything compiles and SwiftLint does not show a violation warning. I believe I have a better understanding now of what the rule tries to accomplish.

Hopefully this GitHub issue will help others better understand as well!

SimplyDanny commented 1 month ago

Perhaps it's better to remove the part "for non-overridable declarations" from the message entirely. Anyone willing to do that? 😄

amcinnis commented 1 month ago

If I did, I would explicitly add this case to the static over final documentation for both triggering and non-triggering examples since it calls out Xcode boilerplate code that's bound to trigger a violation upon project creation.

However, my job prevents me from contributing to open source software so hopefully someone else can take the lead here 😅

tonyarnold commented 1 month ago

Yeah, the wording of the warning here isn't clear. I took "non-overridable declarations" to mean methods that weren't overridden.

ZevEisenberg commented 1 month ago

I still find the wording to be confusing:

SomeFile.swift:8:5: error: Static Over Final Class Violation: Prefer 'static' over 'final class' (static_over_final_class)

For this call site:

final class SomeClass: NSObject {
    @objc class func someFunc(...)

The confusing part is that the error message says "over final class", but it's not the final class part we're changing. We're changing class func to static func.

tonyarnold commented 1 month ago

"Prefer static methods over class methods on final classes"?

For interest: I'm honestly unclear what the benefit is to this rule — is it just for consistency? (It would seem not, given that it's only catching final classes).

Is there any functional difference between a class method and a static method on a final class?

ZevEisenberg commented 1 month ago

TIL: static is an alias for final class: https://stackoverflow.com/a/29206635/255489

This could be part of the confusion: I didn't know final class functions were a thing in Swift, so seeing that in the error message was confusing. Maybe leave it out if it was like the declaration I mentioned?

SimplyDanny commented 2 weeks ago

TIL: static is an alias for final class: https://stackoverflow.com/a/29206635/255489

This is one aspect of the rule. They are both equivalent, so the rule suggests to prefer the shorter one. In final classes, final is implicitly applied to all contained declarations, thus every class becomes final class.

Is there any functional difference between a class method and a static method on a final class?

I don't think there is, at least not in Swift. Perhaps there are both class and static in Swift because of backwards compatibility with Objective-C. But this is not my competence.

Anyway, due to having two keywords, the prescribed meaning for class is "can be overridden" as opposed to static which cannot. However, together with final, nothing can actually be overridden, hence the suggestion to use static instead.

Incidentally, the Swift compiler complains about open being used in final classes or declarations likewise.

My suggestions are "Prefer static over final class" on f in

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

and "Prefer static over class in a final class" on f in

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

Does that make things clearer?