Skip to content

[lsp] - rename symbol keeping the same name #42573

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

Closed
ericdallo opened this issue Jul 4, 2020 · 6 comments
Closed

[lsp] - rename symbol keeping the same name #42573

ericdallo opened this issue Jul 4, 2020 · 6 comments
Labels
devexp-lsp Issues with analysis server's support of Language Server Protocol devexp-server Issues related to some aspect of the analysis server legacy-area-analyzer Use area-devexp instead.

Comments

@ericdallo
Copy link
Contributor

It seems that dart_analysis_server crashes when receive a textDocument/rename with the same symbol name on the newName field.

I could not reproduce it on VSCode because vscode seems to not send the textDocument/rename when the symbol name is the name, but on Emacs using lsp-dart, we send the textDocument/rename and after the server returns null on the response(Which is ok following the LSP spec), the dart_analysis_server process crash.

Some log from the lsp-dart client:

[Trace - 04:36:07 PM] Sending request 'textDocument/prepareRename - (183)'.
Params: {
  "textDocument": {
    "uri": "file:///home/greg/dev/flutter_sample/lib/screens/dashboard/components/rightscreen.dart"
  },
  "position": {
    "line": 52,
    "character": 21
  }
}

[Trace - 04:36:07 PM] Received response 'textDocument/prepareRename - (183)' in 8ms.
Result: {
  "placeholder": "_goToNextPage",
  "range": {
    "end": {
      "character": 28,
      "line": 52
    },
    "start": {
      "character": 15,
      "line": 52
    }
  }
}

[Trace - 04:36:12 PM] Sending request 'textDocument/rename - (184)'.
Params: {
  "textDocument": {
    "uri": "file:///home/greg/dev/flutter_sample/lib/screens/dashboard/components/rightscreen.dart"
  },
  "position": {
    "line": 52,
    "character": 21
  },
  "newName": "_goToNextPage"
}

[Trace - 04:36:12 PM] Received response 'textDocument/rename - (184)' in 7ms.
Result: null

Tested on flutter channel stable, dart 2.8.1 and flutter dev channel, Dart 2.9.0 (build 2.9.0-19.0.dev 7e72c9ae7e)

@ericdallo
Copy link
Contributor Author

emacs-lsp/lsp-dart#41

@bwilkerson
Copy link
Member

@scheglov @DanTup

@bwilkerson bwilkerson added devexp-lsp Issues with analysis server's support of Language Server Protocol devexp-server Issues related to some aspect of the analysis server legacy-area-analyzer Use area-devexp instead. labels Jul 4, 2020
@DanTup
Copy link
Collaborator

DanTup commented Jul 6, 2020

I get a different response from the server than in the log above (an error instead of null):

Content-Length: 114
Content-Type: application/vscode-jsonrpc; charset=utf-8
{"id":7,"jsonrpc":"2.0","error":{"code":-32010,"message":"The new name must be different than the current name."}}

However, the server still stopped responding (even in a test). It turned out to be a silly mistake - I had reused an error code, and it happened to the the one raised then the client/server are out-of-sync which results in a server shutdown 😞

I've got a fix here: https://dart-review.googlesource.com/c/sdk/+/153341/

@ericdallo
Copy link
Contributor Author

I see, thank you for the quick fix @DanTup, I'll keep an eye on that :)

dart-bot pushed a commit that referenced this issue Jul 6, 2020
Bug: #42573
Change-Id: I615563ed636cf48b4fe84e7588f49bb7137a4f53
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/153341
Reviewed-by: Brian Wilkerson <[email protected]>
Commit-Queue: Danny Tuppeny <[email protected]>
@DanTup
Copy link
Collaborator

DanTup commented Aug 3, 2020

This was fixed by 34ad5a5.

(@bwilkerson @ericdallo I don't have powers to close this, so someone will have to do it for me. Thanks!)

@ericdallo
Copy link
Contributor Author

Thank you very much @DanTup :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devexp-lsp Issues with analysis server's support of Language Server Protocol devexp-server Issues related to some aspect of the analysis server legacy-area-analyzer Use area-devexp instead.
Projects
None yet
Development

No branches or pull requests

3 participants