Skip to content

Migrate MultilineParametersBracketsRule from SourceKit to SwiftSyntax #6131

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Migrate MultilineParametersBracketsRule from SourceKit to SwiftSyntax #6131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
* `file_header`
* `file_length`
* `line_length`
* `multiline_parameters_brackets`
* `trailing_whitespace`
* `vertical_whitespace`
<!-- Keep empty line to have the contributors on a separate line. -->
Expand Down
211 changes: 134 additions & 77 deletions Source/SwiftLintBuiltInRules/Rules/Style/MultilineParametersBracketsRule.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import Foundation
import SourceKittenFramework
import SwiftLintCore
import SwiftSyntax

struct MultilineParametersBracketsRule: OptInRule {
@SwiftSyntaxRule(optIn: true)
struct MultilineParametersBracketsRule: Rule {
var configuration = SeverityConfiguration<Self>(.warning)

static let description = RuleDescription(
Expand Down Expand Up @@ -89,97 +90,153 @@ struct MultilineParametersBracketsRule: OptInRule {
"""),
]
)
}

func validate(file: SwiftLintFile) -> [StyleViolation] {
violations(in: file.structureDictionary, file: file)
}
private extension MultilineParametersBracketsRule {
final class Visitor: ViolationsSyntaxVisitor<ConfigurationType> {
override func visitPost(_ node: FunctionDeclSyntax) {
checkViolations(for: node.signature.parameterClause)
}

private func violations(in substructure: SourceKittenDictionary, file: SwiftLintFile) -> [StyleViolation] {
var violations = [StyleViolation]()

// find violations at current level
if let kind = substructure.declarationKind,
SwiftDeclarationKind.functionKinds.contains(kind) {
guard
let nameOffset = substructure.nameOffset,
let nameLength = substructure.nameLength,
case let nameByteRange = ByteRange(location: nameOffset, length: nameLength),
let functionName = file.stringView.substringWithByteRange(nameByteRange)
else {
return []
}

let parameters = substructure.substructure.filter { $0.declarationKind == .varParameter }
let parameterBodies = parameters.compactMap { $0.content(in: file) }
let parametersNewlineCount = parameterBodies.map { body in
body.countOccurrences(of: "\n")
}.reduce(0, +)
let declarationNewlineCount = functionName.countOccurrences(of: "\n")
let isMultiline = declarationNewlineCount > parametersNewlineCount

if isMultiline && parameters.isNotEmpty {
if let openingBracketViolation = openingBracketViolation(parameters: parameters, file: file) {
violations.append(openingBracketViolation)
}
override func visitPost(_ node: InitializerDeclSyntax) {
checkViolations(for: node.signature.parameterClause)
}

if let closingBracketViolation = closingBracketViolation(parameters: parameters, file: file) {
violations.append(closingBracketViolation)
}
private func significantStartToken(for parameter: FunctionParameterSyntax) -> TokenSyntax? {
// The first name token (external or internal)
if parameter.firstName.tokenKind == .wildcard, let secondName = parameter.secondName {
return secondName
}
return parameter.firstName
}

// find violations at deeper levels
for substructure in substructure.substructure {
violations += self.violations(in: substructure, file: file)
private func significantEndToken(for parameter: FunctionParameterSyntax) -> TokenSyntax? {
// End of ellipsis, or type, or name (in that order of preference)
if let ellipsis = parameter.ellipsis {
return ellipsis
}
// Type is not optional, so directly use it
return parameter.type.lastToken(viewMode: .sourceAccurate) // Gets the very last token of the type
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to the .type an example like

func foo(a: T,
         b: String
             = "")

would only trigger on a but not at the closing parenthesis where it actually should. Not sure if that's relevant, though, as it's a quite strange way of formatting parameters. However, for pretty long names and types and short lines, the default value could have to be placed on a new line like in the example. Probably depends on what the previous behavior was ...

}

return violations
}
private func checkViolations(
for parameterClause: FunctionParameterClauseSyntax
) {
guard parameterClause.parameters.isNotEmpty else {
return
}

private func openingBracketViolation(parameters: [SourceKittenDictionary],
file: SwiftLintFile) -> StyleViolation? {
guard
let firstParamByteRange = parameters.first?.byteRange,
let firstParamRange = file.stringView.byteRangeToNSRange(firstParamByteRange)
else {
return nil
}
let parameters = parameterClause.parameters
let leftParen = parameterClause.leftParen
let rightParen = parameterClause.rightParen

let prefix = file.stringView.nsString.substring(to: firstParamRange.lowerBound)
let invalidRegex = regex("\\([ \\t]*\\z")
let leftParenLine = locationConverter.location(for: leftParen.positionAfterSkippingLeadingTrivia).line
let rightParenLine = locationConverter.location(for: rightParen.positionAfterSkippingLeadingTrivia).line

guard let invalidMatch = invalidRegex.firstMatch(in: prefix, options: [], range: prefix.fullNSRange) else {
return nil
}
guard let firstParam = parameters.first, let lastParam = parameters.last else { return }

return StyleViolation(
ruleDescription: Self.description,
severity: configuration.severity,
location: Location(file: file, characterOffset: invalidMatch.range.location + 1)
)
}
guard let firstParamStartToken = significantStartToken(for: firstParam),
let lastParamEndToken = significantEndToken(for: lastParam) else {
return // Should not happen with valid parameters
}

let firstParamSignificantStartLine = locationConverter.location(
for: firstParamStartToken.positionAfterSkippingLeadingTrivia
).line
let lastParamSignificantEndLine = locationConverter.location(
for: lastParamEndToken.endPositionBeforeTrailingTrivia
).line

guard isStructurallyMultiline(
parameters: parameters,
firstParam: firstParam,
firstParamStartLine: firstParamSignificantStartLine,
lastParamEndLine: lastParamSignificantEndLine,
leftParenLine: leftParenLine
) else {
return // Not structurally multiline, so this rule doesn't apply.
}

// Check if opening paren has first parameter on same line
if leftParenLine == firstParamSignificantStartLine {
violations.append(
ReasonedRuleViolation(
position: firstParam.positionAfterSkippingLeadingTrivia,
reason: "Opening parenthesis should be on a separate line when using multiline parameters"
)
)
}

private func closingBracketViolation(parameters: [SourceKittenDictionary],
file: SwiftLintFile) -> StyleViolation? {
guard
let lastParamByteRange = parameters.last?.byteRange,
let lastParamRange = file.stringView.byteRangeToNSRange(lastParamByteRange)
else {
return nil
// Check if closing paren is on same line as last parameter's significant part
if rightParenLine == lastParamSignificantEndLine {
violations.append(
ReasonedRuleViolation(
position: rightParen.positionAfterSkippingLeadingTrivia,
reason: "Closing parenthesis should be on a separate line when using multiline parameters"
)
)
}
}

let suffix = file.stringView.nsString.substring(from: lastParamRange.upperBound)
let invalidRegex = regex("\\A[ \\t]*\\)")
private func isStructurallyMultiline(
parameters: FunctionParameterListSyntax,
firstParam: FunctionParameterSyntax,
firstParamStartLine: Int,
lastParamEndLine: Int,
leftParenLine: Int
) -> Bool {
// First check if parameters themselves span multiple lines
if parameters.count > 1 && areParametersOnDifferentLines(parameters: parameters, firstParam: firstParam) {
return true
}

guard let invalidMatch = invalidRegex.firstMatch(in: suffix, options: [], range: suffix.fullNSRange) else {
return nil
// Also check if first parameter starts on a different line than opening paren
if firstParamStartLine > leftParenLine {
return true
}

// Also check if parameters span from opening to closing paren across lines
if firstParamStartLine != lastParamEndLine {
return true
}

return false
}

let characterOffset = lastParamRange.upperBound + invalidMatch.range.upperBound - 1
return StyleViolation(
ruleDescription: Self.description,
severity: configuration.severity,
location: Location(file: file, characterOffset: characterOffset)
)
private func areParametersOnDifferentLines(
parameters: FunctionParameterListSyntax,
firstParam: FunctionParameterSyntax
) -> Bool {
var previousParamSignificantEndLine = -1
if let firstParamEndToken = significantEndToken(for: firstParam) {
previousParamSignificantEndLine = locationConverter.location(
for: firstParamEndToken.endPositionBeforeTrailingTrivia
).line
}

for (index, parameter) in parameters.enumerated() {
if index == 0 { continue } // Already used firstParam for initialization

guard let currentParamStartToken = significantStartToken(for: parameter) else { continue }
let currentParamSignificantStartLine = locationConverter.location(
for: currentParamStartToken.positionAfterSkippingLeadingTrivia
).line

if currentParamSignificantStartLine > previousParamSignificantEndLine {
return true
}

if let currentParamEndToken = significantEndToken(for: parameter) {
previousParamSignificantEndLine = locationConverter.location(
for: currentParamEndToken.endPositionBeforeTrailingTrivia
).line
} else {
// If a parameter somehow doesn't have a significant end,
// fallback to its start line to avoid issues in comparison.
previousParamSignificantEndLine = currentParamSignificantStartLine
}
}
return false
}
}
}