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

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

Closed
2 tasks done
amcinnis opened this issue May 12, 2024 · 9 comments · Fixed by #5583
Closed
2 tasks done

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

amcinnis opened this issue May 12, 2024 · 9 comments · Fixed by #5583

Comments

@amcinnis
Copy link

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
Copy link
Contributor

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

@SimplyDanny
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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

@amcinnis
Copy link
Author

amcinnis commented May 13, 2024

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
Copy link
Contributor

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

@ZevEisenberg
Copy link
Contributor

ZevEisenberg commented May 29, 2024

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 classpart we're changing. We're changingclass functostatic func`.

@tonyarnold
Copy link
Contributor

"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
Copy link
Contributor

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?

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.

4 participants