Skip to content
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

Rename local variable #2457

Closed
smackesey opened this issue Mar 8, 2022 · 9 comments
Closed

Rename local variable #2457

smackesey opened this issue Mar 8, 2022 · 9 comments
Assignees
Labels
fixed in next version (main) A fix has been implemented and will appear in an upcoming version waiting for user response Requires more information from user

Comments

@smackesey
Copy link

smackesey commented Mar 8, 2022

Not sure if this is a bug or purposefully unsupported feature, but attempting to rename a local variable in LSP mode appears to be a no-op:

def myfunc(foo):  # Renaming does nothing
    bar = foo  # Renaming bar does nothing
    return bar

Running pyright 1.1.227 in LSP mode in Neovim.

@erictraut
Copy link
Contributor

The code that you pasted above has a syntax error because there's an extra space before bar = foo. If I fix that error, I'm able to rename the local variable without a problem, at least in VS Code.

@smackesey
Copy link
Author

Sorry for that syntax error.

On further experimentation I find that if I create a small example project with the above code, rename works fine, but if I try to do the same rename in the context of my large project, things get wonky:

  • Sometimes the rename works but only after a long time
  • After one rename works sometimes subsequent ones don't

Haven't systematically explored this, but my guess is that pyright is not following a simple optimized path for local variable renames, and instead at least part of the pathway is identical to what's used for project-wide renames (which can be very slow). This makes local var renames slow on large projects. Is that correct?

@erictraut
Copy link
Contributor

Pyright follows an optimized path for local variables like bar in your example. Renames should be almost immediate in that case. For symbols that have visibility outside of the function, such as foo (since it's potentially a keyword argument), pyright needs to do partial analysis of every file in your project. That can take a long time the first time you perform a rename.

The "subsequent ones don't work" is probably explained by the fact that you're using neovim which doesn't support asynchronous (file-based) cancellation, so all LSP commands must run to completion. If the first one takes a long time, you'll need to wait for it to finish.

@smackesey
Copy link
Author

smackesey commented Mar 8, 2022

For symbols that have visibility outside of the function, such as foo (since it's potentially a keyword argument), pyright needs to do partial analysis of every file in your project.

Hmm I just tried renaming foo in this new snippet and there is the same issue (takes forever):

def myfunc():
    foo = 1
    print(foo)

Is this something you would expect, given that foo doesn't have external visibility?

Looking at Neovim's LSP logs, it looks like the pyright language server is sending a response to the textDocument/rename request, it's just empty:

[DEBUG][2022-03-08 10:11:47] .../vim/lsp/rpc.lua:347	"rpc.send"	{  id = 3,  jsonrpc = "2.0",  method = "textDocument/rename",  params = {    newName = "bar",    position = {      character = 5,      line = 18    },    textDocument = {      uri = "file:///Users/smackesey/stm/code/elementl/dagster/python_modules/dagster/dagster_tests/general_tests/check_tests/test_check.py"    }  }}
[DEBUG][2022-03-08 10:11:47] .../vim/lsp/rpc.lua:454	"rpc.receive"	{  id = 3,  jsonrpc = "2.0"}

In contrast, when I perform the same rename in my toy project the immediate response is not empty:

[DEBUG][2022-03-08 10:15:36] .../vim/lsp/rpc.lua:347	"rpc.send"	{  id = 5,  jsonrpc = "2.0",  method = "textDocument/rename",  params = {    newName = "baz",    position = {      character = 5,      line = 1    },    textDocument = {      uri = "file:///Users/smackesey/Desktop/testpyright/testpy.py"    }  }}
[DEBUG][2022-03-08 10:15:36] .../vim/lsp/rpc.lua:454	"rpc.receive"	{  id = 5,  jsonrpc = "2.0",  result = {    changes = {      ["file:///Users/smackesey/Desktop/testpyright/testpy.py"] = { {          newText = "baz",          range = {            end = {              character = 7,              line = 1            },            start = {              character = 4,              line = 1            }          }        }, {          newText = "baz",          range = {            end = {              character = 13,              line = 2            },            start = {              character = 10,              line = 2            }          }        } }    }  }}

Also: have you considered having Pyright apply privacy rules when renaming? For instance, if I have a function _foo, understanding that _foo is module-local and thus narrowing its reference search scope to within the file when performing a rename? This could potentially make renaming much more useful in large projects.

@erictraut
Copy link
Contributor

It sounds like more investigation is needed. Since this is a language server feature and not a core type checking issue, I'm going to transfer it to the pylance-release project.

@erictraut erictraut transferred this issue from microsoft/pyright Mar 8, 2022
@github-actions github-actions bot added the triage label Mar 8, 2022
@judej judej added the needs investigation Could be an issue - needs investigation label Mar 9, 2022
@github-actions github-actions bot removed the triage label Mar 9, 2022
@heejaechang
Copy link
Contributor

@smackesey can you try it on vscode pylance and let us know whether it still repro? if it does, can you provide a log following instructions on https://github.com/microsoft/pylance-release/blob/main/TROUBLESHOOTING.md#filing-an-issue ?

it should give us more context to figure out what is going on. thank you.

@heejaechang heejaechang added waiting for user response Requires more information from user and removed needs investigation Could be an issue - needs investigation labels Mar 14, 2022
@heejaechang heejaechang added the fixed in next version (main) A fix has been implemented and will appear in an upcoming version label Mar 17, 2022
@smackesey
Copy link
Author

@heejaechang Do you still need help with this? I see you added a "fixed in next version" label.

@heejaechang
Copy link
Contributor

I added our rename to limit scope a bit more if we can. please, try next release and let us know. thank you.

@bschnurr
Copy link
Member

This issue has been fixed in version 2022.3.3, which we've just released. You can find the changelog here: CHANGELOG.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed in next version (main) A fix has been implemented and will appear in an upcoming version waiting for user response Requires more information from user
Projects
None yet
Development

No branches or pull requests

5 participants