Add 'Generate enum associated value accessors' code action#2680
Add 'Generate enum associated value accessors' code action#2680ayush-that wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a new SwiftSyntax-based refactoring code action to generate as<Case>/is<Case> accessors for enum cases, and wires it into the code action registry with a corresponding test.
Changes:
- Register the new
GenerateEnumAssociatedValueAccessorsprovider in the global syntax code action list. - Implement the enum accessor generation refactoring provider.
- Add a code action test asserting the expected workspace edit.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Tests/SourceKitLSPTests/CodeActionTests.swift | Adds a test validating the new “Generate enum associated value accessors” code action and its edit. |
| Sources/SwiftSyntaxCodeActions/SyntaxCodeActions.swift | Registers the new code action provider in the aggregated provider list. |
| Sources/SwiftSyntaxCodeActions/GenerateEnumAssociatedValueAccessors.swift | Implements the refactoring provider that inserts asX/isX computed properties for enum cases. |
| Sources/SwiftSyntaxCodeActions/CMakeLists.txt | Includes the new Swift source file in the CMake build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| markers: ["1️⃣"], | ||
| exhaustive: false | ||
| ) { uri, positions in |
| var accessors: [String] = [] | ||
| for element in caseElements { | ||
| let caseName = element.name.identifier?.name ?? element.name.text | ||
| let capitalized = caseName.prefix(1).uppercased() + caseName.dropFirst() | ||
| let casePattern = element.name.text | ||
|
|
||
| if let parameters = element.parameterClause?.parameters, !parameters.isEmpty { | ||
| let asName = "as\(capitalized)" | ||
| if usedNames.insert(asName).inserted { | ||
| accessors.append( | ||
| makeAsAccessor( | ||
| name: asName, | ||
| casePattern: casePattern, | ||
| parameters: Array(parameters), | ||
| memberIndentation: memberIndentation, | ||
| bodyIndentation: bodyIndentation | ||
| ) | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| let isName = "is\(capitalized)" | ||
| if usedNames.insert(isName).inserted { | ||
| accessors.append( | ||
| makeIsAccessor( | ||
| name: isName, | ||
| casePattern: casePattern, | ||
| memberIndentation: memberIndentation, | ||
| bodyIndentation: bodyIndentation | ||
| ) | ||
| ) | ||
| } | ||
| } |
| /// | ||
| /// After: | ||
| /// | ||
| /// ```swift |
There was a problem hiding this comment.
I think we should have two separate refactorings to generate the as and is names.
There was a problem hiding this comment.
okie. split into two.
| // Attached macros, `#if` blocks, and freestanding macro expansions can | ||
| // introduce cases or members that are not visible to a purely syntactic | ||
| // walk, which could produce incomplete accessors or collide with members | ||
| // added by macro expansion. Bail conservatively in those cases. | ||
| if !syntax.attributes.isEmpty { | ||
| throw RefactoringNotApplicableError("Enum has attributes that may be attached macros") | ||
| } |
There was a problem hiding this comment.
I don’t think we need this bail-out. If we fail to generate one is accessor, that’s not a correctness issue and if it collides with a member generated by the macro, the user will get a compilation issue, which they can also decide how to fix, we shouldn’t try to make assumptions on their behalf.
Similarly, we can generate accessors if the enum contains #if since we also won’t produce a correctness issue.
| // Infer indentation from the source rather than hardcoding it: members are | ||
| // indented one step deeper than the enum, and property bodies one step | ||
| // deeper than the members. Deriving the step from the existing members | ||
| // honors whatever width (or tabs) the file already uses. |
There was a problem hiding this comment.
I don’t think this comment provides much value
| syntax.memberBlock.members.first?.firstToken(viewMode: .sourceAccurate)?.indentationOfLine.description | ||
| ?? (enumIndentation + " ") |
There was a problem hiding this comment.
We shouldn’t assume 4 space indentation. Use BasicFormat.inferIndentation to determine the file’s indentation.
|
|
||
| if let parameters = element.parameterClause?.parameters, !parameters.isEmpty { | ||
| let asName = "as\(capitalized)" | ||
| if usedNames.insert(asName).inserted { |
There was a problem hiding this comment.
We shouldn’t generate the same name twice, so I think it’s sufficient to check usedNames.contains(asName).
f77ad8a to
ff61ae1
Compare
| guard !usedNames.contains(name) else { continue } | ||
| usedNames.insert(name) |
There was a problem hiding this comment.
You should be able to combine this in one call as follows.
| guard !usedNames.contains(name) else { continue } | |
| usedNames.insert(name) | |
| guard !usedNames.insert(name).inserted else { continue } |
There was a problem hiding this comment.
this code is gone now, along with the rest of the name tracking.
| continue | ||
| } | ||
| let name = "as\(capitalizedCaseName(of: element))" | ||
| guard !usedNames.contains(name) else { continue } |
There was a problem hiding this comment.
Why do we need to check for duplicate names here? Do you have an example?
There was a problem hiding this comment.
no. i removed the whole name tracking, including the skip for existing members. duplicate names now end up as compiler errors.
| ] | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Could you add a test case for the following source code?
enum Foo {
case value(int: Int)
case value(string: String)
}There was a problem hiding this comment.
done. i've added testGenerateEnumCaseAsAccessorsForOverloadedCaseNames.
| } | ||
| let types = parameters.map { $0.type.trimmedDescription } | ||
| let returnType: String | ||
| if parameters.count == 1 { |
There was a problem hiding this comment.
If you write it as follows, you don’t need the [0] access below.
| if parameters.count == 1 { | |
| if let parameter = parameters.only { |
| name: String, | ||
| casePattern: String, | ||
| parameters: [EnumCaseParameterSyntax], | ||
| indentation: (member: String, body: String) |
There was a problem hiding this comment.
Instead of passing the member and body indentation here, it would be cleaner to pass a base indentation and the indentation step so that we can compute the effective indentation in here. Otherwise you need to understand how indentation.member and indentation.body are used to correctly set it.
| } | ||
| } | ||
|
|
||
| extension GenerateEnumCaseAsAccessors: SyntaxRefactoringCodeActionProvider { |
There was a problem hiding this comment.
Instead of having extensions of the same type in the same file, I think it’s clearer if we just throw all members into on struct declaration.
| newText: """ | ||
|
|
||
|
|
||
| var asPoint: (Int, Int)? { |
There was a problem hiding this comment.
I think this should also contain the tuple labels, ie. this should be
| var asPoint: (Int, Int)? { | |
| var asPoint: (x: Int, y: Int)? { |
| newText: """ | ||
|
|
||
|
|
||
| var isFoo: Bool { |
There was a problem hiding this comment.
I would rather generate duplicate cases here and have a compiler error that the user needs to fix instead of implicitly assuming that isFoo should refer to the underscore version of foo.
There was a problem hiding this comment.
i agree. i've removed the name tracking, both isFoo accessors get generated and the foo/Foo test checks the duplicate output.
ff61ae1 to
f431821
Compare
|
ahoppen
left a comment
There was a problem hiding this comment.
Thanks for the update. Now, if an accessor already exists, we will create a second copy of it, right? I don’t think we should do that. Also: If all accessors already exist, we shouldn’t suggest the refactoring action.
f431821 to
4e6aeb2
Compare
|
You're right 😓. It duplicated, so I've added back the skip for accessor names that already exist, and when all of them already exist the action isn't offered anymore. |
Implements #2522.
Adds a syntactic code action on an enum that generates, per case, an
is<Case>Boolean property and (for cases with associated values) anas<Case>property returning the value(s) as an optional.For example, on:
it generates
asText/isText/asNumber/isNumber.Question (from #2544): should
is<Case>be generated for every case(current behavior), or only for cases with associated values?Testing
Adds
testGenerateEnumAssociatedValueAccessors.