RenameProvider for variable/function renaming

[Razmo99]

Issue: Smart Variable Rename

PR Summary

This pull request implements the LSP textDocument/rename and textDocument/prepareRename handlers for PowerShell.

https://github.com/PowerShell/PowerShellEditorServices/pull/2152 exposes the related service settings in vscode-powershell.

Reviewer's Guide

PowerShell is not a statically typed language and as such, some variable definitions and relations are determined at runtime, therefore true static analysis of a full PowerShell project is not possible. However, by presenting a disclaimer of the limitations, we feel we can provide fairly stable rename support within a single document for several scenarios without turning this into a bug/issue farm.

All work is driven through a RenameService which controls the messaging and registers the handlers. The general flow is to:

1. Identify if a symbol at least meets the basic criteria for a rename, as a performance optimization
2. Find the original definition/assignment where the target symbol is referenced
3. Use that as an anchor for all renames in the document.

The RenameHandlerTests and PrepareRenameHandlerTests follow the rename behavior, and the Handler is considered our public API for these purposes and what we test against, even though there is a lot of implementation detail inside.

Taking this approach minimizes state maintenance in the Service at the expense of probably more-than-necessary AST walks, but current testing performance finds this to be sufficient even for large documents. There are several places caching of some AST walks could be performed to optimize performance but they are out of scope for this initial PR.

Reviewer Asks

• Identify areas where there is a public PowerShell API where logic is being duplicated that could be replaced with the API instead
• Find edge cases/scenarios that we should be able to reasonably cover, if not, we will add them to the exclusions.

TODO List

• [x] Replaced Custom LSP calls with OmniSharp IPrepareRenameHandler and IRenameHandler
• [x] Use custom ScriptExtentAdapter and ScriptPositionAdapter types to translate between script and lsp positions to avoid "off-by-one" errors since lsp is zero-based and script is one-based
• [x] Tests for PrepareRenameProvider and RenameProvider
• [x] Deduplicate PrepareRenameProvider and RenameProvider logic
• [x] Add Configuration Support via Omnisharp ILanguageServerConfiguration
• [x] Create Settings for Function and Variable Aliases and pass thru the info to LSP
• [x] Wire up Settings to actually work
• [x] Add Message Support via ILanguageServerFacade
• [x] Present Risk Warning and acceptance via LSP ShowMessageRequest (cannot set the setting directly ATM as it's not an LSP standard scenario)
• [x] Rewrite IterativeFunctionVisitor to an AstVisitor
• [x] Rewrite IterativeVariableVisitor to an AstVisitor
• [x] Finish fixing tests
• [x] Add more "sad path" tests for things that should not get triggered for rename
• [x] Add E2E tests for testing the actual rename process
• [x] Validate FunctionName and VariableName to be valid before passing through
• [ ] Support scope modifiers (e.g. $GLOBAL, $SCRIPT, etc.)
• [x] Evaluate against PSScriptAnalyzer rules to use consistent discovery methods if reasonable

Potential Additional Features

• [ ] Simple conditional splat parameter rename.

function Do-Thing($test) {} #test should rename
$x = @{test = 2} #test here should rename
$x.test = 5 #Test here should rename
Do-Thing @x

• [ ] Renaming $_ or $PSItem can place a $_ = or $PSItem = at the top of the scope. (I would use this a lot and seems pretty reasonable to do, but we would need an implicit definition of $PSItem in the current structure since we require a definition)

More Tests

• [x] Space in variable or function rename request should fail "$va riable" along with any other invalid characters for a rename
• [ ] Parameter, Function, and Variable renames where they are not defined in scope should fail
• [x] Renaming a parameter and removing the '-' should still work

[JustinGrote]

Looks like there's some tests that need to be fixed after pulling in latest main

[Razmo99]

Hey @JustinGrote I've updated the tests to use Async, they should at least run / compile now

[JustinGrote]

Latest MacOS/Ubuntu test failures look like a path combine issue. System.IO.DirectoryNotFoundException : Could not find a part of the path '/Users/runner/work/PowerShellEditorServices/PowerShellEditorServices/test/PowerShellEditorServices.Test.Shared/Refactoring\Utilities/TestDetection.ps1'.

Note the backslash after Refactoring, may want to make sure you are using Path.Combine or Join-Path

EDIT: Found the backslash hardcoded, refactored to use the three-argument path.combine, this should still work with net462

[JustinGrote]

OK, all tests pass in CI now :). I'll do my best to get a first-pass review done sometime this week, and provide devcontainer/codespaces instructions to allow people to test for edge cases we need to document.

[JustinGrote]

Looking pretty good so far in terms of design, some cleanup will be needed.

Several foreach rename issues I've already come across, I'll try to define tests for these when I get a chance.

#Doesnt rename testvar
$a = 1..5
$b = 6..10
function test {
    process {
        foreach ($testvar in $a) {
            $testvar
        }
        
        foreach ($testvar in $b) {
            $testvar
        }
    }
}

#Renames both foreach local variables ($aPath), should only rename the one in scope (agreed this is bad practice though)
function Import-FileNoWildcard {
    [CmdletBinding(SupportsShouldProcess=$true)]
    param(
        # Specifies a path to one or more locations.
        [Parameter(Mandatory=$true,
                   Position=0,
                   ParameterSetName="Path",
                   ValueFromPipeline=$true,
                   ValueFromPipelineByPropertyName=$true,
                   HelpMessage="Path to one or more locations.")]
        [Alias("PSPath", "Path")]
        [ValidateNotNullOrEmpty()]
        [string[]]
        $Path2
    )

    begin {
    }

    process {
        # Modify [CmdletBinding()] to [CmdletBinding(SupportsShouldProcess=$true)]
        $paths = @()
        foreach ($aPath in $Path2) {
            if (!(Test-Path -LiteralPath $aPath)) {
                $ex = New-Object System.Management.Automation.ItemNotFoundException "Cannot find path '$aPath' because it does not exist."
                $category = [System.Management.Automation.ErrorCategory]::ObjectNotFound
                $errRecord = New-Object System.Management.Automation.ErrorRecord $ex,'PathNotFound',$category,$aPath
                $psCmdlet.WriteError($errRecord)
                continue
            }

            # Resolve any relative paths
            $paths += $psCmdlet.SessionState.Path.GetUnresolvedProviderPathFromPSPath($aPath)
        }

        foreach ($aPath in $paths) {
            if ($pscmdlet.ShouldProcess($aPath, 'Operation')) {
                # Process each path
                $aPath
            }
        }
    }

    end {
    }
}

[JustinGrote]

We will also need to make sure we account for the scoping issues as mentioned by Rob that are very difficult to handle, even if it means we warn about this as best effort, or straight up refuse to do it. https://github.com/PowerShell/vscode-powershell/issues/261#issuecomment-465249755

[Razmo99]

I made some test cases for renaming from within a for / each loop and limiting the rename to the scope if the var is defined within the creation of the statement. My only concern is a case like below.

If you run the code powershell treats the $i = 10 as the highest scope assignment which is then redefined in the loop but kept in function scope for when its printed. As it stands if you renamed $i = 10 it would rename the loop using the same variable. however if you renamed from within the loop it would not touch the $i=10

$a = 1..5
$b = 6..10
function test {
    process {
        $i = 10
        
        for ($Renamed = 0; $Renamed -lt $b.Count; $Renamed++) {
            $null = $Renamed
        }
        
        for ($i = 0; $i -lt $a.Count; $i++) {
            write-output $i
        }
        
        write-output "I will be 5 : $i not 10"
    }
}
test

[JustinGrote]

@Razmo99 uh I don't know what I did but a bunch of non-matching main commits showed up, it's not letting me force push a reset, can you please revert to ff9c25ba9180f9e71829bbccfcbcd0961ed99bce?

EDIT: Nevermind fixed it!

[JustinGrote]

Rebased so tests can run again.

[JustinGrote]

I've started a new branch that refactors things to the LSP native rename functions, which has the main advantages of requiring no client side code, so should "just work" in other editors such as NeoVim, etc.

https://github.com/PowerShell/PowerShellEditorServices/tree/refactor/rename-using-omnisharp-lsp

It compiles and runs but has a couple issues in the AST matching that I'm aware of and will fix. Once this works and looks good, we can remove 90% of the code on the vscode-powershell for this PR, the only thing we will need is for the opt-in confirmation.

[JustinGrote]

Here's a build from the new alpha branch powershell-2024.2.2-renameAlpha2.zip

[Razmo99]

I've started a new branch that refactors things to the LSP native rename functions, which has the main advantages of requiring no client side code, so should "just work" in other editors such as NeoVim, etc.

https://github.com/PowerShell/PowerShellEditorServices/tree/refactor/rename-using-omnisharp-lsp

It compiles and runs but has a couple issues in the AST matching that I'm aware of and will fix. Once this works and looks good, we can remove 90% of the code on the vscode-powershell for this PR, the only thing we will need is for the opt-in confirmation.

@JustinGrote Do you want any help on this fixing up the AST issues?

[JustinGrote]

@Razmo99 once I finish things up and get all tests passing I'll have you help me with more scenarios and ASTVisitor rework, there's others who will jump in too. It'll be pretty simple to contribute new test scenarios now, you do it all in one place by creating the before and after .ps1 and then update the class in the same folder with the target cursor location, so we can have people contribute tests and then we can investigate and support (or not support) them separately.

[JustinGrote]

Here is also the new branch for vscode-powershell client side, the only thing needed ATM is definition of the new settings.

https://github.com/PowerShell/vscode-powershell/tree/feature/lsp-rename

[Razmo99]

@Razmo99 once I finish things up and get all tests passing I'll have you help me with more scenarios and ASTVisitor rework, there's others who will jump in too. It'll be pretty simple to contribute new test scenarios now, you do it all in one place by creating the before and after .ps1 and then update the class in the same folder with the target cursor location, so we can have people contribute tests and then we can investigate and support (or not support) them separately.

To easy, I'll wait for you to ping me when ready