vscode-powershell
vscode-powershell copied to clipboard
PowerShell
F2 (rename) doesn't work for PowerShell variables
F2 doesn't seem to work for PowerShell variables, and the "Rename" option doesn't seem to appear. Renaming PowerShell variables can be tedious, so we should work to support this feature.
But we also need to be careful, since (unlike e.g. C# where lexical scope makes renaming safe) PowerShell's dynamic scoping means that renaming local variable references needs to occur conservatively. script:, global:, and env: on the other hand should be easy to rename.
Ok, I've investigated this and we haven't ever supported it -- I was under the impression that we once did and it broke, but that's not the case.
Marking this as up-for-grabs in case anybody else wants to try implementing it (we can help you through).
@rjmholt I would like to give it a try if I can be pointed in the right direction
Awesome! There are going to be a few pieces to the puzzle, but basically it will look like:
- Add binding in vscode-PowerShell and plumb through the request to the LSP API (client request/response)
- Add request handler in PowerShellEditorServices (PSES) (server request/response <-> call/return value)
- Actually do the symbol reference resolution in PSES (find the symbols in the script when the method is called and return them)
- Add tests
Keybinding
https://github.com/PowerShell/vscode-powershell/blob/26079742a26e8195f5e40739a9dec5a6b5b3bca3/package.json#L86-L114
You'll need to add a configuration here and then a new feature with a request type, like this one:
https://github.com/PowerShell/vscode-powershell/blob/0fb852c164d113da2e99044e40141ec6991179af/src/features/ShowHelp.ts#L1-L49
Request Handler
Once you've turned the keybinding into a request, you need to add a request handler on the back end in PSES.
Those get added here:
https://github.com/PowerShell/PowerShellEditorServices/blob/38cb599344ce6c22c4276f9d835c05e8d0c3c703/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs#L126-L135
You also need to create request and response types for deserialisation like this one: https://github.com/PowerShell/PowerShellEditorServices/blob/master/src/PowerShellEditorServices.Protocol/LanguageServer/References.cs
Under the current architecture, the actual request handler will be a method on the LanguageServer object added as an event handler. An example of this is this method: https://github.com/PowerShell/PowerShellEditorServices/blob/38cb599344ce6c22c4276f9d835c05e8d0c3c703/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs#L771-L808
It takes in the request type and the arguments, does whatever it needs to do and if required, fires off a response. Again for the response, you have to create a new response type to serialise to.
That method is where you'd need to call the relevant API to actually find the occurrences of the variable you were given in the relevant scope (hint: you need the request to tell you not just the variable name but also where it is in the script so you can work out the scope).
Language Service API
All PowerShell-language-related analysis done by the extension currently goes through the LanguageService, so you'll want to add a new method on this like Task<FindVariableOccurrencesResult> FindVariableOccurences(ScriptFile scriptFile, string variableName, int lineNumber, int columnNumber) or Task<RenameVariableResult> RenameVariable(...). A similar example is something like the symbol reference API: https://github.com/PowerShell/PowerShellEditorServices/blob/38cb599344ce6c22c4276f9d835c05e8d0c3c703/src/PowerShellEditorServices/Language/LanguageService.cs#L188-L214
AST function
All the stuff above has just been plumbing. This is where we do the fun, actually-parse-PowerShell-and-help-people part.
You need to add some logic that can look at the parsed PowerShell AST, and find the extents (IScriptExtent is a PowerShell type representing a region in a script associated with a syntactic element like a variable). This would then need to be added to the AstOperations static class. Other methods on this class implement a PowerShell AST visitor like this one. In fact it looks like there's already a FindOccurrencesResult in the code but I'm not sure if or where it's used.
Testing
Since the important part of this is in PSES, you'll want to add xUnit tests somewhere like here that correctly find variable references in some example PowerShell files. Some example tests might be:
- A simple script with no scopes and two of the same variable in it
- Two variables, one with 2 occurrences, one with 3, in the same script
- Variables of the same name in different scopes
Stretch Goals
This is already plenty of work, and the important part is to get something small working before trying to enhance it, but there are a couple of extra cases you can ignore at first but might be useful to implement at some point (we can kick that out to another PR or just later):
- Variables using a
${}or a$local:syntax -
script:variables, which have script-lexical rather than dynamic scope -
global:andenv:variables, which may be referenced in other files -
Set-andGet-variable, which can't be done in general but might be doable in cases where the variable name is used verbatim and inline with no scope modifier -
variable:provider path usages (same as above but probably even less used)
Submitting PRs
Since you have to modify two codebases, the general principle is, after any PR is merged neither should have any broken bits. So generally the workflow is:
- Clone vscode-PowerShell and PowerShellEditorServices into sibling directories
- Create a new branch in both
- The "Start Development Extension" run option when you open vscode-PowerShell in VSCode will build and run both with each other for you
- When you have the whole thing (both branches) working the way you want, open PRs with your branches in both vscode-PowerShell and PowerShellEditorServices
- We will review the PRs (sometimes this can take a while) and then merge the PSES PR first, then the vscode-PowerShell one
That should cover most things hopefully, but let us know if you hit any problems. If you get stuck, open a PR, mark it as a WIP and tag us in the comments to let us know you want some guidance
wow, how could I go wrong with such concise instructions, thanks. Give me a few days to review and get my head around all the working parts.
If I try and run any of the LanguageService tests I get the following:-
Message:
System.Management.Automation.CommandNotFoundException : The term 'Get-ExecutionPolicy' is not recognized as the name of a cmdlet, function, script file, or operable program.
Check the spelling of the name, or if a path was included, verify that the path is correct and try again.
do I need to do anything first? (I've followed step 4 from the readme)
Bizarre... @Benny1007 are you running the tests in Windows PowerShell or PowerShell Core?
ah sorry perhaps I should have been a bit clearer. I was just opening the PowerShellEditorServices project in Visual Studio 2017 and running the tests from Test Explorer.
Ah yes. I personally do not use Visual Studio to run the tests... This could be the problem.
Try this in a PowerShell console:
Invoke-Build Build Test
Assuming you have the InvokeBuild module installed from the Gallery.
@rkeithhill, do you use Visual Studio? Any ideas?
I used to use the Visual Studio test explorer and that was a real nice experience but that hasn't worked well in a long time (pretty much since we went cross-platform). These days, I use the command line that you refer to.
Invoke-Build Build Test doesn't work but Invoke-Build Test does but fails tests, this is on the master branch.
Total tests: 194. Passed: 188. Failed: 5. Skipped: 1. Test Run Failed.
What does the first give you?
Did you try:
Invoke-Build Build
Invoke-Build Test
Yep, Invoke-Build Build successfully builds the project. Invoke-Build Test returns lots of test failures...
Starting test execution, please wait...
[xUnit.net 00:00:01.8300459] AsyncDebouncerFlushesAfterInterval [SKIP]
Skipped AsyncDebouncerFlushesAfterInterval
[xUnit.net 00:00:06.7751709] CorrectlyResolvesPaths(givenPath: "file:///path/wi[th]/squ[are/brackets/", expectedPath: "C:\\path\\wi[th]\\squ[are\\brackets\\") [FAIL]
Failed CorrectlyResolvesPaths(givenPath: "file:///path/wi[th]/squ[are/brackets/", expectedPath: "C:\\path\\wi[th]\\squ[are\\brackets\\")
Error Message:
Assert.Equal() Failure
(pos 0)
Expected: C:\path\wi[th]\squ[are\brackets\
Actual: D:\path\wi[th]\squ[are\brackets\
(pos 0)
Stack Trace:
at Microsoft.PowerShell.EditorServices.Test.Session.WorkspaceTests.CorrectlyResolvesPaths(String givenPath, String expectedPath) in D:\dev\repos\PowerShellEditorServices\test\PowerShellEditorServices.Test\Session\WorkspaceTests.cs:line 88
[xUnit.net 00:00:06.7801918] CorrectlyResolvesPaths(givenPath: "file:///home/maxim/test%20folder/%D0%9F%D0%B0%D0%B"..., expectedPath: "C:\\home\\maxim\\test folder\\?????\\helloworld.ps"...) [FAIL]
[xUnit.net 00:00:06.7815654] CorrectlyResolvesPaths(givenPath: "file:///Users/barnaby/%E8%84%9A%E6%9C%AC/Reduce-Di"..., expectedPath: "C:\\Users\\barnaby\\??\\Reduce-Directory") [FAIL]
[xUnit.net 00:00:06.7829945] CorrectlyResolvesPaths(givenPath: "file:///Carrots/A%5Ere/Good/", expectedPath: "C:\\Carrots\\A^re\\Good\\") [FAIL]
[xUnit.net 00:00:06.7849927] CorrectlyResolvesPaths(givenPath: "file:///path/with/no/drive", expectedPath: "C:\\path\\with\\no\\drive") [FAIL]
Failed CorrectlyResolvesPaths(givenPath: "file:///home/maxim/test%20folder/%D0%9F%D0%B0%D0%B"..., expectedPath: "C:\\home\\maxim\\test folder\\?????\\helloworld.ps"...)
Error Message:
Assert.Equal() Failure
(pos 0)
Expected: C:\home\maxim\test folder\?????\helloworl···
Actual: D:\home\maxim\test folder\?????\helloworl···
(pos 0)
Stack Trace:
at Microsoft.PowerShell.EditorServices.Test.Session.WorkspaceTests.CorrectlyResolvesPaths(String givenPath, String expectedPath) in D:\dev\repos\PowerShellEditorServices\test\PowerShellEditorServices.Test\Session\WorkspaceTests.cs:line 88
Failed CorrectlyResolvesPaths(givenPath: "file:///Users/barnaby/%E8%84%9A%E6%9C%AC/Reduce-Di"..., expectedPath: "C:\\Users\\barnaby\\??\\Reduce-Directory")
Error Message:
Assert.Equal() Failure
(pos 0)
Expected: C:\Users\barnaby\??\Reduce-Directory
Actual: D:\Users\barnaby\??\Reduce-Directory
(pos 0)
Stack Trace:
at Microsoft.PowerShell.EditorServices.Test.Session.WorkspaceTests.CorrectlyResolvesPaths(String givenPath, String expectedPath) in D:\dev\repos\PowerShellEditorServices\test\PowerShellEditorServices.Test\Session\WorkspaceTests.cs:line 88
Failed CorrectlyResolvesPaths(givenPath: "file:///Carrots/A%5Ere/Good/", expectedPath: "C:\\Carrots\\A^re\\Good\\")
Error Message:
Assert.Equal() Failure
(pos 0)
Expected: C:\Carrots\A^re\Good\
Actual: D:\Carrots\A^re\Good\
(pos 0)
Stack Trace:
at Microsoft.PowerShell.EditorServices.Test.Session.WorkspaceTests.CorrectlyResolvesPaths(String givenPath, String expectedPath) in D:\dev\repos\PowerShellEditorServices\test\PowerShellEditorServices.Test\Session\WorkspaceTests.cs:line 88
Failed CorrectlyResolvesPaths(givenPath: "file:///path/with/no/drive", expectedPath: "C:\\path\\with\\no\\drive")
Error Message:
Assert.Equal() Failure
(pos 0)
Expected: C:\path\with\no\drive
Actual: D:\path\with\no\drive
(pos 0)
Stack Trace:
at Microsoft.PowerShell.EditorServices.Test.Session.WorkspaceTests.CorrectlyResolvesPaths(String givenPath, String expectedPath) in D:\dev\repos\PowerShellEditorServices\test\PowerShellEditorServices.Test\Session\WorkspaceTests.cs:line 88
If I update the following lines:- PowerShellContext.cs ln 1963 ln 1984 ln 2135 to call 'AddScript' instead of 'AddCommand' all of the tests then run from Visual Studio 2017 test explorer. However I am left with 38 failing tests.
Sorry, meant to get back sooner. The problem here is that the tests use a hosted PowerShell instance (since xUnit doesn't play well with PowerShell), so it depends on some setup in the build. It looks like you solved that one.
The test problems look like those path ones could be test bugs; if you're executing the code from D:, then .NET's URI resolver might resolve file:/// to there rather than C:, which is the assumed root location built into tests. We'd need to fix that by rewriting those tests to be aware of the executing drive unfortunately.
An interim solution might be to try running the build/tests from the C: drive
This is likely a duplicate of https://github.com/PowerShell/vscode-powershell/issues/261
@Benny1007 @rjmholt Any updates on this feature? I would greatly appreciate if I could rename variables only in their respective scope.
From the PowerShell team's perspective, currently we are not actively working on this feature due to some other work. However, if anyone (like @Benny1007) does want to pick it back up, we are happy to review PRs. Especially with it being Hacktoberfest :)
I was under the impression that this would be hard (impossible?) to implement since PowerShell uses dynamic instead of lexical scoping. For instance, given this script:
$foo = "bar"
function useScriptFoo {
"script level foo is $foo"
}
function innerFunc {
"outerFunc foo is $foo"
}
function outerFunc {
$foo = "baz" # this is essentially declaring a new variable named foo
innerFunc
}
useScriptFoo
outerFunc
If you were to rename $foo on line 1, it is not entirely clear which of the other $foo refs should be renamed unless you could analyze the runtime behavior of the script (who calls who) somehow. Even then it is not clear that the $foo assignment in outerFunc is meant to override the script level variable or if it's just declaring a new variable that happens to have the same name.
Adding onto @rkeithhill's example:
& { $foo = 'test' }
. { $foo = 'test' }
& { $foo }
0..10 | ForEach-Object { $foo }
0..10 | Where-Object { $_ -eq $foo }
Invoke-Command -ComputerName dc1 { $foo }
{ $foo } | ForEach-Object { & $_ }
{ $foo } | Out-File ./script.ps1
Yes I think our desire here is to implement renaming only in the current scope, so not trying to infer any scope sharing. So if you were to rename $foo in any of the examples given, it would only rename the single instance, but if you reused the variable multiple times in the same block it would rename all of those (without touching variables of the same name in other blocks above or below).
That could give folks false hope that it worked though... gotta be careful...
Oh I agree, but at the end of the day we can:
- Give people nothing
- Implement a safe, restrained feature
- Make a dangerous decision that does invalid things in various scenarios
So far we've done (1) and I don't think we should do (3).
I get that this is tricky to reason about, and frankly it's hard explaining it to people repeatedly, but there's clearly some demand and there's something we can do and it probably would help in most of the common scenarios.
As a user, I would be happy if I could rename variables in a function without worrying that they will be also renamed in other functions just because the name is the same. Don't overthink this, it's good enough if variables of the same scope can be renamed.
On Sat, Oct 17, 2020, 04:26 Robert Holt [email protected] wrote:
Oh I agree, but at the end of the day we can:
- Give people nothing
- Implement a safe, restrained feature
- Make a dangerous decision that does invalid things in various scenarios
So far we've done (1) and I don't think we should do (3).
I get that this is tricky to reason about, and frankly it's hard explaining it to people repeatedly, but there's clearly some demand and there's something we can do and it probably would help in most of the common scenarios.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/PowerShell/vscode-powershell/issues/1440#issuecomment-710720209, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABRANOGA7JMNN2VY5QW5HADSLDXC3ANCNFSM4FKOK7DQ .
@rjmholt Definitely agree that it would be useful. It's hard to guess what the ratio of confusion/frustration to benefit will be though. Most users won't understand the reason why some variables will be renamed and some won't. They may not even know to check if some weren't.
I don't really have a problem with trying it out in a preview. Say this gets implemented, which of these would you expect to be renamed?
& {
# Rename this
$var = 'thing'
# 1
$var
# 2
0..10 | % { $var }
# 3
{ $var }
# 4
$var = Get-ChildItem
# 5
foreach ($var in (0..10)) {
# 6, 7
$var = 'Number: ' + $var
}
# 8
$var
}
Yes I think our desire here is to implement renaming only in the current scope, so not trying to infer any scope sharing.
Just to clarify, current scope or current block?
I get that this is tricky to reason about, and frankly it's hard explaining it to people repeatedly, but there's clearly some demand and there's something we can do and it probably would help in most of the common scenarios.
Yeah. Can we think of a good way to alert the user that some variables might not be changed? I worry that we're at best sidegrading problems. Right now if we provide nothing, it doesn't cause problems. But adding it as a feature could end up with folks spending way more time debugging (or worse, end up deleting more than they mean to with a Remove-Item). Missing a single variable rename in a big script can be devastating 😕
What about adding it as an editor command with the name Dangerous Variable Rename or something 😄
Ugh, I'd really like this too. I think it should just be best-effort. The editor already highlights matched variables, between that and source control I think it's probably enough to mitigate the danger of our scoping issues.
We're not trying to stop people from breaking code, we're trying to make development more efficient so that you don't stop in the middle of your coding with a ...Huh moment, and don't get interrupted when in the flow. If we replace ALL the instances, in most cases the code actually doesn't break. And it can be argued that is the intent. I have outter variable called X, inner variable I've named X as well, and maybe I want to rename both to Y. Manually fixing by hand the very few edge cases where the inner variable was accidentally named the same often only involved a few lines of code where the entire scope is visible.
