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

Functions w/ Only Multiple Trailing Closures Trigger "No Space in Method Call" (Swift 5.3) #3259

Closed
2 tasks done
giladronat opened this issue Jul 7, 2020 · 5 comments · Fixed by #3280
Closed
2 tasks done
Labels
bug Unexpected and reproducible misbehavior.

Comments

@giladronat
Copy link

giladronat commented Jul 7, 2020

New Issue Checklist

Describe the bug

When using Swift 5.3's multiple trailing closure syntax (disabling the rule for multiple_closures_with_trailing_closure), a function call with only multiple trailing closures as arguments will trigger "No Space in Method Call Violation."

Common examples of such functions are:

  1. SwiftUI Button initializer:
Button { // No Space in Method Call Violation
    print("Button tapped")
} label: {
    Text("Button")
}
  1. Combine sink Operator:
Just(1)
    .sink { completion in // No Space in Method Call Violation
        print(completion)
    } receiveValue: { value in
        print(value)
    }
    .store(in: &disposeBag)

Function calls that begin with non-closure parameters followed by multiple trailing closures work just fine. For example:

UIView.animate(withDuration: 0.5) { // All clear, no violation
    print("animating")
} completion: { _ in
    print("done")
}

Removing the space properly triggers "Opening Brace Spacing Violation" (see swiftlint autocorrect output below).

Complete output when running SwiftLint, including the stack trace and command used
$ swiftlint lint
Loading configuration from '.swiftlint.yml'
Linting Swift files at paths
Linting 'ContentView.swift' (1/2)
Linting 'LintTestApp.swift' (2/2)
.../LintTest/ContentView.swift:17:15: warning: No Space in Method Call Violation: Don't add a space between the method name and the parentheses. (no_space_in_method_call)
Done linting! Found 1 violations, 0 serious in 2 files.
$ swiftlint autocorrect
Loading configuration from '.swiftlint.yml'
Correcting Swift files at paths
Correcting 'AnimationView.swift' (1/4)
Correcting 'LintViewModel.swift' (2/4)
Correcting 'ContentView.swift' (3/4)
Correcting 'LintTestApp.swift' (4/4)
.../LintTest/LintTest/ContentView.swift:17:15 Corrected No Space in Method Call
.../LintTest/LintTest/ContentView.swift:17:15 Corrected Opening Brace Spacing
.../LintTest/LintTest/LintViewModel.swift:20:18 Corrected No Space in Method Call
.../LintTest/LintTest/LintViewModel.swift:20:18 Corrected Opening Brace Spacing
Done correcting 4 files!

Environment

  • SwiftLint version (run swiftlint version to be sure)?
    0.39.2
  • Installation method used (Homebrew, CocoaPods, building from source, etc)?
    Homebrew
  • Paste your configuration file:
disabled_rules:
  - multiple_closures_with_trailing_closure

# Implicitly relevant:
# - no_space_in_method_call
# - opening_brace
  • Are you using nested configurations?
    No.
  • Which Xcode version are you using (check xcodebuild -version)?
    Xcode 12.0
    Build version 12A6159
    (Beta 1)
  • Do you have a sample that shows the issue?
Button { // No Space in Method Call Violation
    print("Button tapped")
} label: {
    Text("Button")
}

Additional Comments

Looking into fixing this by adjusting NoSpaceInMethodCallRule.swift. Once the issue is resolved, it may be worth considering disabling the multiple_closures_with_trailing_closure rule by default. While the new syntax isn't universally loved, it mitigates the call site ambiguity that motivated the rule's creation.

@ohayon
Copy link

ohayon commented Jul 7, 2020

I am seeing this same issue, and getting two warnings:

Button { // No Space in Method Call Violation
    print("Button tapped")
} label: { // Multiple Closures with Trailing Closure Violation
    Text("Button")
}

@giladronat
Copy link
Author

giladronat commented Jul 19, 2020

@ohayon I assumed the "Multiple Closures with Trailing Closure" to be working correctly in detecting multiple closures when a trailing closure is used, and that it could be disabled now. However, it could be modified to detect multiple trailing closures instead.

This way, it won't trigger on:

Button {
    print("Tapped")
} label: {
    Text("Button")
}

But will trigger on the current violation:

Button(action: {
    print("Tapped")
}) {
    Text("Button")
}

And trigger on a three-closure parameters function not using three trailing closure:

myMethod(param1: { "closure1" }) {
    "closure 2"
} param3: {
    "closure 3"
}

I'm playing around with the SourceKit params for structures to understand how some of the current rules around these are implemented, then learn how to determine when a closure is a proper trailing closure.

@marcelofabri marcelofabri added the bug Unexpected and reproducible misbehavior. label Aug 5, 2020
@marcelofabri
Copy link
Collaborator

@giladronat Can you open another issue to discuss multiple_closures_with_trailing_closure? Let's focus on no_space_in_method_call here 🙏

@giladronat
Copy link
Author

giladronat commented Aug 6, 2020

Wow fantastic work @marcelofabri, thank you. Yes, will gladly open another issue to discuss multiple_closures_with_trailing_closure.

Regarding #3280, if you have a moment, could you explain how the initial closure's body offset matching the body offset of the function call determines it's the beginning of trailing closures (this line)?

I'll admit I spent far too long trying to understand what differentiates a trailing closure from non-trailing closures and unnamed closure arguments in order to PR a fix I knew accounted for this case without breaking others. For example, looking at TrailingClosureRule and MultipleClosuresWithTrailingClosures gave me the impression the solution would be more involved.

@giladronat
Copy link
Author

giladronat commented Aug 10, 2020

Opened #3295 to discuss multiple_closures_with_trailing_closure in Swift 5.3.

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.

3 participants