Added null check to some adapters and changed type definition

[ayame113]

Some LSP requests lack null checking and are causing errors, so I fixed them.

I made the following changes:

1. Added null check (e20bba5)
2. Fixed the type definition (85cc505)
3. Use the vscode-languageserver-protocol package for type definition (https://github.com/atom-community/atom-languageclient/compare/85cc505..d665159)

If you don't need the third change, reverting it will still work.

[UziTech]

I think we were discussing this in #132 (comment)

I'm not sure if it is better to throw an error if the lsp returns something invalid or fail silently like this PR. Is null or undefined a valid return for .codeAction?

[aminya]

I think we were discussing this in #132 (comment)

This is different though.

[UziTech]

This is different though.

How? Why are these checks necessary if null or undefined are not correct outputs of .codeAction anyway?

If null or undefined are valid outputs then we should change the types to reflect that.

[aminya]

I meant the changes under lib/languageclient.ts which seem reasonable as they are using the original types.

[UziTech]

I meant the changes under lib/languageclient.ts which seem reasonable as they are using the original types.

Ya I think those changes are good. I'm just not sure about null checks.

[ayame113]

(some comment)

• Currently, the vscode-languageserver-protocol module is used for type definition. In this PR, I also want to use it in the return value of the language-client method.
• Since the return type is not exported directly from the module, I am using the generics of the function.
  • I used it as a reference for implementation
    https://github.com/microsoft/vscode-languageserver-node/blob/d58c00bbf8837b9fd0144924db5e7b1c543d839e/client/src/common/client.ts#L83-L86
• This will cause a type error if the wrong return type.
  
• Removed null from the return value of the following request according to the type definition
  • completionItem / resolve
  • codeLens / resolve
• Added null
  • textDocument/references
  • textDocument/documentHighlight
  • textDocument/documentSymbol
  • workspace/symbol
  • textDocument/codeAction
  • textDocument/codeLens
  • textDocument/documentLink
  • textDocument/formatting
  • textDocument/rangeFormatting
  • textDocument/onTypeFormatting
  • textDocument/rename
• I think this implementation fits the spec, but you can leave null in the return value if you want to deal with invalid null checks outside the spec.

[UziTech]

According to codeAction it returns Promise<Array<lsp.Command | lsp.CodeAction>> so a promise resolving to null or undefined is an invalid return type and should probably throw an error (as it currently does) instead of being handled the same as an empty array. Or the return type should be updated to include null and undefined as valid returns.

[UziTech]

I tend to prefer throwing a valid error instead of failing silently like this. @aminya This is what I was talking about in #132 (comment) when I said "types do not catch runtime errors".

[ayame113]

Thank you for your review!

The spec seems to say that codeAction returns (Command | CodeAction) [] | null. Is it not necessary to raise an error if the language server returns valid null?

https://microsoft.github.io/language-server-protocol/specifications/specification-current/#textDocument_codeAction
Request:

• method: ‘textDocument/codeAction’

Response:

• result: (Command | CodeAction)[] | null

By the way, such a change is also an option.

 public static async getCodeActions(
   ...
-): Promise<atomIde.CodeAction[]> {
+): Promise<atomIde.CodeAction[] | null> {
   if (linterAdapter == null) {
     return []
   }
   assert(serverCapabilities.codeActionProvider, "Must have the textDocument/codeAction capability")
   const params = CodeActionAdapter.createCodeActionParams(linterAdapter, editor, range, diagnostics)
   const actions = await connection.codeAction(params)
-  return (actions || []).map((action) => CodeActionAdapter.createCodeAction(action, connection))
+  if (!actions) {return null}
+  return actions.map((action) => CodeActionAdapter.createCodeAction(action, connection))
 }

[UziTech]

I like the early null check rather than still mapping over an empty array.

We should change the return type of codeAction if null is a valid response according to the spec.

NVM I see you already did that. 👍

[ayame113]

At e7c26b4, I changed it to return null instead of an empty array when actions value is null.

[aminya]

I'll review today or tomorrow

[github-actions]

🎉 This PR is included in version 1.8.3 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀