-
Notifications
You must be signed in to change notification settings - Fork 238
RenameProvider for variable/function renaming #2152
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
Conversation
e390755
to
db3d833
Compare
caa9e27
to
677f42c
Compare
Looks like there's some tests that need to be fixed after pulling in latest main
Hey @JustinGrote I've updated the tests to use Async, they should at least run / compile now
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
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.
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 { } }
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)
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
test/PowerShellEditorServices.Test/Refactoring/RefactorVariableTests.cs
Outdated
Show resolved
Hide resolved
test/PowerShellEditorServices.Test.Shared/Refactoring/Functions/ForeachFunction.ps1
Outdated
Show resolved
Hide resolved
test/PowerShellEditorServices.Test.Shared/Refactoring/Functions/ForeachFunction.ps1
Outdated
Show resolved
Hide resolved
test/PowerShellEditorServices.Test.Shared/Refactoring/Functions/ForeachFunction.ps1
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/PowerShell/Refactoring/IterativeVariableVisitor.cs
Outdated
Show resolved
Hide resolved
test/PowerShellEditorServices.Test/Refactoring/RefactorFunctionTests.cs
Outdated
Show resolved
Hide resolved
ff9c25b
to
386b707
Compare
...educe some unnecessary ast searches
1897e7b
to
981f6e9
Compare
Rebased to main
Uh oh!
There was an error while loading. Please reload this page.
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:The
RenameHandlerTests
andPrepareRenameHandlerTests
follow the rename behavior, and theHandler
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
TODO List
IPrepareRenameHandler
andIRenameHandler
ScriptExtentAdapter
andScriptPositionAdapter
types to translate between script and lsp positions to avoid "off-by-one" errors since lsp is zero-based and script is one-basedShowMessageRequest
(cannot set the setting directly ATM as it's not an LSP standard scenario)AstVisitor
AstVisitor
Potential Additional Features
More Tests