-
Notifications
You must be signed in to change notification settings - Fork 510
"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
Comments
I think you can do this already by 1) selecting the variable, 2) use the command |
@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:
Basic Explanations: |
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.) |
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: |
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 Now if line 2 was 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. |
@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? |
PowerShell Studio does it as well and I used it religiously.
…On Tue, Dec 6, 2016 at 6:22 AM, Matt McNabb ***@***.***> wrote:
@rkeithhill <https://github.com/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?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#261 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEG3-kvlvLvsAcgdC_PWAXft0mynxmnmks5rFVOXgaJpZM4JtI2E>
.
|
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. |
Great! Can we get that next week? ;)
…On Tue, Dec 6, 2016 at 8:54 AM, David Wilson ***@***.***> wrote:
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.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#261 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEG3-nH5Z8DLHCGTTs5RFqVOrV2DkBxZks5rFXcWgaJpZM4JtI2E>
.
|
Nope ;) You will get a new release with some other shiny features, though. |
Would be a great feature! |
Already a fan of vscode but this would really take it to the next level. |
Also would like this feature |
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. |
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.) |
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. |
@gyro2death have you tried the PowerShell Preview extension with PSReadLine support in the integrated console? |
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:
If you rename the Even if you only rename the single 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:
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. |
But then again, calling a variable inside a function that was defined outside is pretty nasty! 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 |
@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 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. |
I can't believe it's impossible to figure out the actual occurrences of a symbol. |
@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. |
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 # 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 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 {
"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 |
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. |
Thanks for the shoutout @JustinGrote :) 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 :) @JustinGrote : |
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. |
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 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 |
Related on Stack Overflow: PowerShell rename refactor (F2) does nothing in VS Code. |
Thanks for the great proof-of-concept, @SeeminglyScience; I took the liberty of enhancing it to deal with scope prefixes, One thing I noticed is that the use of multiple 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);
} |
@SeeminglyScience I'm not too familiar with the extension API, but what about |
Yep! We'd basically be adding a publicly accessible version of that. Under the covers that's all |
Wait- why is there a need to add a publicly accessible version of it? Isn't it already "publicly accessible" to extensions? |
@starball5 I think it's more exposing the rename action thru the LSP stack, e.g. VSCode -> Omnisharp -> PSES -> Extension and back. |
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 |
can we bump this somehow and get vscode to F2 (rename) a powershell symbol? |
*bump |
EDIT: New Build 2025-03-01 powershell-2025.99.0-renameAlpha.zip
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 |
Awesome! It even takes splatting into account (though it is erroneously adding a |
@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. |
F2 gets me nothing but |
@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. |
I'm working on this again, just rebased it with the latest changes. See the previous post for the newest build |
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
The text was updated successfully, but these errors were encountered: