nwolverson / purescript-language-server

MIT License
185 stars 42 forks source link

Hover (show documentation) does not work with recent coc.nvim #185

Closed k-jsmith01 closed 2 years ago

k-jsmith01 commented 2 years ago

Does anyone also have problems with show documentation (default shortcut “K”) in current version of coc.nvim?

A couple weeks (months?) ago everything went fine. Now I get

[coc.nvim] hover not found

when pressing K on a symbol, which previously provided helpful type info in a popup menu.

:CocInfo
vim version: NVIM v0.7.2
node version: v16.17.0
coc.nvim version: 0.0.82-347e33d7

npm package versions:

$ npm -g list
├── corepack@0.12.1
├── npm@8.15.0
├── purescript-language-server@0.16.6
├── purescript@0.15.4
├── purty@7.0.0
└── spago@0.20.9

Example config from GitHub - neoclide/coc.nvim:

" Use K to show documentation in preview window.
nnoremap <silent> K :call ShowDocumentation()<CR>

function! ShowDocumentation()
  if CocAction('hasProvider', 'hover')
    call CocActionAsync('doHover')
  else
    call feedkeys('K', 'in')
  endif
endfunction

I know there have been breaking changes around the custom popup menu. Is there anything I need to change in the vim config - or has hover functionality been affected/changed/broken in the language server?

(Note, this is a cross-post from https://discourse.purescript.org/t/vim-ide-options/1477/16)

nwolverson commented 2 years ago

There have been no changes in the language server.

k-jsmith01 commented 2 years ago

Thanks anyway. Hoped to find an clue about its cause, unfortunately I am still out of luck.

stevenyap commented 2 years ago

@k-jsmith01 This has happened to me too. Have you resolved it?

stevenyap commented 2 years ago

Hi, I have done some investigation into this issue and (possibly) determined the source of the problem.

First of all, the Hover type required by language-server-protocol is that the field range is undefined or Range (NOT null) as below:

export interface Hover {
    /**
     * The hover's content
     */
    contents: MarkupContent | MarkedString | MarkedString[];

    /**
     * An optional range inside the text document that is used to
     * visualize the hover, e.g. by changing the background color.
     */
    range?: Range;
}

Code above taken from here.

However, this package defined range as nullable: https://github.com/nwolverson/purescript-language-server/blob/4f8112ca189f432fd7e65482051213c5c8080a4c/src/LanguageServer/Protocol/Types.purs#L251-L252

This was fine all along until coc made a change last month (25 Aug 2022):

public async provideHover(
    document: TextDocument,
    position: Position,
    token: CancellationToken
  ): Promise<Hover[]> {
    let items = this.getProviders(document)
    let hovers: Hover[] = []
    let results = await Promise.allSettled(items.map(item => {
      return Promise.resolve(item.provider.provideHover(document, position, token)).then(hover => {
        if (!Hover.is(hover)) return // This causes our hover to fail!!!
        if (hovers.findIndex(o => equals(o.contents, hover.contents)) == -1) {
          hovers.push(hover)
        }
      })
    }))
    this.handleResults(results, 'provideHover')
    return hovers
  }

Code above taken from here

coc has changed to use vscode-languageserver-protocol's Hover.is() to check the Hover object (previously coc does not do any checks). The Hover.is() failed our returned Hover object because our range is null as opposed to undefined. Hence, the hover does not work now.

I am quite new to Purescript so I am not sure what would be a good solution here. One idea is to change the definition of Hover record type to use some sort of Undefined type for range so that it can give the correct undefined representation in JS.

nwolverson commented 2 years ago

@stevenyap thanks for the investigation, I will check if we're actually sending the wrong thing in that range field, and change accordingly. There's certainly no problem switching from Nullable, though I would be surprised if this is the only instance of this issue.

(Aside - this is why an effective bug report would be accompanied with LSP communication logs and couched in those terms - there is no world where I'm using vim to find out what's going on)

nwolverson commented 2 years ago

@k-jsmith01 can you test this, version 0.17.0 should fix this

stevenyap commented 2 years ago

@nwolverson I verify that the [show documentation] is working again. Thank you for the fix!

k-jsmith01 commented 2 years ago

@nwolverson Works for me as well now, thanks for the quick fix.

Thanks as well @stevenyap for thorough investigation!