Convert implicitly unwrapped optionals to proper optionals

[AppAppWorks]

fixed #1320

[ahoppen]

Very nice.

[ahoppen]

As far as I can tell, we don’t use this type in tests, so ConvertIUOToProperOptional doesn’t need to be @_spi or public but can be internal instead.

[ahoppen]

I’d suggest that we’re a little more verbose here and spell out IUO as ImplicitlyUnwrappedOptional. It’s a little more verbose but makes the code action easier to newcomers to the code base.

Also, let’s just call the ? syntax Optional instead of Proper Optional.

Suggested change

	public struct ConvertIUOToProperOptional: SyntaxRefactoringProvider {
	public struct ConvertImplicitlyUnwrappedOptionalToOptional: SyntaxRefactoringProvider {

[AppAppWorks]

Agree, I was under a false impression there was a restriction on the maximum length of any file name, as I had never come across a file name longer than this!

[ahoppen]

Suggested change

	static let title: String = "Convert Implicitly Unwrapped Optional to Proper Optional"
	static let title: String = "Convert Implicitly Unwrapped Optional to Optional"

[ahoppen]

I think we can save all the type aliases if we spell out the types in the function signature. I think it also makes the code easier to read because you don’t always need to cross-reference the type aliases when reading the function signature of refactor.

Suggested change

	public typealias Input = ImplicitlyUnwrappedOptionalTypeSyntax
	public typealias Context = Void
	public typealias Output = OptionalTypeSyntax
	@_spi(Testing)
	public static func refactor(syntax: Input, in context: Context) -> Output? {
	@_spi(Testing)
	public static func refactor(syntax: ImplicitlyUnwrappedOptionalTypeSyntax, in context: Void) -> OptionalTypeSyntax? {

[ahoppen]

Is there any reason why you didn’t use findParentOrSelf(ofType:stoppingIf:) like the other refactoring actions?

I think the benefit of that would be that the refactoring applies in more situations. Eg. in your test case, you would also get the code action when placing the cursor in Int, which seems reasonable to me.

Something like

Suggested change

	guard let token = scope.innermostNodeContainingRange else {
	return nil
	}
	return if let iuoType = token.as(Input.self) ?? token.parent?.as(Input.self) {
	iuoType
	} else if token.is(TokenSyntax.self),
	let wrappedType = token.parent?.as(TypeSyntax.self),
	let iuoType = wrappedType.parent?.as(Input.self),
	iuoType.wrappedType == wrappedType
	{
	iuoType
	} else {
	nil
	}
	return scope.innermostNodeContainingRange?.findParentOfSelf(
	ofType: ImplicitlyUnwrappedOptionalTypeSyntax.self,
	stoppingIf: { $0.is(CodeBlockSyntax.self) || $0.is(MemberBlockSyntax.self) }
	}

[AppAppWorks]

I didn't use findParentOrSelf(ofType:stoppingIf:) like other refactoring actions because I wanted to restrict the scope of this code action to the nearest type syntax code that might make local reasoning easier and the namespace of code actions cleaner?

Say, if this code action is triggered by pointing to a type situated deeply inside a heavily nested IUO, it may confuse me a bit.

[ahoppen]

I think it’s fair to assume that usually types are not nested too deeply and in the past we’ve had more issues with being to restrictive with the scopes in which refactorings are shown than being to widely applicable.

[AppAppWorks]

In my original plan, I wanted to insert optional chaining for any code affected by the refactoring in the same source file, do you think it's a worthwhile and feasible effort?

[ahoppen]

In my original plan, I wanted to insert optional chaining for any code affected by the refactoring in the same source file, do you think it's a worthwhile and feasible effort?

The question is: What exactly should we do here? I can think of essentially three options:

1. If we want to make the code compile after the refactoring, we should introduce force unwraps at every location that the refactored because otherwise types might become optional that weren’t before.
2. We could introduce optional chaining in all member accesses, which possibly makes some code compile but introduces compilation errors where this produces nil where nil wasn’t acceptable before
3. We don’t do anything at the references and leave it to the user to fix things up

I would be leaning towards option (1) even though I don’t like introducing force unwraps to the user’s code base.

Is it worthwhile?

I think the biggest win with such an implementation is to have a blueprint of a refactoring that uses information from sourcekitd and the index to then perform syntactic refactorings. This refactoring in itself isn’t all that interesting, I think but it could clear the pathway for quite a few interesting refactorings. Some that come to my mind would be:

• Changing the order of function parameters
• Changing an initializer to a static method
• Changing a function to an operator

[AppAppWorks]

Having looked into the spec of LSP for a while, I think we need to add a new field data to CodeAction,

public struct CodeAction: ResponseType, Hashable {
...
  /// A data entry field that is preserved between a `CodeActionRequest` and a `CodeActionResolveRequest`.
  public var data: LSPAny?
...

  public init(
    title: String,
    kind: CodeActionKind? = nil,
    diagnostics: [Diagnostic]? = nil,
    edit: WorkspaceEdit? = nil,
    command: Command? = nil,
    data: LSPAny? = nil
  ) {
    self.title = title
    self.kind = kind
    self.diagnostics = diagnostics
    self.edit = edit
    self.command = command
    self.data = data
  }
}

To play it safe, I think we need to keep empty both command and edit in the CodeAction returned and put all necessary information for CodeActionResolveRequest in data, as the official spec provides no guarantee that nil edit is a necessary and sufficient condition for issuing a CodeActionResolveRequest,

This(Code Action Resolve Request) is usually used to compute the edit property of a code action to avoid its unnecessary computation during the textDocument/codeAction request.

Our most pessimistic expectation may be any well-behaving and non-assuming sourcekit-LSP client should issue a CodeActionResolveRequest after receiving a CodeAction with nil edit and nil command?

[ahoppen]

Having looked into the spec of LSP for a while, I think we need to add a new field data to CodeAction,

Oh yes, the isPreferred and disabled fields from the LSP spec are also missing and should be added as well.

To play it safe, I think we need to keep empty both command and edit in the CodeAction returned and put all necessary information for CodeActionResolveRequest in data, as the official spec provides no guarantee that nil edit is a necessary and sufficient condition for issuing a CodeActionResolveRequest,
Our most pessimistic expectation may be any well-behaving and non-assuming sourcekit-LSP client should issue a CodeActionResolveRequest after receiving a CodeAction with nil edit and nil command?

It would be good to double-check how a few editors behave but I assume that you’re right here. We should, however, populate edits and command if the client doesn’t support resolving these properties using codeAction/resolve request.

[AppAppWorks]

I think the biggest win with such an implementation is to have a blueprint of a refactoring that uses information from sourcekitd and the index to then perform syntactic refactorings. This refactoring in itself isn’t all that interesting, I think but it could clear the pathway for quite a few interesting refactorings. Some that come to my mind would be:

• Changing the order of function parameters
• Changing an initializer to a static method
• Changing a function to an operator

I can envision the necessity of using index for the resolution of this code action, it then implies if the index wasn't available the resolution couldn't proceed. Should we introduce a mechanism to disable code actions when certain conditions aren't met?

[ahoppen]

I think it would be fine to offer the code action and then return an error response from codeAction/resolve is something goes wrong.