natebosch / vim-lsc

A vim plugin for communicating with a language server
BSD 3-Clause "New" or "Revised" License
692 stars 79 forks source link

Not showing actions from eclipse-jdt-ls #88

Closed nemrod closed 4 years ago

nemrod commented 6 years ago

If I have e.g. a missing import and have the cursor on the word, I expect vim-lsc to show me the code action to import it. The language server should be providing it, seeing as how it works in LanguageClient-neovim (shows a big list of actions, including importing it from various packages and changing the spelling of the word). Instead I get "[lsc:log] No actions available".

natebosch commented 6 years ago

Hmm, are you able to capture some communication logs with a client that is working and with this client (looks like you might be able to add a flag to the server startup, I'm thinking -Dlog.protocol=true might do that)

I'm not sure what could cause this. Might be something in the client capabilities that make the server think the client can't handle code actions? Or perhaps there is some bug in the parameters this client is sending in the request... We always send empty diagnostics...

natebosch commented 6 years ago

could you please try the branch diagnostics-for-code-actions and see if it makes a difference for you?

nemrod commented 6 years ago

I tried the branch and actions now appear :+1: When applying one I get this error though: LSC Error!: No delegateCommandHandler for java.apply.workspaceEdit

natebosch commented 6 years ago

This looks like https://github.com/eclipse/eclipse.jdt.ls/issues/376

As far as I can tell this is a bug in the server - they are making assumptions about the client that are not specified in the protocol.

I'll have to give this some thought. LanguageClient-neovim chose to support this by hardcoding support for this behavior. I'm not sure I want to add custom code in the plugin to handle a broken server implementation so I'll see if there is some way I can give a hook to handling this with an extension outside the plugin.

2chilled commented 5 years ago

Hi, any progress on this? I want to switch from LanguageClient-neovim to vim-lsc because of better Scala support (through metals), but unfortunately I still have legacy Java projects to maintain :)

hrj commented 5 years ago

I'm not sure I want to add custom code in the plugin to handle a broken server implementation

:+1: for that. Adding custom logic for basic functionality will undermine the power of LSP. I wish more clients and servers took this stand, as it will reduce fragmentation in the long run.

pwntester commented 4 years ago

I have the same problem which is pretty bad since missing imports is one of the reasons why I use a LSP client in vi.

Have you considered hardcoding the eclise-lts behaviour as langugeClient is doing? or maybe add some way users can override default behaviour?

Also, I cant find diagnostics-for-code-actions branch, was it already merged?

Thanks

natebosch commented 4 years ago

Have you considered hardcoding the eclise-lts behaviour as langugeClient is doing?

I decided against that approach.

or maybe add some way users can override default behaviour?

I'd like to explore this further.

Also, I cant find diagnostics-for-code-actions branch, was it already merged?

Yes it was merged.

natebosch commented 4 years ago

@pwntester - can you switch to the branch in https://github.com/natebosch/vim-lsc/pull/225

Then in your config for the java language server add a 'response_hooks' that points 'textDocument/codeAction' at a function which will translate the weirdness in the server response.

For example:

let g:lsc_server_commands = {
    \ 'java': {
    \   'command': '...',
    \   'response_hooks': {
    \     'textDocument/codeAction': function('<SID>fixEdits'),
    \    }
    \  }
    \}

function! s:fixEdits(actions) abort
  return map(a:actions, function('<SID>fixEdit'))
endfunction

function! s:fixEdit(idx, maybeEdit) abort
  if !has_key(a:maybeEdit, 'command') ||
      \ a:maybeEdit.command !=# 'java.apply.workspaceEdit'
    return a:maybeEdit
  endif
  return {'edit': a:maybeEdit.arguments[0], 'title': a:maybeEdit.title}
endfunction
pwntester commented 4 years ago

Awesome, thanks, this looks like a flexive solution that may help with other server "weirdnesses"

I used the response_hooks branch and the configuration above but got:

[lsc:Error] Error dispatching message: 'Vim(if):E735: Can only compare Dictionary with Dictionary'

The a:actions received is:

[
   {
      'diagnostics':[

      ],
      'kind':'source.organizeImports',
      'title':'Organize imports',
      'command':{
         'arguments':[
            {
               'changes':{
                  'file:///Users/alvaro/Spring/SpringBoot/custom_endpoints/src/main/java/com/test/spring/songs/actuator/CustomNativeActuator.java':[
                     {
                        'range':{
                           'end':{
                              'character':69,
                              'line':6
                           },
                           'start':{
                              'character':74,
                              'line':3
                           }
                        },
                        'newText':'                                                                                                  import org.springframework.boot.actuate.endpoint.annotation.Selector;                                                                                                                                                                         import org.springframework.boot.actuate.endpoint.annotation.WriteOperation;                                                                                                                                                                   import org.springframework.stereotype.Component;'
                     }
                  ]
               }
            }
         ],
         'title':'Organize imports',
         'command':'java.apply.workspaceEdit'
      }
   },
   {
      'diagnostics':[

      ],
      'kind':'source.generate.toString',
      'title':'Generate toString()...',
      'command':{
         'arguments':[
            {
               'changes':{
                  'file:///Users/alvaro/Spring/SpringBoot/custom_endpoints/src/main/java/com/test/spring/songs/actuator/CustomNativeActuator.java':[
                     {
                        'range':{
                           'end':{
                              'character':5,
                              'line':34
                           },
                           'start':{
                              'character':5,
                              'line':34
                           }
                        },
                        'newText':'                                                                                                                                                                                                                                                                                                                                                                                                                                                                 @Override                                                                                                                                                                                                                                     public String toString() {                                                                                                                                                                                                                            return "CustomNativeActuator []";                                                                                                                                                                                                     }'
                     }
                  ]
               }
            }
         ],
         'title':'Generate toString()...',
         'command':'java.apply.workspaceEdit'
      }
   }
]

So each item passed to s:fixEdit looks like:

{   
    'diagnostics': [], 
    'kind': 'source.organizeImports', 
    'title': 'Organize imports', 
    'command': {
        'arguments': [ ... ], 
        'title': 'Organize imports', 
        'command': 'java.apply.workspaceEdit'
    }
}

I changed your fixEdit function to:

function! s:fixEdit(idx, maybeEdit) abort
    if !has_key(a:maybeEdit, 'command') || a:maybeEdit.command.command !=# 'java.apply.workspaceEdit'
        return a:maybeEdit
    endif
    return {'edit': a:maybeEdit.command.arguments[0], 'title': a:maybeEdit.command.title}
endfunction

and now it doesnt throw an error.

Note, that LSC is displaying the actions even if they have the java.apply.workspaceEdit but they are not applied if selected.

Also, the server is not sending the "add missing import" actions, which seems to be related with LSC not sending the right info (see: https://github.com/natebosch/vim-lsc/issues/223)

Thanks!

natebosch commented 4 years ago

Note, that LSC is displaying the actions even if they have the java.apply.workspaceEdit but they are not applied if selected.

Hmm, the goal was that by modeling them as something with an 'edit' key they would be handled correctly - that is what happened with my toy example (I don't have the server installed so I tried to mimic it, incorrectly I suppose).

In theory once we mutate the response like this we should fall into this case:

https://github.com/natebosch/vim-lsc/blob/730d901164497f17b15a42d531b346752cc10765/autoload/lsc/edit.vim#L26-L28

Choosing that action should then apply the edit directly. I'm not sure why that wouldn't be happening here.

Also, the server is not sending the "add missing import" actions, which seems to be related with LSC not sending the right info

Any chance you can capture some wire logs in the client where it is working? I wonder if the server is expecting more or different diagnostics than what we are sending.

Today we'll send diagnostics on the same line as the cursor:

https://github.com/natebosch/vim-lsc/blob/730d901164497f17b15a42d531b346752cc10765/autoload/lsc/edit.vim#L9-L10

That is only the diagnostics that start on the same line as the cursor, so if it is a multiline diagnostic it won't work to fetch the actions from any line but the first one.

If we figure out what other info triggers getting the missing import action to show up we can definitely find a way to add it to the request.

natebosch commented 4 years ago

The a:actions received is:

Did you sanitize the newText fields there, or did something strange happen? It looks like invalid json as copied here.

pwntester commented 4 years ago

Did you sanitize the newText fields there, or did something strange happen? It looks like invalid json as copied here.

Yep, I used an online prettifier to format the vim dictionary since it was a long one-line

Any chance you can capture some wire logs in the client where it is working? I wonder if the server is expecting more or different diagnostics than what we are sending.

This is the request languageClient-neovim is sending:

13:23:46 INFO writer-Some("java") src/rpcclient.rs:215 => Some("java") {"jsonrpc":"2.0","method":"textDocument/codeAction","params":{"context":{"diagnostics":[{"code":"16777218","message":"File cannot be resolved to a type","range":{"end":{"character":20,"line":19},"start":{"character":16,"line":19}},"severity":1,"source":"Java"}]},"range":{"end":{"character":18,"line":19},"start":{"character":18,"line":19}},"textDocument":{"uri":"file:///Users/alvaro/Spring/SpringBoot/testcases/custom_endpoints/src/main/java/com/test/spring/songs/actuator/CustomNativeActuator.java"}},"id":2}

And this is the server response (just the relevant action):

"title":"Import \u0027File\u0027 (java.io)","command":"java.apply.workspaceEdit","arguments":[{"changes":{"file:///Users/alvaro/Spring/SpringBoot/testcases/custom_endpoints/src/main/java/com/test/spring/songs/actuator/CustomNativeActuator.java":[{"range":{"start":{"line":0,"character":0},"end":{"line":2,"character":0}},"newText":"package com.test.spring.songs.actuator;\n\nimport java.io.File;\n\n"}]}}]

When using LSC, I can see that server sends the same diagnostic and LSC shows it:

{"jsonrpc":"2.0","method":"textDocument/publishDiagnostics","params":{"uri":"file:///Users/alvaro/Spring/SpringBoot/testcases/custom_endpoints/src/main/java/com/test/spring/songs/actuator/CustomNativeActuator.java","diagnostics":[{"range":{"start":{"line":19,"character":16},"end":{"line":19,"character":20}},"severity":1,"code":"16777218","source":"Java","message":"File cannot be resolved to a type"}]}}Content-Length: 36

However codeaction request is:

{"method": "textDocument/codeAction", "jsonrpc": "2.0", "id": 25, "params": {"context": {"diagnostics": []}, "range": {"end": {"character": 18, "line": 19}, "start": {"character": 17, "line": 19}}, "textDocument": {"uri": "file:///Users/alvaro/Fortify/SSR/Spring/SpringBoot/testcases/custom_endpoints/src/main/java/com/amitph/spring/songs/actuator/CustomNativeActuator.java"}}}

And server response does not include the "Import" action:

{"jsonrpc":"2.0","id":25,"result":[{"title":"Organize imports","kind":"source.organizeImports","diagnostics":[],"command":{"title":"Organize imports","command":"java.apply.workspaceEdit","arguments":[{"changes":{"file:///Users/alvaro/Fortify/SpringBoot/testcases/custom_endpoints/src/main/java/com/test/spring/songs/actuator/CustomNativeActuator.java":[{"range":{"start":{"line":3,"character":74},"end":{"line":6,"character":69}},"newText":"\nimport org.springframework.boot.actuate.endpoint.annotation.Selector;\nimport org.springframework.boot.actuate.endpoint.annotation.WriteOperation;\nimport org.springframework.stereotype.Component;"}]}}]}},{"title":"Generate toString()...","kind":"source.generate.toString","diagnostics":[],"command":{"title":"Generate toString()...","command":"java.apply.workspaceEdit","arguments":[{"changes":{"file:///Users/alvaro/Spring/SpringBoot/testcases/custom_endpoints/src/main/java/com/test/spring/songs/actuator/CustomNativeActuator.java":[{"range":{"start":{"line":34,"character":5},"end":{"line":34,"character":5}},"newText":"\n\n\t@Override\n\tpublic String toString() {\n\t\treturn \"CustomNativeActuator []\";\n\t}"}]}}]}}]}

Hope that helps!

Regarding the response_hook, I was expecting to return the same server response, but replacing java.apply.workspaceEdit with workspaceEdit. But it seems we need to return an internal LSC object with the edit key. Is that correct? If so, that would require knowing what LSC is expecting. Maybe just hooking into the server repsonse and being able to modify it before handling it to LSC as if it came from the server would be better since no knowledge about LSC would be required. (this is just an assumption :) )

Thanks again!

natebosch commented 4 years ago

I installed a recent version of the server and with your updates for fixEdit I was able to get the edits working. I tested by fetching code actions and choosing the "Generate toString()" action. If you are setting g:lsc_enable_apply_edit to v:false then the edit would end up silently getting dropped. Otherwise we should be able to apply it, I'm not sure why you wouldn't be seeing the changes.

natebosch commented 4 years ago

But it seems we need to return an internal LSC object with the edit key. Is that correct? If so, that would require knowing what LSC is expecting. Maybe just hooking into the server repsonse and being able to modify it before handling it to LSC as if it came from the server would be better since no knowledge about LSC would be required.

The fixEdit function (after your fix) should translate the edit to follow the LSP spec: https://microsoft.github.io//language-server-protocol/specifications/specification-3-14/#textDocument_codeAction

The interface CodeAction can have either a command or an edit. So we're translating from the non-standard server approach of using a "command" with an inline edit, to the properly spec supported approach of sending it as an "edit".

natebosch commented 4 years ago

I had a bug in the diagnostics I was sending when fetching code actions. Fixed by https://github.com/natebosch/vim-lsc/commit/9f6c06e553822888fde80e9427fb8efa050497cb

I think the remaining issue to work out is why the edits aren't working even after normalizing them to follow the LSP spec.

Since the feature seems to be working to translate the response I'll get that documented and merged.

natebosch commented 4 years ago

I merged #225 and so the workaround should be available on the master branch.

@pwntester let me know if it's still not applying the edit for you when you choose an action.

pwntester commented 4 years ago

Awesome, it works for me now. Thanks!

natebosch commented 4 years ago

Glad it's working 😄

Consider adding a note about your working config to https://github.com/natebosch/vim-lsc/wiki/Language-Servers so that others can copy it. Or, you might even consider publishing a plugin similar to https://github.com/natebosch/vim-lsc-dart that configures the Java server in an ftplugin/java.vim and handles this.

pwntester commented 4 years ago

Will do, Im still setting the whole thing up, but as soon as I have it working as I want I will send a PR for the wiki