Implementing Client side Rename provider to allow variable/function rename

[Razmo99]

PR Summary

This pull request implements the VSCode RenameProvider class along with two methods within it provideRenameEdits and prepareRename the goal being to complete #261

This will allow for the client to make requests to the server side of the extension when pressing F2 to perform refactoring on variables and functions.

This client side code has been modified from the C# VSCode extension and modified to work with the Powershell extension's server side component.

There will be a sister pull request on the PowershellEditorServices repository to implement the server side.

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets. Please mark anything not applicable to this PR NA.

• [X] PR has a meaningful title
• [X] Summarized changes
• [NA] PR has tests
• [x] This PR is ready to merge and is not work in progress
  • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

[Razmo99]

@microsoft-github-policy-service agree

[SydneyhSmith]

Hi @Razmo99 super appreciate the detailed PR! Just want to give you a heads up that the maintainers are all pretty heads down on commitments at the moment so it may be a couple of weeks before we get a chance to review the PR

[silverqx]

I would like to ask, what is the state of this PR, does it work actually? I fastly scrolled through the proposed PR and also PowerShellEditorServices#2152, looks like @Razmo99 did a great job about registerRenameProvider and worked ~1 year on this.

The code quality looks good to me and contains a lot of tests, which is a good thing.

Would it be possible to review it, git checkout it, test it, and release it in the next version? @andyleejordan

I lack renaming in PS extension so much, would greatly boost dev. productivity in many cases. I know there always is Search & Replace, but properly implemented rename refactorings are gold in some cases 🙂 To just hit ctrl+shift+r or f2 and rename the symbol under the cursor, to avoid classic search and replace.

I know that the PR is huge, but also the functionality is complex.

[andyleejordan]

I would really like to get this feature in, yes. However, we're currently paused on feature work for a bit while some security work takes priority. Said another way: please expect to see it within the year, but probably not in the next month.

[silverqx]

Ok, thx for stating it out, would be nice to make it a little quicker but whatever, at least check-in it out and test it to see if it works and in which state the PR is would be a good idea. I think that having a new feature like this is more interesting for developers than having npm packages at the latest versions every month or every release 😁

[JustinGrote]

For what it's worth I'm currently reviewing the PSES part of this to hopefully help reduce the friction when the opportunity arises for a more thorough review.

[andyleejordan]

Justin's work will be very helpful and should speed up the process!

[JustinGrote]

Here is a build of the latest PSES rename changes. Lots of progress to go but it's here to test out: powershell-2024.6.0-renameAlpha.vsix.zip

You'll need to rename the .vsix.zip to just .vsix and then install using VSIX locally with vscode.

[JustinGrote]

Caught here too:

$NeededTools = @{
    OpenSsl       = 'openssl for macOS'
    PowerShellGet = 'PowerShellGet latest'
    InvokeBuild   = 'InvokeBuild latest'
}

function getMissingTools () {
    $missingTools = @()

    if (needsOpenSsl) {
        $missingTools += $NeededTools.OpenSsl
    }
    if (needsPowerShellGet) {
        $missingTools += $NeededTools.PowerShellGet
    }
    if (needsInvokeBuild) {
        $missingTools += $NeededTools.InvokeBuild
    }

    return $missingTools
}

If you try to rename $NeededTools inside the function, it only renames the external reference but not the internal ones. Vice-versa does not work, if we support one we should support the other.

Maybe this should be a configurable option? Or maybe we require SCRIPT or GLOBAL scope references in order for rename of variables referenced outside functions to work? Probably good to put a warning here that if you try to rename an inner referenced variable but the initial reference is not found, should fail with a nice error message explaining that.

Should maybe look at the AST logic of PSUseDeclaredVarsMoreThanAssignments as a reference (or even call it directly) for finding those references.

[andyleejordan]

Hi! We discussed this in triage and our main condition on accepting it is that it's an opt-in feature controlled by a setting that ideally has a good explanation of when and how it can fail (due to PowerShell scoping). As long as it's opt-in most of our concerns don't apply.

[JustinGrote]

Hi! We discussed this in triage and our main condition on accepting it is that it's an opt-in feature controlled by a setting that ideally has a good explanation of when and how it can fail (due to PowerShell scoping). As long as it's opt-in most of our concerns don't apply.

Right, I was thinking more specifically for this handling, but I plan to assist it in such a way that it will operate as such:

First time you attempt F2 rename, a popup will appear with a link to Markdown docs we will have in the docs section explaining about PowerShell being a dynamic language, and the limitations and at-your-own-risk aspects. You will then have to click an "Accept", "Accept for Workspace", etc. option that will drop a setting in the appropriate location to suppress the dialog and allow rename to continue.

[JustinGrote]

@razmo99 this is great work, FYI, I will be PTO June 14-28 in Europe so I may be limited in further reviews until then but I will probably show this off at PSConfEU and give you full credit!

[Razmo99]

@JustinGrotes, are you back in action from Europe? just wondering what are next steps are to move forward. Do you want me to implement the opt in mechanism on the client side?

[JustinGrote]

@Razmo99 thanks for following up and all your hard work. I've had some life circumstances come up that are going to unfortunately minimize my involvement as a volunteer at the moment, I hope I'll be able to get involved again soon, but I think in the meantime as mentioned by @andyleejordan the opt-in triggered on first F2 rename, controlled by a setting is a good next step as well as preparing a markdown document that details the expectations (no multi-file for now, etc.) and the edge cases. He would ultimately be performing/approving the merge so his guidance would be paramount to the merge.

[JustinGrote]

I added the disclaimer, at least the base scaffolding of it. Not super-happy about the display format, I would like to do markdown preview, but I would have to have it as a button instead and recycle the info message.

https://github.com/user-attachments/assets/c2928880-3273-48b8-ad24-896eb4e3243c

[JustinGrote]

Force pushed a rebase to deal with the silly not-really-merge-conflicts.

[JustinGrote]

@Razmo99 since I've had some time to dive into this, is there a particular reason you made a custom PowerShell/prepareRename rather than using the standard TextDocument/prepareRename that the LSP uses? Since we don't get the full documentUri, that helps explain why unsaved/untitled files don't work right.

[Razmo99]

I didn't know there was a standard prepearRename provider 😅 I made it the way I did as I wanted a separate endpoint to send rename requests to the language server and handle them differently rather them complicating the main rename symbol endpoint

[ALIENQuake]

Great feature, one suggestion, the initial variable name should not contain $ in order to not require repeating this char when providing new name.

[JustinGrote]

Great feature, one suggestion, the initial variable name should not contain $ in order to not require repeating this char when providing new name.

There is logic in the latest commits where it doesn't matter if you have the $ or not, it'll get fixed automatically, and it has checking to make sure it's a valid variable name (e.g. no spaces unless theres a {} around the name, etc.)

[JustinGrote]

I'm closing this PR in favor of #5056 since the scope has drastically changed. @Razmo99 you should get co-author credit on the PR, if not we'll make sure it ends up in the release notes for sure.

[Razmo99]

I'm closing this PR in favor of #5056 since the scope has drastically changed. @Razmo99 you should get co-author credit on the PR, if not we'll make sure it ends up in the release notes for sure.

@JustinGrote no problem at all, honestly I'm just happy to see this finally get implemented as a feature of this plugin!