Skip to content

Commit

Permalink
Add support codeAction/resolve requests
Browse files Browse the repository at this point in the history
So...
We advertise that we can handle CodeActions that are either missing an
edit or a command (or both).
We also advertise that we will preserve server `data` that we receive in
a CodeAction, when we resolve the said fixit. This allows more servers
to take advantage of the lazy code actions.

The server chooses whether it supports either of the two. If it does, it
advertises itself as a "code action resolve provider".

For servers that are code action resolve providers:
- If either edit or command is missing, we need to try resolving the
  code action via codeAction/resolve.
  - Unless we actually got a LSP Command, in which case
    codeAction/resove is skipped.
- After resolving it that way we have a CodeAction in one of these forms:
  - A LSP Command
  - A LSP CodeAction with only an edit.
  - A LSP CodeAction with only a command.
  - A LSP CodeAction with both an edit and a command.
- Edits are WorkspaceEdits and can easily be converted into ycmd FixIts.
- Commands are to be executed, yielding ApplyEdits. A single ApplyEdit
  can be converted into a ycmd FixIt.

For servers that are not code action resolve providers, the steps are
the same, but we skip the codeAction/resolve route.

One thing missing is properly defined handling of fixit resolutions that
yield multiple fixits. That can happen in a few ways:

- On /resolve_fixit, by a LSP command yielding multiple ApplyEdits.
- When eagerly resolving a fixit, again by a LSP command yielding
  multiple ApplyEdits.
- Even if all commands always yield a single ApplyEdit, if a CodeAction
  has both an edit and a command, that's again two fixits.

The first two above don't seem to be used by any server ever. The LSP
specs nudges servers away from doing that, but no promises.
We are still not supporting any scenario where resolving a single fixit
results in more than one fixit.

Another scenario that does not seem to happen:
- The server is a code action resove provider.
- The received CodeAction has neither an edit nor a command.
- After resolving, we get only a command.
- We then need to execute the command and collect ApplyEdits.

In practice, such code actions resolve to a CodeAction containing an edit.

As for the ycmd<->client side of things... it's a bit ugly on the ycmd
side, but we're completely preserving the API, so clients do not need to
change a thing.

Previously, clients got `{ 'fixits': [ { 'command': LSPCommand ... } ]
}` for unresolved fixits.  However, we have not given any guarantees
about the value of `'command'`.  We have only said that it should be
returned to ycmd for the purposes of `/resolve_fixit`.  With this pull
request, we need to pass the entire CodeAction, but we're still putting
it in the `command` key.
  • Loading branch information
bstaletic committed Aug 17, 2024
1 parent 0dd5feb commit 00b01be
Show file tree
Hide file tree
Showing 7 changed files with 258 additions and 184 deletions.
17 changes: 7 additions & 10 deletions ycmd/completers/java/java_completer.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import threading

from ycmd import responses, utils
from ycmd.completers.language_server import language_server_protocol as lsp
from ycmd.completers.language_server import language_server_completer
from ycmd.utils import LOGGER

Expand Down Expand Up @@ -624,15 +623,13 @@ def GetDoc( self, request_data ):


def OrganizeImports( self, request_data ):
fixit = {
'resolve': True,
'command': {
'title': 'Organize Imports',
'command': 'java.edit.organizeImports',
'arguments': [ lsp.FilePathToUri( request_data[ 'filepath' ] ) ]
}
}
return self._ResolveFixit( request_data, fixit )
fixits = super().GetCodeActions( request_data )[ 'fixits' ]
for fixit in fixits:
if fixit[ 'command' ][ 'kind' ] == 'source.organizeImports':
return self._ResolveFixit( request_data, fixit )
# We should never get here. With codeAction/resolve support,
# JDT always sends the organizeImports code action.
raise RuntimeError( 'OrganizeImports not available.' )


def CodeActionCommandToFixIt( self, request_data, command ):
Expand Down
85 changes: 58 additions & 27 deletions ycmd/completers/language_server/language_server_completer.py
Original file line number Diff line number Diff line change
Expand Up @@ -2850,7 +2850,6 @@ def GetCodeActions( self, request_data ):
cursor_range_ls,
matched_diagnostics ),
REQUEST_TIMEOUT_COMMAND )

return self.CodeActionResponseToFixIts( request_data,
code_actions[ 'result' ] )

Expand All @@ -2861,28 +2860,22 @@ def CodeActionResponseToFixIts( self, request_data, code_actions ):

fixits = []
for code_action in code_actions:
if 'edit' in code_action:
# TODO: Start supporting a mix of WorkspaceEdits and Commands
# once there's a need for such
assert 'command' not in code_action

# This is a WorkspaceEdit literal
fixits.append( self.CodeActionLiteralToFixIt( request_data,
code_action ) )
continue

# Either a CodeAction or a Command
assert 'command' in code_action

action_command = code_action[ 'command' ]
if isinstance( action_command, dict ):
# CodeAction with a 'command' rather than 'edit'
fixits.append( self.CodeActionCommandToFixIt( request_data,
code_action ) )
capabilities = self._server_capabilities[ 'codeActionProvider' ]
if ( ( isinstance( capabilities, dict ) and
capabilities.get( 'resolveProvider' ) ) or
'command' in code_action ):
# If server is a code action resolve provider, either we are obligated
# to resolve, or we have a command in the code action response.
# If server does not want us to resolve, but sends a command anyway,
# we still need to lazily execute that command.
fixits.append( responses.UnresolvedFixIt( code_action,
code_action[ 'title' ],
code_action.get( 'kind' ) ) )
continue
# No resoving here - just a simple code action literal.
fixits.append( self.CodeActionLiteralToFixIt( request_data,
code_action ) )

# It is a Command
fixits.append( self.CommandToFixIt( request_data, code_action ) )

# Show a list of actions to the user to select which one to apply.
# This is (probably) a more common workflow for "code action".
Expand Down Expand Up @@ -2986,10 +2979,44 @@ def Format( self, request_data ):


def _ResolveFixit( self, request_data, fixit ):
if not fixit[ 'resolve' ]:
return { 'fixits': [ fixit ] }
code_action = fixit[ 'command' ]
capabilities = self._server_capabilities[ 'codeActionProvider' ]
if ( isinstance( capabilities, dict ) and
capabilities.get( 'resolveProvider' ) ):
# Resolve through codeAction/resolve request, before resolving commands.
# If the server is an asshole, it might be a code action resolve
# provider, but send a LSP Command instead. We can not resolve those with
# codeAction/resolve!
if ( 'command' not in code_action or
isinstance( code_action[ 'command' ], str ) ):
request_id = self.GetConnection().NextRequestId()
msg = lsp.CodeActionResolve( request_id, code_action )
code_action = self.GetConnection().GetResponse(
request_id,
msg,
REQUEST_TIMEOUT_COMMAND )[ 'result' ]

unresolved_fixit = fixit[ 'command' ]
result = []
if 'edit' in code_action:
result.append( self.CodeActionLiteralToFixIt( request_data,
code_action ) )

if 'command' in code_action:
assert not result, 'Code actions with edit and command is not supported.'
if isinstance( code_action[ 'command' ], str ):
unresolved_command_fixit = self.CommandToFixIt( request_data,
code_action )
else:
unresolved_command_fixit = self.CodeActionCommandToFixIt( request_data,
code_action )
result.append( self._ResolveFixitCommand( request_data,
unresolved_command_fixit ) )

return responses.BuildFixItResponse( result )


def _ResolveFixitCommand( self, request_data, fixit ):
unresolved_fixit = fixit.command
collector = EditCollector()
with self.GetConnection().CollectApplyEdits( collector ):
self.GetCommandResponse(
Expand All @@ -3001,19 +3028,23 @@ def _ResolveFixit( self, request_data, fixit ):
response = collector.requests
assert len( response ) < 2
if not response:
return responses.BuildFixItResponse( [ responses.FixIt(
return responses.FixIt(
responses.Location( request_data[ 'line_num' ],
request_data[ 'column_num' ],
request_data[ 'filepath' ] ),
[] ) ] )
[] )
fixit = WorkspaceEditToFixIt(
request_data,
response[ 0 ][ 'edit' ],
unresolved_fixit[ 'title' ] )
return responses.BuildFixItResponse( [ fixit ] )
return fixit


def ResolveFixit( self, request_data ):
fixit = request_data[ 'fixit' ]
if 'command' not in fixit:
# Somebody has sent us an already resolved fixit.
return { 'fixits': [ fixit ] }
return self._ResolveFixit( request_data, request_data[ 'fixit' ] )


Expand Down
11 changes: 10 additions & 1 deletion ycmd/completers/language_server/language_server_protocol.py
Original file line number Diff line number Diff line change
Expand Up @@ -326,8 +326,13 @@ def Initialize( request_id,
'refactor.inline',
'refactor.rewrite',
'source',
'source.organizeImports' ]
'source.organizeImports',
'source.fixAll' ]
}
},
'dataSupport': True,
'resolveSupport': {
'properties': [ 'edit', 'command' ]
}
},
'completion': {
Expand Down Expand Up @@ -580,6 +585,10 @@ def CodeAction( request_id, request_data, best_match_range, diagnostics ):
} )


def CodeActionResolve( request_id, code_action ):
return BuildRequest( request_id, 'codeAction/resolve', code_action )


def Rename( request_id, request_data, new_name ):
return BuildRequest( request_id, 'textDocument/rename', {
'textDocument': TextDocumentIdentifier( request_data ),
Expand Down
3 changes: 2 additions & 1 deletion ycmd/tests/clangd/subcommands_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,8 @@ def FixIt_Check_cpp11_DelAdd( results ):
has_entries( {
'text': 'Move function body to declaration',
'resolve': True,
'command': has_entries( { 'command': 'clangd.applyTweak' } )
'command': has_entries( { 'command': has_entries( {
'command': 'clangd.applyTweak' } ) } )
} ),
)
} ) )
Expand Down
29 changes: 21 additions & 8 deletions ycmd/tests/go/subcommands_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ def CombineRequest( request, data ):
# We also ignore errors here, but then we check the response code
# ourself. This is to allow testing of requests returning errors.
response = app.post_json(
'/run_completer_command',
test.get( 'route', '/run_completer_command' ),
CombineRequest( test[ 'request' ], {
'completer_target': 'filetype_default',
'contents': contents,
Expand All @@ -91,8 +91,14 @@ def CombineRequest( request, data ):
return response.json


def RunFixItTest( app, description, filepath, line, col, fixits_for_line ):
RunTest( app, {
def RunFixItTest( app,
description,
filepath,
line,
col,
fixits_for_line,
chosen_fixit = None ):
test = {
'description': description,
'request': {
'command': 'FixIt',
Expand All @@ -104,7 +110,17 @@ def RunFixItTest( app, description, filepath, line, col, fixits_for_line ):
'response': requests.codes.ok,
'data': fixits_for_line,
}
} )
}
if chosen_fixit is not None:
test_no_expect = test.copy()
test_no_expect.pop( 'expect' )
response = RunTest( app, test_no_expect )
request = test[ 'request' ]
request.update( {
'fixit': response[ 'fixits' ][ chosen_fixit ]
} )
test[ 'route' ] = '/resolve_fixit'
RunTest( app, test )


def RunHierarchyTest( app, kind, direction, location, expected, code ):
Expand Down Expand Up @@ -445,9 +461,6 @@ def test_Subcommands_FixIt_NullResponse( self, app ):
filepath, 1, 1, has_entry( 'fixits', empty() ) )


@ExpectedFailure(
'Gopls bug. See https://github.com/golang/go/issues/68904',
matches_regexp( 'Browse free symbols' ) )
@SharedYcmd
def test_Subcommands_FixIt_Simple( self, app ):
filepath = PathToTestFile( 'fixit.go' )
Expand All @@ -464,7 +477,7 @@ def test_Subcommands_FixIt_Simple( self, app ):
} ),
)
} )
RunFixItTest( app, 'Only one fixit returned', filepath, 1, 1, fixit )
RunFixItTest( app, 'Only one fixit returned', filepath, 1, 1, fixit, 0 )


@SharedYcmd
Expand Down
Loading

0 comments on commit 00b01be

Please sign in to comment.