Code action to convert between computed property and stored property

[DougGregor]

Description

A stored property with an initializer, e.g.,

static let defaultColor: Color = .red

might be better off as a computed property:

static var defaultColor: Color { .red }

or vice versa. Let's create a pair of code actions to go from the first to second or vice-versa.

[ahoppen]

Synced to Apple’s issue tracker as rdar://128016684

[AppAppWorks]

@ahoppen I'm working on this enhancement, I have a few questions for the requirement,

1. To what extent should trivia be preserved after the conversion?
2. Should stored properties without a type annotation be covered? In yes, the type annotation of the generated computed property would be left blank. e.g. let a = 1 -> var a: { 1 }
3. In a more general sense, if the conversion will result in illegal code, should the code action proceed? e.g. convert an instance computed property in an enum declaration to an instance stored property

[ahoppen]

Thanks for looking into this @AppAppWorks!

1. To what extent should trivia be preserved after the conversion?

It would be great to preserve all comments. I don’t care about whitespace that much.

2. Should stored properties without a type annotation be covered? In yes, the type annotation of the generated computed property would be left blank. e.g. let a = 1 -> var a: { 1 }

What do you think about running a SwiftLanguageService.variableTypeInfos to determine the variable’s type if it doesn’t have an explicit annotation. If that fails, I think it’s fine to leave the type blank.

3. In a more general sense, if the conversion will result in illegal code, should the code action proceed? e.g. convert an instance computed property in an enum declaration to an instance stored property

Ideally we shouldn’t offer the code action if the surrounding type cannot have a stored property.

[AppAppWorks]

Thanks for looking into this @AppAppWorks!

1. To what extent should trivia be preserved after the conversion?

It would be great to preserve all comments. I don’t care about whitespace that much.

2. Should stored properties without a type annotation be covered? In yes, the type annotation of the generated computed property would be left blank. e.g. let a = 1 -> var a: { 1 }

What do you think about running a SwiftLanguageService.variableTypeInfos to determine the variable’s type if it doesn’t have an explicit annotation. If that fails, I think it’s fine to leave the type blank.

3. In a more general sense, if the conversion will result in illegal code, should the code action proceed? e.g. convert an instance computed property in an enum declaration to an instance stored property

Ideally we shouldn’t offer the code action if the surrounding type cannot have a stored property.

Thanks for your clarifications and suggestions! I didn't know it's possible to determine variable types with SwiftLanguageService.variableTypeInfos.

[ahoppen]

@AppAppWorks have you stared work on this already? @antigluten opened swiftlang/swift-syntax#2712 to implement the syntactic transformation in swift-syntax. Would you like to collaborate on this issue.

For example @antigluten finishes his PR in swift-syntax to cover the syntactic cases and @AppAppWorks would you be interested in adding the cases you mentioned in this issue on top, like adding explicit type annotations to the computed property if it was previously inferred and restricting it to the contexts that can validly have a stored property? Or would you be interested on tackling those extended goals as well, @antigluten?

[AppAppWorks]

@AppAppWorks have you stared work on this already? @antigluten opened swiftlang/swift-syntax#2712 to implement the syntactic transformation in swift-syntax. Would you like to collaborate on this issue.

I've started work on this for a while but I'm currently stuck with finding a way to inject a language service instance into the code action.

For example @antigluten finishes his PR in swift-syntax to cover the syntactic cases and @AppAppWorks would you be interested in adding the cases you mentioned in this issue on top, like adding explicit type annotations to the computed property if it was previously inferred and restricting it to the contexts that can validly have a stored property? Or would you be interested on tackling those extended goals as well, @antigluten?

I'd definitely love to collaborate with @antigluten!

[ahoppen]

I've started work on this for a while but I'm currently stuck with finding a way to inject a language service instance into the code action.

If you’re stuck and you need help with something, feel free to put up a PR and highlight where you’ve got questions and I can help you.

[AppAppWorks]

I've started work on this for a while but I'm currently stuck with finding a way to inject a language service instance into the code action.

If you’re stuck and you need help with something, feel free to put up a PR and highlight where you’ve got questions and I can help you.

I've figured out the entry point of code actions in SwiftLanguageService

func retrieveSyntaxCodeActions(_ request: CodeActionRequest) async throws -> [CodeAction] {
  let uri = request.textDocument.uri
  let snapshot = try documentManager.latestSnapshot(uri)

  let syntaxTree = await syntaxTreeManager.syntaxTree(for: snapshot)
  guard let scope = SyntaxCodeActionScope(snapshot: snapshot, syntaxTree: syntaxTree, request: request) else {
    return []
  }
  return await allSyntaxCodeActions.concurrentMap { provider in
    return provider.codeActions(in: scope)
  }.flatMap { $0 }
}

The problem is SwiftLanguageService.variableTypeInfos is an async method while code actions run in a blocking context. What came to my mind is requesting variable type information of the entire source file every time retrieveSyntaxCodeActions is invoked and passing the info to all code actions. But I'm afraid it'd be time and resource-consuming.

[ahoppen]

The proper solution here would be that the retrieveSyntaxCodeActions method returns a code action without any edits here – it doesn’t need to know the type to determine whether the conversion between computed and stored property is possible. Then, when the user selects the code action, the editor should send a codeAction/resolve request, which computes the edits and can invoke the variableTypeInfos method.

[AppAppWorks]

1. CodeActionResolveRequest is currently left unhandled in SourceKitLSPServer.handleImpl, that means I am going to create a new switch arm for CodeActionResolveRequest and a new handler method like rename?

2. What do you think is a better identifier for a CodeAction requiring resolution? A new case element in CodeActionKind? Or a string-based marker as AnnotatedTextEdit.annotationId?

I'm also asking on behalf of #1539,

3. The effects of a type change can propagate beyond the file boundary, are there some methods/combinations of methods provided by sourcekitd that can perform searches like "Find Selected Symbol in Workspace" in Xcode?

4. Should we ask for confirmation from users if we're modifying files beyond the source file being edited?

[ahoppen]

1. CodeActionResolveRequest is currently left unhandled in SourceKitLSPServer.handleImpl, that means I am going to create a new switch arm for CodeActionResolveRequest and a new handler method like rename?

Yes

2. What do you think is a better identifier for a CodeAction requiring resolution? A new case element in CodeActionKind? Or a string-based marker as AnnotatedTextEdit.annotationId?

My understanding of the codeAction/resolve request is that it will be sent automatically from the client if CodeAction.edit is nil and the editor announces resolve support for these actions in by having the following in the client capabilities

textDocument.codeAction.resolveSupport = { properties: ['edit'] };

3. The effects of a type change can propagate beyond the file boundary, are there some methods/combinations of methods provided by sourcekitd that can perform searches like "Find Selected Symbol in Workspace" in Xcode?

SourceKit-LSP has support for find references that we should be able to re-use:

sourcekit-lsp/Sources/SourceKitLSP/SourceKitLSPServer.swift

Lines 1910 to 1934 in 607292a

	func references(
	_ req: ReferencesRequest,
	workspace: Workspace,
	languageService: LanguageService
	) async throws -> [Location] {
	let symbols = try await languageService.symbolInfo(
	SymbolInfoRequest(
	textDocument: req.textDocument,
	position: req.position
	)
	)
	guard let index = await workspaceForDocument(uri: req.textDocument.uri)?.index(checkedFor: .deletedFiles) else {
	return []
	}
	let locations = symbols.flatMap { (symbol) -> [Location] in
	guard let usr = symbol.usr else { return [] }
	logger.info("Finding references for USR \(usr)")
	var roles: SymbolRole = [.reference]
	if req.context.includeDeclaration {
	roles.formUnion([.declaration, .definition])
	}
	return index.occurrences(ofUSR: usr, roles: roles).compactMap { indexToLSPLocation($0.location) }
	}
	return locations.unique.sorted()
	}

4. Should we ask for confirmation from users if we're modifying files beyond the source file being edited?

I don’t think LSP supports any kind of confirmation dialog like that, so no.