realm / SwiftLint

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

Rule Request: No Empty Function #5615

Closed Ueeek closed 3 days ago

Ueeek commented 4 weeks ago

New Issue Checklist

New rule request

A Function body should not be empty. Otherwise, show a warning.

An empty function, init and deinit can harm readability and lead to confusion because readers need to guess whether it's intentional or not. Developers should put a comment to explain why the function/ init / deinit body is empty.

SonarLint for Swift and ESLint have the same rule. SwiftLint can be better by having this rule.

Triggering and Non-Triggering

Triggering:
func f1 () {}

func f2 () {
}

// Comment
func f3 () {}

init() {}

deinit {}
Non-Triggering
func f4 () {
    let s = "xyz"
}

func f5 () { /* Comment*/  }

func f6() {
    // Comment
}

init() { /* comment */ }

deinit { /* comment */ }

Just so you know, this behavior is the same as SonarLint and ESLint.

Should be configurable?

No parameter to configure.

Should be opt-in?

In my opinion, this rule should be "opt-in rule" since there is no consensus, though it's active by default in SonarLint and ESLint. Got feed back, and change to default.

If this rule makes sense, I will work for this! Thanks.

Future enhancement

We can implement rules for closures and initializers as well. But, they should be implemented as separate rules.

mildm8nnered commented 3 weeks ago

Should empty inits and deinits be violations as well?

I actually have these as custom rules, but I think it would be nice if there was a built-in

I would personally lean towards this being on by default except that I think a lot of Xcode templates have empty functions in - OTOH maybe that's even more of a reason to make it a default.

  empty_functions:
   included: ".*.swift"
   name: "Functions must have a non-empty body"
   message: "Function bodies must have some content, even if it's just a comment"
   regex: 'func\s+[^{]+\{\s*\}'
   severity: warning
  empty_init:
    included: ".*.swift"
    name: "init methods must have a non-empty body"
    message: "Init function bodies must have some content, even if it's just a comment"
    regex: 'init[^{]+\{\s*\}'
    severity: warning
Ueeek commented 3 weeks ago

@mildm8nnered , Thank you for your feedback! I also think it's a good idea to support init() and deinit.

OTOH maybe that's even more of a reason to make it a default.

I agree with this point. Let me change it to the default rule from the opt-in rule. If anyone has a different opinion, we can change it back to opt-in.

I updated PR and the above description based on your feed back! Thank you!