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.

#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

• Replaced Custom LSP calls with OmniSharp IPrepareRenameHandler and IRenameHandler
• 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
• Tests for PrepareRenameProvider and RenameProvider
• Deduplicate PrepareRenameProvider and RenameProvider logic
• Add Configuration Support via Omnisharp ILanguageServerConfiguration
• Create Settings for Function and Variable Aliases and pass thru the info to LSP
• Wire up Settings to actually work
• Add Message Support via ILanguageServerFacade
• Present Risk Warning and acceptance via LSP ShowMessageRequest (cannot set the setting directly ATM as it's not an LSP standard scenario)
• Rewrite IterativeFunctionVisitor to an AstVisitor
• Rewrite IterativeVariableVisitor to an AstVisitor
• Finish fixing tests
• Add more "sad path" tests for things that should not get triggered for rename
• Add E2E tests for testing the actual rename process
• Validate FunctionName and VariableName to be valid before passing through
• Support scope modifiers (e.g. $GLOBAL, $SCRIPT, etc.)
• 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

• 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
• 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.
PowerShell/vscode-powershell#261 (comment)

[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 ff9c25b?

EDIT: Nevermind fixed it!

[HeyItsGilbert]

Just installed. It renamed the function in the file that was open. VSCode saw a reference in a nother file but the reference wasn't renamed. I also noticed it didn't add the Alias attribute. Let me know if there's any other info I could provide to help.

Also checked if I could rename from where it was used. But that gave me an error about the function not being defined.

I don't see anything in the PowerShell or the PowerShell: LSP Trace output panels with logging turned up.

[JustinGrote]

VSCode saw a reference in another file but the reference wasn't renamed.

As was explained in the README that you clicked "I Accept" on, rename is limited to a single file only :). Whereas the references may or may not be able to find cross-file references, there's no guarantee those are the same reference, and that's fine when read-only but potentially disastrous when making changes.

Also the Alias is now an opt-in setting, but I think it needs a little more work, you'll need to check settings for it and enable it.

[HeyItsGilbert]

As was explained in the README that you clicked "I Accept" on,

Should have RTFM. My bad! This seems like the kind of thing that would be nice to have opt-in. It requires an extra step so users have to explicitly decide and at that point we can warn them of the possible issues (e.g. ensuring all the code base is loaded into AST, how to check if all the references are correct, how to exclude files from being referenced, etc.). With the rename functionality you also have the option to hit Ctrl + Enter to see the Refactor Preview pane. That shows you a list of changes and you can choose which to apply.

Also the Alias is now an opt-in setting

Why opt in? If we start with the assumption that the user isn't familiar with the Alias attribute, at worst they have an extra/unused line of code. This may prompt them to investigate what it is and then make an informed decision. Compared to a user not knowing and not realizing they can offer a simple backwards compatible feature.

[JustinGrote]

Should have RTFM. My bad! This seems like the kind of thing that would be nice to have opt-in.

I know, but the amount of unit tests to cover a proper implementation, not to mention what the model would have to look like, is extremely ugly, only to have it only ever be a half-baked, "works sometimes" implementation. There's just too many ways that dot sourcing etc. break this model. It might be somewhat feasible for functions in a best effort manner given the same limitations, but for variables it's far too risky as people reuse variable names within functions constantly.

The intent of this feature is for simple refactors within a single file, e.g. renaming a variable within a function. It's not meant for multi-file project-wide refactors. You really need a type-safe language for something like that to be effective which PowerShell isn't.

Why opt in? If we start with the assumption that the user isn't familiar with the Alias attribute, at worst they have an extra/unused line of code. This may prompt them to investigate what it is and then make an informed decision. Compared to a user not knowing and not realizing they can offer a simple backwards compatible feature.

During development a lot of people felt the ergonomics of the Alias attribute coming in was annoying, as they weren't looking for backwards compatibility, just to rename a variable/parameter of a function-in-development, so more often than not they were going back and deleting the alias than it being actually useful.

[JustinGrote]

As an example for variables, here's what Find All References returns:

Should we rename all of these $result? They aren't all the same variable.

Same with Parameters. Finding all references across files with splatting, etc. is extremely difficult. What if you defined your splat in one file and reference it in another? Do we need to cross correlate your dotsources? How do we know those are accurate?

Function definitions and usage have their issues, but tend to be fairly "unique" to a module/project, but if you have two modules in the same repo with their own internal function definitions, how do we find the module "boundary"?

And these are just the problems I can think of off the top of my head. As mentioned in the above, the major risk here is turning this into a "feature/bug farm". If I enable function renaming across files, immediately we are going to get requests for variable rename across files, and then people will be disappointed when it won't work, so we have to establish certain reasonable boundaries. Maybe we will enable function renaming across files as an opt-in feature in the future, but that's another set of scenarios and unit tests etc. and there's only so much I'm willing to do with my volunteer time on this vs. other pressing issues :)