Fix multiline_call_arguments with enum-case patterns.#6444
Open
GandaLF2006 wants to merge 4 commits intorealm:mainfrom
Open
Fix multiline_call_arguments with enum-case patterns.#6444GandaLF2006 wants to merge 4 commits intorealm:mainfrom
GandaLF2006 wants to merge 4 commits intorealm:mainfrom
Conversation
GandaLF2006
force-pushed
the
gandalf2006/multiline_call_arguments-improve
branch
from
295f111 to
241eec6
Compare
Generated by 🚫 Danger |
GandaLF2006
force-pushed
the
gandalf2006/multiline_call_arguments-improve
branch
from
241eec6 to
2800379
Compare
GandaLF2006
force-pushed
the
gandalf2006/multiline_call_arguments-improve
branch
from
2800379 to
1cb7ca7
Compare
Contributor
Author
|
@SimplyDanny Hey-hey. One more PR to fix rule :) |
GandaLF2006
force-pushed
the
gandalf2006/multiline_call_arguments-improve
branch
3 times, most recently
from
e553709 to
ff1b214
Compare
GandaLF2006
force-pushed
the
gandalf2006/multiline_call_arguments-improve
branch
2 times, most recently
from
30e2640 to
b4c8cca
Compare
GandaLF2006
force-pushed
the
gandalf2006/multiline_call_arguments-improve
branch
2 times, most recently
from
c4db9e6 to
2155214
Compare
GandaLF2006
force-pushed
the
gandalf2006/multiline_call_arguments-improve
branch
2 times, most recently
from
93019ad to
6ab0d0c
Compare
GandaLF2006
force-pushed
the
gandalf2006/multiline_call_arguments-improve
branch
2 times, most recently
from
992a540 to
986ca2c
Compare
SimplyDanny
requested changes
Apr 1, 2026
Collaborator
SimplyDanny
left a comment
SimplyDanny
left a comment
There was a problem hiding this comment.
Thanks for the update! Please consider my comments.
| description: "Call should have each parameter on a separate line", | ||
| description: """ | ||
| Enforces one-argument-per-line for multi-line calls; \ | ||
| optionally limits or forbids multi-argument single-line calls via configuration |
Collaborator
There was a problem hiding this comment.
Suggested change
| optionally limits or forbids multi-argument single-line calls via configuration | |
| optionally limits or forbids multi-argument single-line calls via configuration. |
| struct MultilineCallArgumentsRule: Rule { | ||
| var configuration = MultilineCallArgumentsConfiguration() | ||
|
|
||
| enum Reason { |
Collaborator
There was a problem hiding this comment.
Each of them is only used once, isn't it? Would be fine to inline them.
Contributor
Author
There was a problem hiding this comment.
I kept Reason intentionally as a single source of truth for violation messages used by both the rule and the unit tests. This avoids duplicating strings in tests and prevents message drift if we ever tweak wording.
Collaborator
There was a problem hiding this comment.
Okay. I missed the them being used in unit tests as well.
| ), b: 3 | ||
| ) | ||
| """, | ||
| configuration: ["max_number_of_single_line_parameters": 10] |
Collaborator
There was a problem hiding this comment.
What is this configuration good for?
| a: ( | ||
| 1, | ||
| 2 | ||
| ), b: 3 |
Collaborator
There was a problem hiding this comment.
Is this really multi-line? 😅
| guard maxNumberOfSingleLineParameters >= 1 else { | ||
| throw Issue.inconsistentConfiguration( | ||
| ruleID: Parent.identifier, | ||
| message: "Option '\($maxNumberOfSingleLineParameters.key)' should be >= 1." |
Collaborator
There was a problem hiding this comment.
We are quite inconsistent with that in general, but I like it more with the period. And in this case, the similar message in other rules also use it.
| var checkedExpressionPattern = false | ||
| var checkedValueBindingPattern = false | ||
|
|
||
| while let node = current { |
Collaborator
There was a problem hiding this comment.
I wonder what an example would be that really requires walking up the whole tree. Isn't it enough to check 1-2 parent levels for specific nodes like MatchingPatternConditionSyntax and others?
Contributor
Author
There was a problem hiding this comment.
Good question. The short version is: the relevant “pattern container” isn’t consistently 1–2 parent hops away from the .caseOne(...) node.
For example, in:
if case let .caseOne(_, _, three, _) = value { }SwiftSyntax models .caseOne(...) as a FunctionCallExprSyntax, but it’s usually wrapped by pattern nodes first. In other words, the parent chain often looks like:
FunctionCallExprSyntax → ExpressionPatternSyntax → ValueBindingPatternSyntax → MatchingPatternConditionSyntax
If we only look 1–2 levels up, we can miss MatchingPatternConditionSyntax and end up linting a pattern as if it were a real call (the exact false positive this PR fixes).
Also, we’re not walking to the root in the normal case: once we hit a top-level container (if/guard/while case, switch case, for case, catch), we check whether we’re inside its pattern subtree and then stop (break). So it stays bounded, but robust.
Collaborator
There was a problem hiding this comment.
Got your point. However, as long as it's only a hand full of cases to check explicitly, I'd still go with them instead of the generic solution for the sake of simplicity.
Also, the isInside doesn't have an early exit, has it?
Contributor
Author
There was a problem hiding this comment.
The pattern subtree can be nested deeper than 1-2 levels. For case .foo(let a), .foo(...) is inside ExpressionPatternSyntax → ValueBindingPatternSyntax → MatchingPatternConditionSyntax. We need to walk up to verify we're in the pattern, not the body.
The current code does stop once it finds a container and confirms position — it just uses isInside to make that verification simple.
| a: ( | ||
| 1, | ||
| 2 | ||
| ), b: 3 |
Collaborator
There was a problem hiding this comment.
Is this really multi-line? 😅
| try MultilineCallArgumentsRule(configuration: config) | ||
| } | ||
|
|
||
| private func validate(_ rule: some Rule, contents: String) -> [StyleViolation] { |
Collaborator
There was a problem hiding this comment.
Could accept the config and create the rule instance itself.
Contributor
Author
There was a problem hiding this comment.
Good suggestion — agreed. I’ll update the test helper to accept a config dictionary and instantiate MultilineCallArgumentsRule internally.
GandaLF2006
force-pushed
the
gandalf2006/multiline_call_arguments-improve
branch
from
986ca2c to
c7e1b1b
Compare
Contributor
Author
@SimplyDanny Fixed your comments. Please re-review :) |
SimplyDanny
reviewed
Apr 7, 2026
| struct MultilineCallArgumentsRule: Rule { | ||
| var configuration = MultilineCallArgumentsConfiguration() | ||
|
|
||
| enum Reason { |
Collaborator
There was a problem hiding this comment.
Okay. I missed the them being used in unit tests as well.
| var checkedExpressionPattern = false | ||
| var checkedValueBindingPattern = false | ||
|
|
||
| while let node = current { |
Collaborator
There was a problem hiding this comment.
Got your point. However, as long as it's only a hand full of cases to check explicitly, I'd still go with them instead of the generic solution for the sake of simplicity.
Also, the isInside doesn't have an early exit, has it?
Source/SwiftLintBuiltInRules/Rules/Lint/MultilineCallArgumentsRuleExamples.swift
Outdated
Show resolved
Hide resolved
…RuleExamples.swift Co-authored-by: Danny Mösch <danny.moesch@icloud.com>
GandaLF2006
force-pushed
the
gandalf2006/multiline_call_arguments-improve
branch
from
22530d5 to
30289cf
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes false positives in multiline_call_arguments when enum-case patterns (e.g. .caseOne(...)) are used in pattern-matching positions. SwiftSyntax represents such patterns as FunctionCallExprSyntax, so the rule previously treated them as real calls and produced violations.
Problem
The rule could incorrectly lint enum-case patterns in pattern-matching constructs, e.g.:
This is a pattern, not a function call, but it was previously linted as a call expression.
Fix
We now ignore FunctionCallExprSyntax nodes only when they are located inside a pattern subtree (pattern-part only), by walking up the parent chain and checking containment within pattern containers: