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.

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

Adjustments to split FunctionParameterSyntax into multiple nodes for function parameters, closure parameters and enum parameters #495

Merged
merged 1 commit into from
Apr 5, 2023
Merged

Adjustments to split FunctionParameterSyntax into multiple nodes for function parameters, closure parameters and enum parameters #495

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
10 changes: 10 additions & 0 deletions Sources/SwiftFormat/Pipelines+Generated.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,11 @@ class LintPipeline: SyntaxVisitor {
return .visitChildren
}

override func visit(_ node: ClosureParameterSyntax) -> SyntaxVisitorContinueKind {
visitIfEnabled(NoLeadingUnderscores.visit, for: node)
return .visitChildren
}

override func visit(_ node: ClosureSignatureSyntax) -> SyntaxVisitorContinueKind {
visitIfEnabled(AlwaysUseLowerCamelCase.visit, for: node)
visitIfEnabled(ReturnVoidInsteadOfEmptyTuple.visit, for: node)
Expand Down Expand Up @@ -92,6 +97,11 @@ class LintPipeline: SyntaxVisitor {
return .visitChildren
}

override func visit(_ node: EnumCaseParameterSyntax) -> SyntaxVisitorContinueKind {
visitIfEnabled(NoLeadingUnderscores.visit, for: node)
return .visitChildren
}

override func visit(_ node: EnumDeclSyntax) -> SyntaxVisitorContinueKind {
visitIfEnabled(BeginDocumentationCommentWithOneLineSummary.visit, for: node)
visitIfEnabled(DontRepeatTypeInStaticProperties.visit, for: node)
Expand Down
97 changes: 94 additions & 3 deletions Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1122,7 +1122,7 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
// When it's parenthesized, the input is a `ParameterClauseSyntax`. Otherwise, it's a
// `ClosureParamListSyntax`. The parenthesized version is wrapped in open/close breaks so that
// the parens create an extra level of indentation.
if let parameterClause = input.as(ParameterClauseSyntax.self) {
if let parameterClause = input.as(ClosureParameterClauseSyntax.self) {
// Whether we should prioritize keeping ") throws -> <return_type>" together. We can only do
// this if the closure has arguments.
let keepOutputTogether =
Expand All @@ -1141,7 +1141,7 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
after(input.lastToken, tokens: .close)
}

arrangeParameterClause(parameterClause, forcesBreakBeforeRightParen: true)
arrangeClosureParameterClause(parameterClause, forcesBreakBeforeRightParen: true)
} else {
// Group around the arguments, but don't use open/close breaks because there are no parens
// to create a new scope.
Expand Down Expand Up @@ -1245,6 +1245,30 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
return .visitChildren
}

override func visit(_ node: ClosureParameterClauseSyntax) -> SyntaxVisitorContinueKind {
// Prioritize keeping ") throws -> <return_type>" together. We can only do this if the function
// has arguments.
if !node.parameterList.isEmpty && config.prioritizeKeepingFunctionOutputTogether {
// Due to visitation order, this .open corresponds to a .close added in FunctionDeclSyntax
// or SubscriptDeclSyntax.
before(node.rightParen, tokens: .open)
}

return .visitChildren
}

override func visit(_ node: EnumCaseParameterClauseSyntax) -> SyntaxVisitorContinueKind {
// Prioritize keeping ") throws -> <return_type>" together. We can only do this if the function
// has arguments.
if !node.parameterList.isEmpty && config.prioritizeKeepingFunctionOutputTogether {
// Due to visitation order, this .open corresponds to a .close added in FunctionDeclSyntax
// or SubscriptDeclSyntax.
before(node.rightParen, tokens: .open)
}

return .visitChildren
}

override func visit(_ node: ParameterClauseSyntax) -> SyntaxVisitorContinueKind {
// Prioritize keeping ") throws -> <return_type>" together. We can only do this if the function
// has arguments.
Expand All @@ -1257,6 +1281,37 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
return .visitChildren
}

override func visit(_ node: ClosureParameterSyntax) -> SyntaxVisitorContinueKind {
before(node.firstToken, tokens: .open)
arrangeAttributeList(node.attributes)
before(
node.secondName,
tokens: .break(.continue, newlines: .elective(ignoresDiscretionary: true)))
after(node.colon, tokens: .break)

if let trailingComma = node.trailingComma {
after(trailingComma, tokens: .close, .break(.same))
} else {
after(node.lastToken, tokens: .close)
}
return .visitChildren
}

override func visit(_ node: EnumCaseParameterSyntax) -> SyntaxVisitorContinueKind {
before(node.firstToken, tokens: .open)
before(
node.secondName,
tokens: .break(.continue, newlines: .elective(ignoresDiscretionary: true)))
after(node.colon, tokens: .break)

if let trailingComma = node.trailingComma {
after(trailingComma, tokens: .close, .break(.same))
} else {
after(node.lastToken, tokens: .close)
}
return .visitChildren
}

override func visit(_ node: FunctionParameterSyntax) -> SyntaxVisitorContinueKind {
before(node.firstToken, tokens: .open)
arrangeAttributeList(node.attributes)
Expand Down Expand Up @@ -1413,7 +1468,7 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
after(node.trailingComma, tokens: .break)

if let associatedValue = node.associatedValue {
arrangeParameterClause(associatedValue, forcesBreakBeforeRightParen: false)
arrangeEnumCaseParameterClause(associatedValue, forcesBreakBeforeRightParen: false)
}

return .visitChildren
Expand Down Expand Up @@ -2588,6 +2643,42 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
return contentsIterator.next() == nil && !commentPrecedesRightBrace
}

/// Applies formatting to a collection of parameters for a decl.
///
/// - Parameters:
/// - parameters: A node that contains the parameters that can be passed to a decl when its
/// called.
/// - forcesBreakBeforeRightParen: Whether a break should be required before the right paren
/// when the right paren is on a different line than the corresponding left paren.
private func arrangeClosureParameterClause(
_ parameters: ClosureParameterClauseSyntax, forcesBreakBeforeRightParen: Bool
) {
guard !parameters.parameterList.isEmpty else { return }

after(parameters.leftParen, tokens: .break(.open, size: 0), .open(argumentListConsistency()))
before(
parameters.rightParen,
tokens: .break(.close(mustBreak: forcesBreakBeforeRightParen), size: 0), .close)
}

/// Applies formatting to a collection of enum case parameters for a decl.
///
/// - Parameters:
/// - parameters: A node that contains the parameters that can be passed to a decl when its
/// called.
/// - forcesBreakBeforeRightParen: Whether a break should be required before the right paren
/// when the right paren is on a different line than the corresponding left paren.
private func arrangeEnumCaseParameterClause(
Copy link
Member Author

Choose a reason for hiding this comment

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

I just duplicated this method for EnumCaseParameterClauseSyntax. Alternatives would be

  1. Make this function take a leftParen: TokenSyntax, isEmpty: Bool, rightParen: TokenSyntax so the logic can be shared with arrangeParameterClause
  2. Create a protocol that offers the three parameters described in (1) and make both ParameterClauseSyntax and EnumCaseParameterClauseSyntax conform to that

(2) seemed like quite an overkill to me and (1) seemed ugly as well. Let me know if you’ve got preferences @allevato.

Copy link
Member

Choose a reason for hiding this comment

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

This is fine; the function is small enough that duplicating it is probably better than overengineering a generic thing that would only be used in one extra place.

Copy link
Member Author

@ahoppen ahoppen Mar 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That’s what I thought as well. But good to hear you agree with me.

_ parameters: EnumCaseParameterClauseSyntax, forcesBreakBeforeRightParen: Bool
) {
guard !parameters.parameterList.isEmpty else { return }

after(parameters.leftParen, tokens: .break(.open, size: 0), .open(argumentListConsistency()))
before(
parameters.rightParen,
tokens: .break(.close(mustBreak: forcesBreakBeforeRightParen), size: 0), .close)
}

/// Applies formatting to a collection of parameters for a decl.
///
/// - Parameters:
Expand Down
26 changes: 21 additions & 5 deletions Sources/SwiftFormatRules/AlwaysUseLowerCamelCase.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,16 @@ public final class AlwaysUseLowerCamelCase: SyntaxLintRule {
diagnoseLowerCamelCaseViolations(
param.name, allowUnderscores: false, description: identifierDescription(for: node))
}
} else if let parameterClause = input.as(ParameterClauseSyntax.self) {
} else if let parameterClause = input.as(ClosureParameterClauseSyntax.self) {
for param in parameterClause.parameterList {
diagnoseLowerCamelCaseViolations(
param.firstName, allowUnderscores: false, description: identifierDescription(for: node))
if let secondName = param.secondName {
diagnoseLowerCamelCaseViolations(
secondName, allowUnderscores: false, description: identifierDescription(for: node))
}
}
} else if let parameterClause = input.as(EnumCaseParameterClauseSyntax.self) {
for param in parameterClause.parameterList {
if let firstName = param.firstName {
diagnoseLowerCamelCaseViolations(
Expand All @@ -84,6 +93,15 @@ public final class AlwaysUseLowerCamelCase: SyntaxLintRule {
secondName, allowUnderscores: false, description: identifierDescription(for: node))
}
}
} else if let parameterClause = input.as(ParameterClauseSyntax.self) {
for param in parameterClause.parameterList {
diagnoseLowerCamelCaseViolations(
param.firstName, allowUnderscores: false, description: identifierDescription(for: node))
if let secondName = param.secondName {
diagnoseLowerCamelCaseViolations(
secondName, allowUnderscores: false, description: identifierDescription(for: node))
}
}
}
}
return .visitChildren
Expand All @@ -106,10 +124,8 @@ public final class AlwaysUseLowerCamelCase: SyntaxLintRule {
for param in node.signature.input.parameterList {
// These identifiers aren't described using `identifierDescription(for:)` because no single
// node can disambiguate the argument label from the parameter name.
if let label = param.firstName {
diagnoseLowerCamelCaseViolations(
label, allowUnderscores: false, description: "argument label")
}
diagnoseLowerCamelCaseViolations(
param.firstName, allowUnderscores: false, description: "argument label")
if let paramName = param.secondName {
diagnoseLowerCamelCaseViolations(
paramName, allowUnderscores: false, description: "function parameter")
Expand Down
2 changes: 1 addition & 1 deletion Sources/SwiftFormatRules/AmbiguousTrailingClosureOverload.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public final class AmbiguousTrailingClosureOverload: SyntaxLintRule {
for fn in functions {
let params = fn.signature.input.parameterList
guard let firstParam = params.firstAndOnly else { continue }
guard let type = firstParam.type, type.is(FunctionTypeSyntax.self) else { continue }
guard firstParam.type.is(FunctionTypeSyntax.self) else { continue }
if let mods = fn.modifiers, mods.has(modifier: "static") || mods.has(modifier: "class") {
staticOverloads[fn.identifier.text, default: []].append(fn)
} else {
Expand Down
2 changes: 1 addition & 1 deletion Sources/SwiftFormatRules/FunctionDeclSyntax+Convenience.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ extension FunctionDeclSyntax {
/// Constructs a name for a function that includes parameter labels, i.e. `foo(_:bar:)`.
var fullDeclName: String {
let params = signature.input.parameterList.map { param in
"\(param.firstName?.text ?? "_"):"
"\(param.firstName.text):"
}
return "\(identifier.text)(\(params.joined()))"
}
Expand Down
18 changes: 17 additions & 1 deletion Sources/SwiftFormatRules/NoLeadingUnderscores.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,15 @@ public final class NoLeadingUnderscores: SyntaxLintRule {
return .visitChildren
}

public override func visit(_ node: FunctionParameterSyntax) -> SyntaxVisitorContinueKind {
public override func visit(_ node: ClosureParameterSyntax) -> SyntaxVisitorContinueKind {
// If both names are provided, we want to check `secondName`, which will be the parameter name
// (in that case, `firstName` is the label). If only one name is present, then it is recorded in
// `firstName`, and it is both the label and the parameter name.
diagnoseIfNameStartsWithUnderscore(node.secondName ?? node.firstName)
return .visitChildren
}

public override func visit(_ node: EnumCaseParameterSyntax) -> SyntaxVisitorContinueKind {
// If both names are provided, we want to check `secondName`, which will be the parameter name
// (in that case, `firstName` is the label). If only one name is present, then it is recorded in
// `firstName`, and it is both the label and the parameter name.
Expand All @@ -66,6 +74,14 @@ public final class NoLeadingUnderscores: SyntaxLintRule {
return .visitChildren
}

public override func visit(_ node: FunctionParameterSyntax) -> SyntaxVisitorContinueKind {
// If both names are provided, we want to check `secondName`, which will be the parameter name
// (in that case, `firstName` is the label). If only one name is present, then it is recorded in
// `firstName`, and it is both the label and the parameter name.
diagnoseIfNameStartsWithUnderscore(node.secondName ?? node.firstName)
return .visitChildren
}

public override func visit(_ node: GenericParameterSyntax) -> SyntaxVisitorContinueKind {
diagnoseIfNameStartsWithUnderscore(node.name)
return .visitChildren
Expand Down
7 changes: 3 additions & 4 deletions Sources/SwiftFormatRules/UseSynthesizedInitializer.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,7 @@ public final class UseSynthesizedInitializer: SyntaxLintRule {
guard parameters.count == properties.count else { return false }
for (idx, parameter) in parameters.enumerated() {

guard let paramId = parameter.firstName, parameter.secondName == nil else { return false }
guard let paramType = parameter.type else { return false }
guard parameter.secondName == nil else { return false }

let property = properties[idx]
let propertyId = property.firstIdentifier
Expand All @@ -124,9 +123,9 @@ public final class UseSynthesizedInitializer: SyntaxLintRule {
return false
}

if propertyId.identifier.text != paramId.text
if propertyId.identifier.text != parameter.firstName.text
|| propertyType.description.trimmingCharacters(
in: .whitespaces) != paramType.description.trimmingCharacters(in: .whitespacesAndNewlines)
in: .whitespaces) != parameter.type.description.trimmingCharacters(in: .whitespacesAndNewlines)
{ return false }
}
return true
Expand Down
4 changes: 1 addition & 3 deletions Sources/SwiftFormatRules/ValidateDocumentationComments.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,9 +139,7 @@ fileprivate func funcParametersIdentifiers(in paramList: FunctionParameterListSy
// If there is a label and an identifier, then the identifier (`secondName`) is the name that
// should be documented. Otherwise, the label and identifier are the same, occupying
// `firstName`.
guard let parameterIdentifier = parameter.secondName ?? parameter.firstName else {
continue
}
let parameterIdentifier = parameter.secondName ?? parameter.firstName
funcParameters.append(parameterIdentifier.text)
}
return funcParameters
Expand Down
2 changes: 1 addition & 1 deletion Sources/generate-pipeline/RuleCollector.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ final class RuleCollector {
guard let function = member.decl.as(FunctionDeclSyntax.self) else { continue }
guard function.identifier.text == "visit" else { continue }
let params = function.signature.input.parameterList
guard let firstType = params.firstAndOnly?.type?.as(SimpleTypeIdentifierSyntax.self) else {
guard let firstType = params.firstAndOnly?.type.as(SimpleTypeIdentifierSyntax.self) else {
continue
}
visitedNodes.append(firstType.name.text)
Expand Down