New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

An operation for refactorings #61

Open
mickaelistria opened this Issue Aug 29, 2016 · 14 comments

Comments

Projects
None yet
4 participants
@mickaelistria

mickaelistria commented Aug 29, 2016

Out of the rename operation, I couldn't find an operation for refactorings. I would expect something like

path: /refactorings
params: TextDocumentIdentifier, Range
result: Refactoring[] with Refactoring={String title, WorkspaceEdit[]}

is there already an operation I missed, which is meant to support that?

@dbaeumer

This comment has been minimized.

Show comment
Hide comment
@dbaeumer

dbaeumer Sep 19, 2016

Member

@mickaelistria no there isn't. We started a discussion about how a refactoring protocol could look like but we haven't found a good way to spec that yet since this is very dependent on the programming language you are using.

Member

dbaeumer commented Sep 19, 2016

@mickaelistria no there isn't. We started a discussion about how a refactoring protocol could look like but we haven't found a good way to spec that yet since this is very dependent on the programming language you are using.

@dbaeumer dbaeumer closed this Sep 19, 2016

@dbaeumer dbaeumer reopened this Sep 19, 2016

@dbaeumer

This comment has been minimized.

Show comment
Hide comment
@dbaeumer

dbaeumer Sep 19, 2016

Member

Sorry didn't want to close the issue.

Member

dbaeumer commented Sep 19, 2016

Sorry didn't want to close the issue.

@mickaelistria

This comment has been minimized.

Show comment
Hide comment
@mickaelistria

mickaelistria Sep 19, 2016

Thanks for your answer. Can you please add some details about some corner-cases you've identified for whatever language or refactoring hat wouldn't fit the suggestion of a refactoring operation I made above?

mickaelistria commented Sep 19, 2016

Thanks for your answer. Can you please add some details about some corner-cases you've identified for whatever language or refactoring hat wouldn't fit the suggestion of a refactoring operation I made above?

@dbaeumer

This comment has been minimized.

Show comment
Hide comment
@dbaeumer

dbaeumer Oct 31, 2016

Member

The problem we see so far are:

  • parameter capturing. For rename we made this trivial by only supplying the new name. However Java for example allows on renaming a type to rename variable with that typeName in its identifier as well.
  • communication on sematic shift warnings. Refactoring should be semantic preserving. But sometimes the semantic shift is wanted and we need to let the user confirm this.
  • how do we handle undos Should the editor simply revert the textual changes or do we need to involve the server as well.
Member

dbaeumer commented Oct 31, 2016

The problem we see so far are:

  • parameter capturing. For rename we made this trivial by only supplying the new name. However Java for example allows on renaming a type to rename variable with that typeName in its identifier as well.
  • communication on sematic shift warnings. Refactoring should be semantic preserving. But sometimes the semantic shift is wanted and we need to let the user confirm this.
  • how do we handle undos Should the editor simply revert the textual changes or do we need to involve the server as well.
@mickaelistria

This comment has been minimized.

Show comment
Hide comment
@mickaelistria

mickaelistria Oct 31, 2016

parameter capturing. For rename we made this trivial by only supplying the new name. However Java for example allows on renaming a type to rename variable with that typeName in its identifier as well.

I was thinking about rendering parameter capturing with "edits" like the {{xxx}} in completion, assuming multiple {{xxx}} means that all occurences should be renamed simultaneously. xxx would be generated as best according to the context.
Depending on the tool, those could be linked edits (like Eclipse rename) or be presented as a list of values to refine before applying the edit.

communication on sematic shift warnings. Refactoring should be semantic preserving. But sometimes the semantic shift is wanted and we need to let the user confirm this.

Ok. If this is the only case, then we could have a flag and message in the refactoring response. Tools can decide whether to honor it and ask for confirmation before applying change.

how do we handle undos Should the editor simply revert the textual changes or do we need to involve the server as well.

IMO, the undo is something I'd expect to be provided by the tool. The protocol doesn't have an undo for rename AFAIK, so I don't see why other refactorings should be treated differently.

mickaelistria commented Oct 31, 2016

parameter capturing. For rename we made this trivial by only supplying the new name. However Java for example allows on renaming a type to rename variable with that typeName in its identifier as well.

I was thinking about rendering parameter capturing with "edits" like the {{xxx}} in completion, assuming multiple {{xxx}} means that all occurences should be renamed simultaneously. xxx would be generated as best according to the context.
Depending on the tool, those could be linked edits (like Eclipse rename) or be presented as a list of values to refine before applying the edit.

communication on sematic shift warnings. Refactoring should be semantic preserving. But sometimes the semantic shift is wanted and we need to let the user confirm this.

Ok. If this is the only case, then we could have a flag and message in the refactoring response. Tools can decide whether to honor it and ask for confirmation before applying change.

how do we handle undos Should the editor simply revert the textual changes or do we need to involve the server as well.

IMO, the undo is something I'd expect to be provided by the tool. The protocol doesn't have an undo for rename AFAIK, so I don't see why other refactorings should be treated differently.

@dbaeumer

This comment has been minimized.

Show comment
Hide comment
@dbaeumer

dbaeumer Nov 1, 2016

Member

With parameter capturing I was referring to capturing input parameter for the refactoring. For rename for example rename in comments and strings. How would the server be able to describe these input parameters. And they might be different from languages if for example the language wouldn't support comments.

Member

dbaeumer commented Nov 1, 2016

With parameter capturing I was referring to capturing input parameter for the refactoring. For rename for example rename in comments and strings. How would the server be able to describe these input parameters. And they might be different from languages if for example the language wouldn't support comments.

@mickaelistria

This comment has been minimized.

Show comment
Hide comment
@mickaelistria

mickaelistria Nov 1, 2016

If for example we use a convention of TextEdits with `{{param}}`` value for example, then the parameters are passed by server to client and it's up to clients to deal with those parameters.

mickaelistria commented Nov 1, 2016

If for example we use a convention of TextEdits with `{{param}}`` value for example, then the parameters are passed by server to client and it's up to clients to deal with those parameters.

@alanz

This comment has been minimized.

Show comment
Hide comment
@alanz

alanz Nov 1, 2016

To make it concrete, some of the refactorings for haskell require the following parameters to be specified by the user in the client and passed to the server as input to the refactoring

rename FILE NEWNAME line col
demote FILE line col
dupdef FILE NEWNAME line col
iftocase FILE startline startcol endline endcol

So, the renaming maps onto the spec, but for the others some need a Position, some need a Range, and in addition some need a new name.

But this mapping of required parameters and command to return to the server needs to be provided as a capability at server start up.

alanz commented Nov 1, 2016

To make it concrete, some of the refactorings for haskell require the following parameters to be specified by the user in the client and passed to the server as input to the refactoring

rename FILE NEWNAME line col
demote FILE line col
dupdef FILE NEWNAME line col
iftocase FILE startline startcol endline endcol

So, the renaming maps onto the spec, but for the others some need a Position, some need a Range, and in addition some need a new name.

But this mapping of required parameters and command to return to the server needs to be provided as a capability at server start up.

@dbaeumer

This comment has been minimized.

Show comment
Hide comment
@dbaeumer

dbaeumer Dec 29, 2016

Member

@mickaelistria does the new executeCommand and applyEdit pair help you here?

Member

dbaeumer commented Dec 29, 2016

@mickaelistria does the new executeCommand and applyEdit pair help you here?

@mickaelistria

This comment has been minimized.

Show comment
Hide comment
@mickaelistria

mickaelistria Jan 10, 2017

This seems to do most of the work properly, I just have a few questions (that may be worth answering in the spec):
When the command is executed, assuming the command is a refactoring, is the state of the files on server-side modified?
ie, If client ignores the applyEdit notification, has anything changed on server side? ie Does the command only compute the necessary change or does it apply and compute it?

mickaelistria commented Jan 10, 2017

This seems to do most of the work properly, I just have a few questions (that may be worth answering in the spec):
When the command is executed, assuming the command is a refactoring, is the state of the files on server-side modified?
ie, If client ignores the applyEdit notification, has anything changed on server side? ie Does the command only compute the necessary change or does it apply and compute it?

@dbaeumer

This comment has been minimized.

Show comment
Hide comment
@dbaeumer

dbaeumer Jan 11, 2017

Member

@mickaelistria good point. The current behavior on the VS Code client side is as follows:

  • for all resources manipulated via a workspace edit we open them in an internal buffer.
  • this fires an open event which will be forwarded to the server
  • the client changes the file
  • this triggers a change event sent to the server
  • the client closes the file which again sends and event to the server

So the bottom line is that the server content should be updated without that the server replays the edits. If the server directly manipulates the files on disk (which he is allowed to do for files which are not open) then the server is responsible to update its internal buffers as well.

Member

dbaeumer commented Jan 11, 2017

@mickaelistria good point. The current behavior on the VS Code client side is as follows:

  • for all resources manipulated via a workspace edit we open them in an internal buffer.
  • this fires an open event which will be forwarded to the server
  • the client changes the file
  • this triggers a change event sent to the server
  • the client closes the file which again sends and event to the server

So the bottom line is that the server content should be updated without that the server replays the edits. If the server directly manipulates the files on disk (which he is allowed to do for files which are not open) then the server is responsible to update its internal buffers as well.

@mickaelistria

This comment has been minimized.

Show comment
Hide comment
@mickaelistria

mickaelistria Mar 7, 2017

I have the impression that the command framework is a bit overkill for some refactoring or codelens. In most case I've seen, those codelens and commands actually give a WorkspaceEdit as argument and expect the tool to apply it through a command. IMHO, everything that return a Command could also return a WorkspaceEdit and expect the tool to apply it - rather than assuming an "applyEdit"-like command is defined for each tool and language server.

mickaelistria commented Mar 7, 2017

I have the impression that the command framework is a bit overkill for some refactoring or codelens. In most case I've seen, those codelens and commands actually give a WorkspaceEdit as argument and expect the tool to apply it through a command. IMHO, everything that return a Command could also return a WorkspaceEdit and expect the tool to apply it - rather than assuming an "applyEdit"-like command is defined for each tool and language server.

@dbaeumer

This comment has been minimized.

Show comment
Hide comment
@dbaeumer

dbaeumer Apr 11, 2017

Member

What if the computation of the workspace edit is expensive and for example code actions return more than one possible action. The having this resolve step does make sense. However for a single thing it might be more convenient to include the workspace edit in the command.

Member

dbaeumer commented Apr 11, 2017

What if the computation of the workspace edit is expensive and for example code actions return more than one possible action. The having this resolve step does make sense. However for a single thing it might be more convenient to include the workspace edit in the command.

@kristijanhusak

This comment has been minimized.

Show comment
Hide comment
@kristijanhusak

kristijanhusak May 25, 2018

Any news on this?

kristijanhusak commented May 25, 2018

Any news on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment