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: unused_closure_parameter does not respect parameters with backticks #4588

Closed
2 tasks done
chosa91 opened this issue Nov 24, 2022 · 6 comments · Fixed by #4590
Closed
2 tasks done

Rule: unused_closure_parameter does not respect parameters with backticks #4588

chosa91 opened this issue Nov 24, 2022 · 6 comments · Fixed by #4590
Assignees
Labels
bug Unexpected and reproducible misbehavior.

Comments

@chosa91
Copy link

chosa91 commented Nov 24, 2022

New Issue Checklist

Describe the bug

In the newly released 0.50.0 version, the rule unused_closure_parameter detects parameter names between backticks as unused parameters, even if they are in use.
This might be intended behavior but it was not an issue in (at least) 0.49.1.

Complete output when running SwiftLint, including the stack trace and command used
$ swiftlint lint
… warning: Unused Closure Parameter Violation: Unused parameter "`self`" in a closure should be replaced with _. (unused_closure_parameter)

Environment

  • SwiftLint version: 0.50.0
  • Installation method used: Homebrew
  • Paste your configuration file:
only_rules:
  - unused_closure_parameter
  • Are you using nested configurations?
    NO
  • Which Xcode version are you using (check xcodebuild -version)?
    Xcode 14.1
    Build version: 14B47b
func weakify<Owner: AnyObject>(_ owner: Owner, closure: @escaping (Owner) -> Void) -> (() -> Void) {
    { [weak owner] in
        if let owner {
            closure(owner)
        }
    }
}

class Foo {
    struct Sut {
        var arg: (() -> Void)?
    }

    var sut = Sut()

    func bar() {
        var sut = Sut()
        sut.arg = weakify(self) { `self` in
            _ = self
        }
    }
}
@jpsim jpsim self-assigned this Nov 24, 2022
@jpsim
Copy link
Collaborator

jpsim commented Nov 24, 2022

I think the rule might be working correctly here and that you're strongly capturing self in your closure.

To only weakly capture self you'd need to refer to it with backticks:

func weakify<Owner: AnyObject>(_ owner: Owner, closure: @escaping (Owner) -> Void) -> (() -> Void) {
    { [weak owner] in
        if let owner {
            closure(owner)
        }
    }
}

class Foo {
    struct Sut {
        var arg: (() -> Void)?
    }

    var sut = Sut()

    func bar() {
        var sut = Sut()
        sut.arg = weakify(self) { `self` in
            _ = `self`
        }
    }
}

Or to use a different closure argument name to avoid the confusion:

func weakify<Owner: AnyObject>(_ owner: Owner, closure: @escaping (Owner) -> Void) -> (() -> Void) {
    { [weak owner] in
        if let owner {
            closure(owner)
        }
    }
}

class Foo {
    struct Sut {
        var arg: (() -> Void)?
    }

    var sut = Sut()

    func bar() {
        var sut = Sut()
        sut.arg = weakify(self) { this in
            _ = this
        }
    }
}

@jpsim jpsim added the help Questions or user problems that require more explanation rather than code changes. label Nov 24, 2022
@chosa91
Copy link
Author

chosa91 commented Nov 24, 2022

Yeah, you're right in the case of `self`, I gave a bad example, sorry.
But what about a parameter that doesn't exist in the func/class/global scope, like this:

class Foo {
    struct Sut {
        var arg: (() -> Void)?
    }

    var sut = Sut()

    func bar() {
        var sut = Sut()
        sut.arg = weakify(self) { `baz` in
            _ = baz
        }
    }
}
warning: Unused Closure Parameter Violation: Unused parameter "`baz`" in a closure should be replaced with _. (unused_closure_parameter)

ps.: It is a really dumb example to use backticks for a non-reserved keyword, so I accept if you close the post because of this. 🙄

@jpsim
Copy link
Collaborator

jpsim commented Nov 24, 2022

Because SwiftLint rules operate before typechecking and semantic analysis, there’s not enough information available to know if baz is referring to the backticked closure argument or some other declaration, perhaps even in a far away imported module.

I’ll do some more research to better understand the behavior of Swift’s identifier shadowing with backticks to see if there’s something we can do to improve this rule without requiring a separate, more semantically-aware analyzer rule.

@jpsim
Copy link
Collaborator

jpsim commented Nov 24, 2022

ok, so some lightweight research tells me that we should be able to ignore backticks when checking usage:

let foo = 0

let closure: (Int) -> Void = { `foo` in
    print(foo)
}

closure(1) // prints '1'

@jpsim jpsim added bug Unexpected and reproducible misbehavior. and removed help Questions or user problems that require more explanation rather than code changes. labels Nov 24, 2022
@jpsim
Copy link
Collaborator

jpsim commented Nov 25, 2022

Fix in #4590

@chosa91
Copy link
Author

chosa91 commented Nov 25, 2022

Thanks for the quick fix, have a nice day!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Unexpected and reproducible misbehavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants