Repository navigation
Add Expand Derived Conformance refactoring - #2780
Paul-Passeron wants to merge 5 commits into
Conversation
bc448fa to
eb18933
Compare
|
@swift-ci please test |
|
@swift-ci please test windows |
| try await SkipUnless.sourcekitdSupportsExpandDerivedConformance() | ||
|
|
||
| let testClient = try await TestSourceKitLSPClient( | ||
| options: SourceKitLSPOptions.deriveConformancesViaMacros(), |
There was a problem hiding this comment.
I think it's also worth adding a test case that makes sure we don't get the refactoring action when we don't pass the experimental feature
|
|
||
| /// The sourcekitd identifier of the refactoring action. | ||
| package var actionString = "source.refactoring.kind.expand.macro" | ||
| package static let actionString = "source.refactoring.kind.expand.macro" |
| /// A cursor position is only ever applicable to a single one of them. Expanding a | ||
| /// macro requires the cursor to be on the macro, and expanding a derived conformance | ||
| /// requires it to be on a protocol in the inheritance clause. | ||
| private static let expansionActionUIDs = [ | ||
| ExpandMacroCommand.actionString, | ||
| ExpandDerivedConformanceCommand.actionString, | ||
| ] |
There was a problem hiding this comment.
Embedding the information in MacroExpansionReferenceDocumentURLData sounds like a reasonable idea. It's not worth to make a separate ReferenceDocumentURL case.
| if action.actionString == ExpandDerivedConformanceCommand.actionString { | ||
| canExpandDerivedConformance = true | ||
| return nil |
There was a problem hiding this comment.
I wonder if we should also consider exposing an "inline" refactoring for cases where you want to explicitly have the witnesses written?
There was a problem hiding this comment.
My concern with this is Codable since the type witnesses aren't synthesised by macro, they are missing after inlining and you end up with invalid code. I remember the issue being that type witnesses could be requested multiple times which didn't play well with the macro expansions but I could be wrong.
There was a problem hiding this comment.
I'm not sure that's a reason not to provide the refactoring, we really should just include CodingKeys in the expansion even if it isn't strictly a witness
There was a problem hiding this comment.
I added the inline refactoring
| ) | ||
| } catch { | ||
| logger.debug("Failed to run \(actionUID) for \(macroExpansionURLData.bufferName): \(error.forLogging)") | ||
| continue |
There was a problem hiding this comment.
(Not relevant anymore if we embed the UID information in the URL, but)
Mild concern with this possibly hides the real error like sourcekitd errors, or cancellation.
At least we should rethrow CancellationError. For other errors, I'd rethrow the last real error caught.
| } | ||
| } | ||
|
|
||
| extension SourceKitLSPOptions { |
There was a problem hiding this comment.
I don’t think that SkipUnless is the right file for this extension. I’d move it to a separate file.
| package static func deriveConformancesViaMacros(enabled: Bool = true) async throws -> SourceKitLSPOptions { | ||
| let toolchain = try await unwrap(ToolchainRegistry.forTesting.default) | ||
| var options = try await SourceKitLSPOptions.testDefault() | ||
| var flags = ["-plugin-path", try toolchain.path.appending(components: "lib", "swift", "host", "plugins").filePath] |
There was a problem hiding this comment.
Instead of setting this on tests, should these arguments be included in the fallback arguments we use inside SourceKit-LSP?
If we do this, we might not need this extension at all. SourceKitLSPOptions.testDefault could just gain a parameter of the experimental features to enable.
| ) | ||
| ) | ||
| guard case .commands(let commands) = response else { | ||
| XCTFail("Expected commands, got \(String(describing: response))") |
There was a problem hiding this comment.
| XCTFail("Expected commands, got \(String(describing: response))") | |
| XCTFail("Expected commands, got \(response)") |
| let expansionURI = try XCTUnwrap(peekDocumentsRequestURIs.value?.only) | ||
| let expansion = try await testClient.send(GetReferenceDocumentRequest(uri: expansionURI)) | ||
| XCTAssert( | ||
| expansion.content.contains("static func __derived_struct_equals(_ lhs: Self, _ rhs: Self) -> Swift::Bool"), |
There was a problem hiding this comment.
High-level question: Is this generated function really something we want to surface to users? If we allow them to inline it, they may start modifying it instead of implementing a custom func ==, which would probably be the preferred solution.
|
|
||
| XCTAssertEqual(innerMacroExpansion.content, #"(1 + 2, "1 + 2")"#) | ||
| } | ||
|
|
There was a problem hiding this comment.
Just anchoring this conversation at a random place.
I suppose this would also be able to generate explicit Codable conformances, right? If so, should we suppress the Add Explicit Coddle Implementation refactoring currently offered by sourcekitd? Or is this already happening on the sourcekitd side?
4 participants
Following the work in, and depending on swiftlang/swift#91811.
Adds a code action to expand conformances that the compiler derives through macros (the experimental
DeriveConformancesViaMacrosfeature).The expansion is shown the same way as Expand Macro and reuses a lot of the same machinery.
ExpandDerivedConformanceCommand, which wraps sourcekitd'ssource.refactoring.kind.expand.derived_conformanceaction. It is offered only when sourcekitd reports that action, so projects that don't use the feature shouldn't see it.MacroExpansionManagerwork with any expansion refactoring, not just macros, so thatworkspace/getReferenceDocumentcan resolve derived-conformance buffers.testExpandDerivedConformance. It is skipped on toolchains that don't support the refactoring.