Migrate MultilineParametersBracketsRule from SourceKit to SwiftSyntax #6131
Conversation
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
Generated by 🚫 Danger |
|
An analysis of the OSS Check results from Claude:
|
64dcbac to
4d6e508
Compare
## Summary Convert MultilineParametersBracketsRule to use SwiftSyntax instead of SourceKit for improved performance and better detection of multiline parameter formatting violations. ## Key Technical Improvements - **Enhanced multiline detection** distinguishing between structurally multiline parameters and parameters with multiline default values - **Accurate position reporting** using SwiftSyntax's precise token locations - **Better handling of default values** by focusing on parameter structure rather than content - **Improved performance** using visitor pattern over regex-based SourceKit analysis - **Reduced false positives** for single parameters with multiline default values ## Migration Details - Replaced `ASTRule` with `@SwiftSyntaxRule(optIn: true)` annotation - Implemented `ViolationsSyntaxVisitor` pattern for function and initializer parameter analysis - Added helper methods to extract significant tokens (name/type) excluding default values - Converted regex-based bracket detection to SwiftSyntax position comparisons - Maintained full compatibility with existing rule behavior and test cases
4d6e508 to
3a1a472
Compare
That cannot be entirely true. The numbers of fixed and new violations is not equal. So there must be a few that are only fixed and a few that are only new and thus have not just their message changed. The ones I found look valid though. I'm trying to improve the categorization further in #6139. |
SimplyDanny
reviewed
Jun 25, 2025
| return ellipsis | ||
| } | ||
| // Type is not optional, so directly use it | ||
| return parameter.type.lastToken(viewMode: .sourceAccurate) // Gets the very last token of the type |
There was a problem hiding this comment.
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 ...
oss-check-summary.md is the report with improved categorization and this PR cherry-picked on top. There are indeed a few fixed and new violations that differ in more than the message. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Convert MultilineParametersBracketsRule to use SwiftSyntax instead of
SourceKit for improved performance and better detection of multiline
parameter formatting violations.
Key Technical Improvements
multiline parameters and parameters with multiline default values
locations
structure rather than content
SourceKit analysis
default values
Migration Details
ASTRulewith@SwiftSyntaxRule(optIn: true)annotationViolationsSyntaxVisitorpattern for function andinitializer parameter analysis
excluding default values
comparisons
cases