vscode-powershell: F2 (rename) doesn't work for PowerShell variables

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.

@goblinfactory feel free to make a separate extension like that, it wouldn’t require the PS extension, but it probably will never be in the official extension because:

1. Generative AIs hallucinate
2. The feature has a dependency on an API key and a model that would require long term support that PS team has no control over.

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: and env: variables, which may be referenced in other files
• Set- and Get- 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

ooh, what about we create an F# plugin that simply calls ChatGPT API to refactor any code you hilight (as you require), so F2 will pop up a select from preselects. You store your openAPI api key in preferences? [ rename ] * <- default, press enter, or press shift+F2 [ rename dangerously ] [ suggest refactorings ] [ code review for security issues ] [ other, create new … ]

When you select the “new” refactoring select choice, you could be presented with a dialog that asks you for title,and chatgpt refactoring prompt. The dialog can also show the “final” prompt that will be sent to chatGPT in a kind of live preview mode, which will most likely consist of some generic prefix (F# project in vscode yada yada context) and suffix (post processing, reconsider the response before responding and if necessary run again ) prompt text etc

Just some fun ideas 😄 (it’s trendy and will make bosses love the many productivity options!) I’ve already done a proof of concept to see how chatGPT responds to the tricky rename that was suggested further above, and just for completeness, this was it’s reply

I asked ChatGPT, what happens if you press F2 when on topmost $foo and rename it?

ChatGPT replied, (below)

If you press F2 while your cursor is on the topmost $foo in a PowerShell script in an integrated development environment (IDE) that supports refactoring (like Visual Studio Code), and rename it, the IDE will typically attempt to rename all references to $foo within the same scope.

In your script, there are different scopes involved. The topmost $foo is declared at the script level. The $foo inside outerFunc is a different variable with the same name but in a different scope. Renaming the topmost $foo would not automatically rename the $foo inside outerFunc, because they are considered separate variables due to their scope.

So, if you renamed the topmost $foo to, say, $fooScriptLevel, the IDE should rename the occurrences of $foo in useScriptFoo to $fooScriptLevel, but the $foo inside outerFunc and the $foo referenced in innerFunc (which is the one from outerFunc) should remain unchanged.

The script after renaming might look like this:

$fooScriptLevel = "bar"

function useScriptFoo {
    "script level foo is $fooScriptLevel"
}

function innerFunc {
    "outerFunc foo is $foo"
}

function outerFunc {
    $foo = "baz" # this is essentially declaring a new variable named foo
    innerFunc
}

useScriptFoo
outerFunc

That’s very usable I rekon? I can see what @JustinGrote is saying, that the hassle of coding this up using something that parses the actual code, or .net code graphs et al, are seriously non trivial and the effort might be better spent elsewhere?.. perhaps like something fun like this? (grin!) What do you all think?

@JustinGrote sorry, I think you replied to a comment I deleted. I re-read everyones recent comments and realised they had basically said similar things. (already been covered) My delete and your comment crossed paths, you’re just too darned fast! 😄

Sure it’s reasonable, but you’d need some kind of visitor process for heavily nested scopes, and there’s lots of scope cases that fall outside of this. There also shouldn’t be an expectation for cross-file rename.

The point is to implement it where it doesn’t clobber incorrectly is going to be a difficult setup.

I think you also underestimate the number of people who will wildly complain that F2 is “broken” if it doesn’t work perfectly every time and doesn’t screw up their script, hence why it needs to be gated behind a warning that it is best effort. Level-set expectations.

It’s still an up-for-grabs item so you could go ahead and try a PR, it’s on the radar but not committed on anyone’s roadmap at the moment.

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.

Certainly I work very hard not to reuse var names in PS, and that’s pretty easy since lambda functions aren’t very common, but a lot of those edge cases like saving a Scriptblock to a variable and executing it in a different scope are more common than you’d think in some tricker modules. As @SeeminglyScience also mentioned we could have a setting for “dangerous renames” which could be more aggressive on renames within the same file only (cross file would be very unlikely to be effective.

Which is a greater interrupt of the flow: Your “huh” moment, or spending the next 4 hours trying to figure out why you’re suddenly getting a weird error message nowhere near what you were working on only to find the rename grabbed something you completely didn’t expect (or worse, finding out at release runtime from users because you didn’t have tests).

@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 😄

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.

This is likely a duplicate of https://github.com/PowerShell/vscode-powershell/issues/261

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.

@rjmholt I would like to give it a try if I can be pointed in the right direction