Renaming a module adds additional new lines
Opened this issue · 15 comments
Before:
After:
Looks like the refactor code adds new lines, to each change.
@karthiknadig I'm unable to reproduce in Neovim with coc.nvim. See gif with latest release under coc.nvim:
This is what i get as a response message:
[Trace - 12:24:36 PM] Received response 'textDocument/rename - (40)' in 15ms.
Result: {
"documentChanges": [
{
"textDocument": {
"uri": "file:///c:/GIT/repro/renaming/myscript.py",
"version": 0
},
"edits": [
{
"range": {
"start": {
"line": 0,
"character": 24
},
"end": {
"line": 1,
"character": 0
}
},
"newText": "new_somemodule\r\n\r"
},
{
"range": {
"start": {
"line": 2,
"character": 10
},
"end": {
"line": 13,
"character": 0
}
},
"newText": "\r\n new_somemodule.bar()\r\n new_somemodule.bar()\r\n new_somemodule.bar()\r\n new_somemodule.bar()\r\n new_somemodule.bar()\r\n\r\n\r\n\r\n\r\n\r\n\r"
},
{
"range": {
"start": {
"line": 14,
"character": 26
},
"end": {
"line": 15,
"character": 9
}
},
"newText": "\r\n run()"
}
]
},
{
"kind": "rename",
"oldUri": "file:///c:/GIT/repro/renaming/somepackage/somemodule.py",
"newUri": "file:///c:/GIT/repro/renaming/somepackage/new_somemodule.py",
"options": {
"overwrite": true,
"ignoreIfExists": true
}
}
]
}
As you can see in the response there is additional \r\n
@pappasam I am seeing more reports of this. microsoft/vscode-python#17024
https://www.reddit.com/r/vscode/comments/nc12yo/rename_symbol_bug/
@karthiknadig I'll try adding the test case in a bit to see if I can narrow it down.
tl;dr I cannot reproduce this issue with the example provided.
@karthiknadig I've added some automated tests with your example. They pass on both Ubuntu and Windows containers with Github actions. Here's the relevant commit: f2c7a5e
If this is indeed an issue with the latest version of jedi-language-server and vscode, it's possible that this has to do with the existence of carriage returns within a file. If that's the case, this is probably a lower-level pygls issue. Either way, given the fact that I'm on a POSIX-like system, any further testing will be somewhat challenging on my end.
I just found a fix (for me anyway) for this issue. Perhaps it could also shed light on what could be happening behind the scenes. I usually use Linux, but I sometimes develop on Windows and this issue seemed to only happen on Windows in my case. The issue seems to be with the carriage return character.
If you switch the line ends in your file for LF (\n) instead of CRLF (\r), the issue stops happening. To change default character used when pressing enter in VSCode, you can follow this guide.
Hope this helps, cause this definitely plagued me.
@karthiknadig is this issue still present with the latest release? I've tried testing this out in several ways on my machine by running files through unix2dos, which adds carriage returns locally, and I've so-far been unable to reproduce on my end.
I will investigate this one. If there is anything, i will make a PR. But it might take some time for me to get to this.
@pappasam I investigated the issue, I think it is coming from lsp_text_edits:
new_code = 'from somepackage import somemodule2\r\n\r\n\r\ndef run():\r\n somemodule2.bar()\r\n somemodule2.bar()\r\n somemodule2.bar()\r\n somemodule2.bar()\r\n somemodule2.bar()\r\n\r\n\r\nif __name__ == "__main__":\r\n run()\r\n'
old_code = 'from somepackage import somemodule\n\n\ndef run():\n somemodule.bar()\n somemodule.bar()\n somemodule.bar()\n somemodule.bar()\n somemodule.bar()\n\n\nif __name__ == "__main__":\n run()\n'
The \r s in the new code causes edits to look like this:
which is forcing the extra line. The new code line ending should probably be cleaned up to look like old code line endings.
Yep, that's the function that I've been examining for a long time and haven't been able to figure out exactly why this is happening. As we've discussed earlier, this seems like a pretty deep, VSCode Windows-specific issue.
If I'm reading your example correctly, the original version of the code does not actually have carriage returns and jedi-language-server is somehow inserting carriage returns? If that's the case, it seems like the base Python dependencies that I'm using are relying on some heuristic to determine that they should be returning carriage returns on the Windows platform.
@Quoding seems to have a technique that fixes this issue in VSCode: #159 (comment)
@karthiknadig Two questions for you:
- Is there a way for us, as part of the VSCode Python configuration, to configure the option referenced by @Quoding by default for Python files?
- Would doing this be a good idea?
@pappasam I don't think defining such a setting would be a good idea here. The issue actually occurs because, how both of these contents are acquired. pygls reads the document using open without specifying line ending, this causes python to read it in universal mode, where all \r\n are replaced with \n. From what I could tell from initial reading through parso and jedi is that parso reads the file in binary mode. Line endings are not universalized and are retained.
In lsp_text_edits the old_code is coming from pygls (with universalized line endings) and new_code is coming from jedi (via parso with line-endings as is).
I think we should try and see if we can update pygls to read with line endings retained, and that could potentially fix this issue. I'll try this out and let you know how this goes.
@pappasam I verified that updating this line pygls with newline='' addresses the problem (https://github.com/openlawlibrary/pygls/blob/bbf671f509b0d499a006daeeae8cdb78e3419fe5/pygls/workspace.py#L275). However, the returned text edits are different (by platform and python version), but the end result of applying the text edits seems to be the same. This means the test might get a bit messy for this, we may have to apply the text edits and then verify.
I am still experiencing this issue, whereby renaming a symbol in one function inserts new lines throughout the module.
Me too.
I am still experiencing this issue, whereby renaming a symbol in one function inserts new lines throughout the module.
I'm not 100% sure it's relevant but I've come across a line endings bug in Pygls where it wasn't taking into consideration \r when calculating positions and ranges, which are the very things used when applying automated edits to a document. So I certainly think there could be a connection here.
I have a PR ready openlawlibrary/pygls#304 I was hoping for a bit more feedback before merging. Is anybody here in a position to test to see if it fixes the issue?