Skip to content

"Smart" variable rename #261

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

Open
adbertram opened this issue Aug 25, 2016 · 44 comments
Open

"Smart" variable rename #261

adbertram opened this issue Aug 25, 2016 · 44 comments
Labels
Area-Symbols & References Issue-Enhancement A feature request (enhancement). Up for Grabs Will shepherd PRs.

Comments

@adbertram
Copy link
Contributor

When I rename a variable in one place, I want to ensure all references to that variable are renamed that are in the same scope. Basically, I want to ensure when a rename a variable that anything that might be depending on that name is renamed as well in the same script. Here's an example:

http://www.screencast.com/t/ggUIoKqSC

@daviwil daviwil added this to the Backlog milestone Sep 2, 2016
@daviwil daviwil added the Issue-Enhancement A feature request (enhancement). label Sep 2, 2016
@Bill-Stewart
Copy link

I think you can do this already by 1) selecting the variable, 2) use the command editor.action.selectHighlights (default key mapping is Ctrl+Shift+L), then 3) type the replacement name.

@sgtoj
Copy link

sgtoj commented Sep 23, 2016

@Bill-Stewart You're somewhat correct. @adbertram is referring to the Rename Symbol (F2) command. It will smartly update the symbol (e.g. variable, class, functions, etc.) versus a blind change of all occurrence of a word.

Example:

# get some objects to work with
$something    = (Get-Something);
$anotherthing = (Get-AnotherThing);

# set some value from the first object
$someValue1  = $something.SomeValue;
$someValue2  = $something.SomeValue;

# set another one from the other object
$somevalue3  = $anotherthing.SomeValue;

Basic Explanations:
With Symbol Rename, the editor will understand that both occurrence SomeValue from $something.SomeValue are the same but the one from $anotherthing.SomeValue is not. So the Symbol Rename command of SomeValue property on the $something object will only make two changes even though there are at least 3 occurrence of "somevalue" (actually there are 6).

@Bill-Stewart
Copy link

Well, I interpreted the OP's request as variables rather than object members. I wouldn't mind this if performance is good. (As noted, we can already change variables pretty easily.)

@mattmcnabb
Copy link
Contributor

Plus one on the request from @adbertram. The key here is that the token rename should be scope-aware in PowerShell. We've been doing this in ISESteroids for a while:

image

@rkeithhill
Copy link
Contributor

rkeithhill commented Dec 5, 2016

Because of PowerShell's dynamic scoping that is hard to get right i.e. infer scripter intent. In the example above, I'd argue that renaming $Var1 on line 2 shouldn't rename the variable on line 5. Line 2 should be considered a "new" $Var1 variable because of PowerShell's "copy on write" behavior.

Now if line 2 was $script:Var1 = ... then it would be obvious that line 5 should be renamed.

However, with dynamic scopes you don't know until runtime which scope a variable is defined in if it isn't defined in the local scope. That makes it hard to ensure correctness with this feature request IMO.

@mattmcnabb
Copy link
Contributor

@rkeithhill you're right about the intention of the example above. Sounds like it's difficult to implement - still would be really nice to have. Wonder how it's achieved in ISE Steroids?

@adbertram
Copy link
Contributor Author

adbertram commented Dec 6, 2016 via email

@daviwil
Copy link
Contributor

daviwil commented Dec 6, 2016

The question isn't whether it's possible (it is), it's whether the change we make is correct in terms of how PowerShell's variable scoping works. Right now our symbol renaming is primitive but once we start doing it the "right" way, it will adhere as well as it can to PowerShell's scoping rules.

@adbertram
Copy link
Contributor Author

adbertram commented Dec 6, 2016 via email

@daviwil
Copy link
Contributor

daviwil commented Dec 6, 2016

Nope ;) You will get a new release with some other shiny features, though.

@Clijsters
Copy link

Would be a great feature!

@ajansveld
Copy link

Already a fan of vscode but this would really take it to the next level.

@johntiger1
Copy link

Also would like this feature

@smk89
Copy link

smk89 commented Jan 10, 2018

I'm a bit surprised that it's not in already. The hard bit should be correctly finding all of the references to a variable, which we can already do by hitting Shift+F12. What's left is a way to replace the highlighted strings with a new string.

@TylerLeonhardt TylerLeonhardt modified the milestone: Future May 24, 2018
@Stephanevg
Copy link

Stephanevg commented Dec 8, 2018

I am also verry interested in this feature. is there any idea when this will be added?( I don't want to be a negative Nancy, but this issue has been open for two years now.)

@EnderSyth
Copy link

I'd like to chime in on this as well. VSCode is great for many languages but for powershell it's lacking some feature you get used to in other languages or even other editors (the ISE integrated terminal with intelisense). I hope this get's added as it would improve the workflow for myself and many others.

@TylerLeonhardt
Copy link
Member

(the ISE integrated terminal with intelisense)

@gyro2death have you tried the PowerShell Preview extension with PSReadLine support in the integrated console?

@rjmholt
Copy link
Contributor

rjmholt commented Feb 19, 2019

I wrote up a quick note in a reddit post a while ago.

PowerShell's dynamic (i.e. determined at runtime) scope makes this impossible to do in general. Take this example:

$x = 1

function Write-X
{
    $x
}

function Write-XInScope
{
    $x = 7
    Write-X
}

Write-X
Write-XInScope

If you rename the $x in Write-X, which $xs do you rename?

Even if you only rename the single $x occurrence in Write-X, you've still broken Write-XInScope.

I think this issue has been blocked by the hope that it might be possible to do something clever here, but it's not; this is runtime-only information, so the only way to always know the scope of a variable is to execute the script.

My thinking is that we should instead take a conservative approach:

  • Renaming a normal variable only changes the name of it in its immediate scope
  • Renaming a script: variable is lexical, so we can safely rename all occurrences there
  • Renaming a global: or env: is slightly more work, but just requires renaming the variable everywhere in a workspace.

This policy may still cause unexpected effects, but it's simpler than almost all other ways of doing things (the other simple one being to rename all occurrences in a script file -- but that file could still be called within another scope).

I have the feeling that many people write PowerShell scripts as though PowerShell has lexical scope, but unfortunately that's not the case. We'll do our best to balance expectations with not breaking PowerShell, but this is one of those edge areas.

@AndersRask
Copy link

I wrote up a quick note in a reddit post a while ago.

PowerShell's dynamic (i.e. determined at runtime) scope makes this impossible to do in general. Take this example:

$x = 1

function Write-X
{
    $x
}

function Write-XInScope
{
    $x = 7
    Write-X
}

Write-X
Write-XInScope

But then again, calling a variable inside a function that was defined outside is pretty nasty!
Having read a ton of other peoples PowerShell, I agree though, that we cannot expect lexical scope to be used always, however I think people who would do this, are not even aware of the implications or problems with the code ;-)

I would be fine with that the refactoring only worked inside the advanced function you were currently working, as this would probably be the case in most instances anway

@JustinGrote
Copy link
Collaborator

JustinGrote commented Dec 28, 2022

@SeidChr as @rjmholt correctly, noted, there's no way to implement this in a way that users of other languages would expect, e.g. to correctly rename all references of a variable in all scopes. However, if we limit the expectation to only rename all occurances of a variable within a single function scope (e.g. you update a parameter and all the references to that parameter within a function will be updated), and maybe have a popup that a user has to acknowledge "this doesn't work like other languages", then this may be feasible.

Trust me, you're not alone, it annoys me all the time when F2 doesnt work, and I may take a stab at it if I can fit it into my priority deck.

Here's some prior art that is sort of along the lines of an approach that might work, mine the AST and then rebuild it: https://www.powershellgallery.com/packages/PSModuleDevelopment/2.2.6.72/Content/functions%5Crefactor%5CRename-PSMDParameter.ps1, @FriedrichWeinmann may have some thoughts.

@SeidChr
Copy link

SeidChr commented Dec 28, 2022

I can't believe it's impossible to figure out the actual occurrences of a symbol.
I'm not asking to rename values in Set-Variable or similar. Although i believe even this is possible.
But i have no choice than to believe it.

@JustinGrote
Copy link
Collaborator

@SeidChr it's not that it's hard to find all occurances of the variable. The problem is that the same variable name can have different contexts. Take this example:

$x = 5

function test ($x, $y) {
  return $x + $y
}

Renaming all occurances of $x will break this script.

@SeeminglyScience
Copy link
Collaborator

SeeminglyScience commented Dec 28, 2022

Edit(ish): I see Justin already beat me to it but hopefully this still clears some stuff up 😁

It's not that it's impossible to find all occurrences, it's that it's impossible to tell which occurrences actually refer to the same variable.

Take this for instance:

$someVar = 10
function DoSomething {
    $someVar = 40
}

Is this the same instance? No not typically. But if you do . DoSomething now it is. Most folks don't know you can dot source a function by name so maybe that's not a huge problem by itself, but here are a few more examples:

# Not same
$var = 10
0..10 | Select-Object @{n='SomeProperty';e={ $var = 30 * $_; $var }}

# Not same
$var = 10
Get-ChildItem | Rename-Item -NewName { $var = $_.FullName + (Get-Random); $var }

# Same
$var = 10
0..10 | ForEach-Object {
    $var += 5
}

# Not same
$var = 10
. (Get-Module Pester) { $var = 30 }

# Same
$var = 10
$sb = { $var = 30 }
. $sb

# ???
$var = 10
$sb = { $var = 30 }
$shouldDotSource = Get-Random -Minimum 0 -Maximum 2
if ($shouldDotSource) {
    . $sb
} else {
    & $sb
}

There's no way to know statically whether any of these scriptblocks will even be invoked, much less what context they save their state to.

Now, all that might not matter to you and I get that - and I also think it would be useful regardless. It's a problem of how we can articulate the limitations to the user. If we just implement the feature with all of the problems that it will inevitably have, then folks will assume we cracked it somehow and we will potentially end up causing a lot of very difficult to troubleshoot bugs.

That said, we do provide extension points that make this pretty easy to put into a profile function. In your $profile, put this:

if ($psEditor) {
    function Profile.RenameVariable {
        [CmdletBinding()]
        param($Context)
        end {
            $variable = Find-Ast -AtCursor
            if ($variable -isnot [System.Management.Automation.Language.VariableExpressionAst]) {
                $psEditor.Window.ShowWarningMessage('No variable found at current cursor location.')
                return
            }

            $esp = [Microsoft.PowerShell.EditorServices.Extensions.EditorObjectExtensions, Microsoft.PowerShell.EditorServices]::GetExtensionServiceProvider(
                $psEditor)
            $task = $esp.EditorUI.PromptInputAsync('New variable name?')

            while (-not $task.AsyncWaitHandle.WaitOne(200)) { }
            $newName = $task.GetAwaiter().GetResult()

            if ([string]::IsNullOrWhiteSpace($newName)) {
                return
            }

            Find-Ast { $_.VariablePath.UserPath -eq $variable.VariablePath.UserPath } |
                Sort-Object { $_.Extent.StartOffset } -Descending |
                ForEach-Object {
                    $Context.CurrentFile.InsertText(
                        "`$$newName",
                        $_.Extent.StartLineNumber,
                        $_.Extent.StartColumnNumber,
                        $_.Extent.EndLineNumber,
                        $_.Extent.EndColumnNumber)
                }
        }
    }

    $splat = @{
        Name = 'Profile.RenameVariable'
        ScriptBlock = ${function:Profile.RenameVariable}
        DisplayName = 'Rename variable in current document'
    }

    Register-EditorCommand @splat
}

And in your VSCode keybindings.json include this:

    {
        "key": "F2",
        "command": "PowerShell.InvokeRegisteredEditorCommand",
        "args": { "commandName": "Profile.RenameVariable" },
        "when": "editorLangId == 'powershell'"
    },

Now you have a really basic version of this functionality. It can be expanded to hit the whole workspace and/or Set-Variable as well, though a good bit less trivial.

@JustinGrote
Copy link
Collaborator

My thought is that we should be able to trigger a modal dialog on first use linking to an article expressing the limitations, and make them click "OK" or "Don't show again" (dropping a setting into their user profile) to at least provide the basic functioanlity, which 99% of the time all I want is to rename a parameter and have all references to that parameter in my function updated (and I generally don't bleed context so I'm not worried about parent variables, etc.). This is a viable MVP case for the feature I think, and if we can get it to a point that it can handle more reasonable edge cases, it can evolve.

@FriedrichWeinmann
Copy link

FriedrichWeinmann commented Dec 29, 2022

Thanks for the shoutout @JustinGrote :)
I'll have to admit that the function you pointed out above is still from one of my early days of messing with the AST, so pardon the implementation details ^^

That said, it is quite possible to detect variable assignments and scope boundaries using AST so it should be doable. Catching Set-Variable and Get-Variable references is also possible, though some of the explicit scope references might be difficult. Catching splatted values might also be possible, but not 100% reliable (but let's be honest, if you use weird hashtable definitions on a splat for Set-Variable, I shall accuse you of intentional refactor evasion ;) ).

Did some of that work in my Refactor module, which is my AST scanning platform where I will also migrate most of my PSMD stuff to.

Quite frankly, one of my Christmas break projects is to write a new PSScriptAnalyzer rule to detect variable scope boundary violations, which would solve most of my concerns for solving this - heck, we could gate the functionality behind that check and to refuse to do the refactor as long as SBVs are detected ...

@SeeminglyScience : Excellent edge cases writeup :)
While we could find much of that ... but only in the file/workspace we work on. So yeah, some ambiguity around dotsourcing remain ... but anybody who knows that one should be able to understand and handle those cases. Furthermore, I think that internal variables should be dealt with like non-public properties/methods/classes in C# where yes you can access them with reflection, but we'll change things as we please and you'll have to live with it.

@JustinGrote :
Regarding the parameter renaming feature - you may want to look into Refactor as it already is. While it can't (yet) rename the variables inside the function (unless you tell it how, it's extensible), it already can scan entire code-bases and find & update references. With some more modules (linked in the readme) it can also scan github and Azure DevOps for uses of the command.

@SeeminglyScience
Copy link
Collaborator

@SeeminglyScience : Excellent edge cases writeup :)
While we could find much of that ... but only in the file/workspace we work on. So yeah, some ambiguity around dotsourcing remain ... but anybody who knows that one should be able to understand and handle those cases. Furthermore, I think that internal variables should be dealt with like non-public properties/methods/classes in C# where yes you can access them with reflection, but we'll change things as we please and you'll have to live with it.

I agree that it's definitely possible to make a "good enough" implementation for folks who are following what we would consider to be best practices. That said, the vast majority of users of the extension won't be, and the fact that the user still needs to be very careful needs to be communicated somehow.

@Razmo99
Copy link

Razmo99 commented Sep 19, 2023

Hello, not sure if this should be another post, I think it's relevant to the discussion.

I've been wanting this feature for a while and forked the repo and started working on a branch

I've just got it working for function renaming within a single file (with some test cases I could think of), as it appeared easier to me than variables Please keep in mind my C# skills are basic and this is my first post to a public repo, and the code is just a proof of concept with minimal safety's

Its using the System.Management.Automation.Language.ICustomAstVisitor2 which is used to visit all the nodes within the AST and to track the state it's in should rename and what's the target function etc.

I'm just looking to know if this is the right direction for implementation before I go too far down the rabbit hole because I could see that I could setup another Visitor for variables. Wondering what the next steps are

@starball5
Copy link

Related on Stack Overflow: PowerShell rename refactor (F2) does nothing in VS Code.

@andyleejordan andyleejordan added the Needs: Triage Maintainer attention needed! label Nov 20, 2023
@mklement0
Copy link
Contributor

mklement0 commented Nov 28, 2023

Thanks for the great proof-of-concept, @SeeminglyScience; I took the liberty of enhancing it to deal with scope prefixes, $using: references, and splatted variables in this answer to the linked SO post. The limitations, tradeoffs and assumptions made are listed there.

One thing I noticed is that the use of multiple .CurrentFile.InsertText() invariably makes each substitution a separate action with respect to undo / redo functionality in VSCode, which is somewhat unfortunate (you cannot just revert to the previous state with a single Ctrl-Z).

Is there any way around that? Do the underlying VSCode APIs support that in principle? Is it worth opening an issue here?

@SeeminglyScience
Copy link
Collaborator

Is there any way around that? Do the underlying VSCode APIs support that in principle? Is it worth opening an issue here?

Yeah we just don't surface it atm. We could add an API sorta like

class FileEdit
{
    IFileRange Range { get; }

    string NewContent { get; }
}

partial class FileContext
{
    void ApplyEdits(params FileEdit[] edits);
}

@starball5
Copy link

starball5 commented Nov 30, 2023

@SeeminglyScience I'm not too familiar with the extension API, but what about WorkspaceEdit? It has insert and replace.

@SeeminglyScience
Copy link
Collaborator

@SeeminglyScience I'm not too familiar with the extension API, but what about WorkspaceEdit? It has insert and replace.

Yep! We'd basically be adding a publicly accessible version of that. Under the covers that's all InsertText uses today iirc

@starball5
Copy link

Yep! We'd basically be adding a publicly accessible version of that. Under the covers that's all InsertText uses today iirc

Wait- why is there a need to add a publicly accessible version of it? Isn't it already "publicly accessible" to extensions? workspace already has applyEdit, and it's the return type of RenameProvider

@JustinGrote
Copy link
Collaborator

@starball5 I think it's more exposing the rename action thru the LSP stack, e.g. VSCode -> Omnisharp -> PSES -> Extension and back.

@Razmo99
Copy link

Razmo99 commented Dec 4, 2023

If you have a look at what the C# extension does to setup the renameprovider it has all you'll need to add this functionality, additionally the prepearrename method can be used to catch out the use of dot sourcing etc and then allow a msg to be raised to the user

@JustinGrote JustinGrote added Up for Grabs Will shepherd PRs. and removed Needs: Triage Maintainer attention needed! labels Dec 30, 2023
@Omzig
Copy link

Omzig commented Mar 21, 2024

can we bump this somehow and get vscode to F2 (rename) a powershell symbol?

@SeidChr
Copy link

SeidChr commented Mar 21, 2024

*bump

@JustinGrote
Copy link
Collaborator

JustinGrote commented Sep 12, 2024

EDIT: New Build 2025-03-01
Progress is happening! Here is a new alpha preview build of the functionality if you want to try it out.

powershell-2025.99.0-renameAlpha.zip

  1. Download the above file
  2. Rename .zip to .vsix
  3. Open VSCode, hit F1, and type "VSIX" and hit enter
  4. Browse for the vsix file
  5. Reload VSCode

Open a .ps1 file on disk (unsaved/untitled currently unsupported) and hit F2. You'll get a popup asking you to opt in, then you can try out renaming functions/providers/symbols

Capture.mp4

@MartinGC94
Copy link
Contributor

Awesome! It even takes splatting into account (though it is erroneously adding a $ before the parameter name).
Regarding the creation of an alias attribute when renaming a parameter, I don't think it should be enabled by default. I'd imagine most people will primarily use it in the initial development phase where breaking changes are not a concern so it just gets in the way.

@JustinGrote
Copy link
Collaborator

@MartinGC94 that's a fair point, it'll be a toggleable setting either way.

We are building a test suite of supported scenarios, I'm currently refactoring it to be LSP-only so that it will be portable to neovim and whatnot. Once that's done I may recruit you if you have some time to help us refine the AST visitors to help with more edge cases we pick up.

@sba923
Copy link

sba923 commented Sep 20, 2024

F2 gets me nothing but Unable to write to User Settings because powershell.renameSymbol.acceptRenameDisclaimer is not a registered configuration.

@JustinGrote
Copy link
Collaborator

@sba923 I'm currently doing a pretty major rewrite/refactor on another branch, the vsix I provided should be fine but I should have a better version by net week.

@JustinGrote
Copy link
Collaborator

I'm working on this again, just rebased it with the latest changes. See the previous post for the newest build

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Symbols & References Issue-Enhancement A feature request (enhancement). Up for Grabs Will shepherd PRs.
Projects
None yet
Development

No branches or pull requests