Skip to content

F2 (rename) doesn't work for PowerShell variables #1440

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

Closed
rjmholt opened this issue Jul 17, 2018 · 38 comments
Closed

F2 (rename) doesn't work for PowerShell variables #1440

rjmholt opened this issue Jul 17, 2018 · 38 comments
Labels
Area-Symbols & References Issue-Enhancement A feature request (enhancement). Up for Grabs Will shepherd PRs.

Comments

@rjmholt
Copy link
Contributor

rjmholt commented Jul 17, 2018

F2 doesn't seem to work for PowerShell variables, and the "Rename" option doesn't seem to appear. Renaming PowerShell variables can be tedious, so we should work to support this feature.

But we also need to be careful, since (unlike e.g. C# where lexical scope makes renaming safe) PowerShell's dynamic scoping means that renaming local variable references needs to occur conservatively. script:, global:, and env: on the other hand should be easy to rename.

@rjmholt rjmholt added Issue-Enhancement A feature request (enhancement). Area-IntelliSense labels Jul 17, 2018
@MarkMichaelis
Copy link

I can confirm that this isn't working and it is disappointing. :(

@rjmholt rjmholt self-assigned this Oct 10, 2018
@rjmholt rjmholt added Issue-Bug A bug to squash. Issue-Enhancement A feature request (enhancement). and removed Issue-Enhancement A feature request (enhancement). Issue-Bug A bug to squash. labels Oct 10, 2018
@rjmholt rjmholt removed their assignment Oct 15, 2018
@rjmholt rjmholt added the Up for Grabs Will shepherd PRs. label Oct 15, 2018
@rjmholt
Copy link
Contributor Author

rjmholt commented Oct 15, 2018

Ok, I've investigated this and we haven't ever supported it -- I was under the impression that we once did and it broke, but that's not the case.

Marking this as up-for-grabs in case anybody else wants to try implementing it (we can help you through).

@Benny1007
Copy link
Contributor

@rjmholt I would like to give it a try if I can be pointed in the right direction

@rjmholt
Copy link
Contributor Author

rjmholt commented Mar 26, 2019

Awesome! There are going to be a few pieces to the puzzle, but basically it will look like:

  • Add binding in vscode-PowerShell and plumb through the request to the LSP API (client request/response)
  • Add request handler in PowerShellEditorServices (PSES) (server request/response <-> call/return value)
  • Actually do the symbol reference resolution in PSES (find the symbols in the script when the method is called and return them)
  • Add tests

Keybinding

"keybindings": [
{
"command": "PowerShell.ShowHelp",
"key": "ctrl+f1",
"when": "editorTextFocus && editorLangId == 'powershell'"
},
{
"command": "PowerShell.ExpandAlias",
"key": "shift+alt+e",
"when": "editorTextFocus && editorLangId == 'powershell'"
},
{
"command": "PowerShell.ShowAdditionalCommands",
"key": "shift+alt+s",
"when": "editorTextFocus && editorLangId == 'powershell'"
},
{
"command": "PowerShell.RunSelection",
"key": "f8",
"when": "editorTextFocus && editorLangId == 'powershell'"
},
{
"command": "editor.action.insertSnippet",
"when": "editorTextFocus && editorLangId == 'powershell'",
"mac": "cmd+alt+j",
"win": "ctrl+alt+j",
"linux": "ctrl+alt+j"
}
],

You'll need to add a configuration here and then a new feature with a request type, like this one:

/*---------------------------------------------------------
* Copyright (C) Microsoft Corporation. All rights reserved.
*--------------------------------------------------------*/
import vscode = require("vscode");
import { LanguageClient, NotificationType, RequestType } from "vscode-languageclient";
import { IFeature } from "../feature";
import { Logger } from "../logging";
export const ShowHelpRequestType =
new RequestType<string, void, void, void>("powerShell/showHelp");
export class ShowHelpFeature implements IFeature {
private command: vscode.Disposable;
private deprecatedCommand: vscode.Disposable;
private languageClient: LanguageClient;
constructor(private log: Logger) {
this.command = vscode.commands.registerCommand("PowerShell.ShowHelp", (item?) => {
if (this.languageClient === undefined) {
this.log.writeAndShowError(`<${ShowHelpFeature.name}>: ` +
"Unable to instantiate; language client undefined.");
return;
}
if (!item || !item.Name) {
const editor = vscode.window.activeTextEditor;
const selection = editor.selection;
const doc = editor.document;
const cwr = doc.getWordRangeAtPosition(selection.active);
const text = doc.getText(cwr);
this.languageClient.sendRequest(ShowHelpRequestType, text);
} else {
this.languageClient.sendRequest(ShowHelpRequestType, item.Name);
}
});
}
public dispose() {
this.command.dispose();
this.deprecatedCommand.dispose();
}
public setLanguageClient(languageclient: LanguageClient) {
this.languageClient = languageclient;
}
}

Request Handler

Once you've turned the keybinding into a request, you need to add a request handler on the back end in PSES.

Those get added here:

https://github.com/PowerShell/PowerShellEditorServices/blob/38cb599344ce6c22c4276f9d835c05e8d0c3c703/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs#L126-L135

You also need to create request and response types for deserialisation like this one: https://github.com/PowerShell/PowerShellEditorServices/blob/master/src/PowerShellEditorServices.Protocol/LanguageServer/References.cs

Under the current architecture, the actual request handler will be a method on the LanguageServer object added as an event handler. An example of this is this method: https://github.com/PowerShell/PowerShellEditorServices/blob/38cb599344ce6c22c4276f9d835c05e8d0c3c703/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs#L771-L808

It takes in the request type and the arguments, does whatever it needs to do and if required, fires off a response. Again for the response, you have to create a new response type to serialise to.

That method is where you'd need to call the relevant API to actually find the occurrences of the variable you were given in the relevant scope (hint: you need the request to tell you not just the variable name but also where it is in the script so you can work out the scope).

Language Service API

All PowerShell-language-related analysis done by the extension currently goes through the LanguageService, so you'll want to add a new method on this like Task<FindVariableOccurrencesResult> FindVariableOccurences(ScriptFile scriptFile, string variableName, int lineNumber, int columnNumber) or Task<RenameVariableResult> RenameVariable(...). A similar example is something like the symbol reference API: https://github.com/PowerShell/PowerShellEditorServices/blob/38cb599344ce6c22c4276f9d835c05e8d0c3c703/src/PowerShellEditorServices/Language/LanguageService.cs#L188-L214

AST function

All the stuff above has just been plumbing. This is where we do the fun, actually-parse-PowerShell-and-help-people part.

You need to add some logic that can look at the parsed PowerShell AST, and find the extents (IScriptExtent is a PowerShell type representing a region in a script associated with a syntactic element like a variable). This would then need to be added to the AstOperations static class. Other methods on this class implement a PowerShell AST visitor like this one. In fact it looks like there's already a FindOccurrencesResult in the code but I'm not sure if or where it's used.

Testing

Since the important part of this is in PSES, you'll want to add xUnit tests somewhere like here that correctly find variable references in some example PowerShell files. Some example tests might be:

  • A simple script with no scopes and two of the same variable in it
  • Two variables, one with 2 occurrences, one with 3, in the same script
  • Variables of the same name in different scopes

Stretch Goals

This is already plenty of work, and the important part is to get something small working before trying to enhance it, but there are a couple of extra cases you can ignore at first but might be useful to implement at some point (we can kick that out to another PR or just later):

  • Variables using a ${} or a $local: syntax
  • script: variables, which have script-lexical rather than dynamic scope
  • global: and env: variables, which may be referenced in other files
  • Set- and Get- variable, which can't be done in general but might be doable in cases where the variable name is used verbatim and inline with no scope modifier
  • variable: provider path usages (same as above but probably even less used)

Submitting PRs

Since you have to modify two codebases, the general principle is, after any PR is merged neither should have any broken bits. So generally the workflow is:

  • Clone vscode-PowerShell and PowerShellEditorServices into sibling directories
  • Create a new branch in both
  • The "Start Development Extension" run option when you open vscode-PowerShell in VSCode will build and run both with each other for you
  • When you have the whole thing (both branches) working the way you want, open PRs with your branches in both vscode-PowerShell and PowerShellEditorServices
  • We will review the PRs (sometimes this can take a while) and then merge the PSES PR first, then the vscode-PowerShell one

That should cover most things hopefully, but let us know if you hit any problems. If you get stuck, open a PR, mark it as a WIP and tag us in the comments to let us know you want some guidance

@Benny1007
Copy link
Contributor

wow, how could I go wrong with such concise instructions, thanks. Give me a few days to review and get my head around all the working parts.

@Benny1007
Copy link
Contributor

Benny1007 commented Mar 28, 2019

If I try and run any of the LanguageService tests I get the following:-

Message:

System.Management.Automation.CommandNotFoundException : The term 'Get-ExecutionPolicy' is not recognized as the name of a cmdlet, function, script file, or operable program.
Check the spelling of the name, or if a path was included, verify that the path is correct and try again.

do I need to do anything first? (I've followed step 4 from the readme)

@TylerLeonhardt
Copy link
Member

Bizarre... @Benny1007 are you running the tests in Windows PowerShell or PowerShell Core?

@Benny1007
Copy link
Contributor

ah sorry perhaps I should have been a bit clearer. I was just opening the PowerShellEditorServices project in Visual Studio 2017 and running the tests from Test Explorer.

@TylerLeonhardt
Copy link
Member

Ah yes. I personally do not use Visual Studio to run the tests... This could be the problem.

Try this in a PowerShell console:

Invoke-Build Build Test

Assuming you have the InvokeBuild module installed from the Gallery.

@rkeithhill, do you use Visual Studio? Any ideas?

@rkeithhill
Copy link
Contributor

I used to use the Visual Studio test explorer and that was a real nice experience but that hasn't worked well in a long time (pretty much since we went cross-platform). These days, I use the command line that you refer to.

@Benny1007
Copy link
Contributor

Invoke-Build Build Test doesn't work but Invoke-Build Test does but fails tests, this is on the master branch.

Total tests: 194. Passed: 188. Failed: 5. Skipped: 1.
Test Run Failed.

@TylerLeonhardt
Copy link
Member

What does the first give you?

Did you try:

Invoke-Build Build
Invoke-Build Test

@Benny1007
Copy link
Contributor

Benny1007 commented Mar 30, 2019

Yep, Invoke-Build Build successfully builds the project. Invoke-Build Test returns lots of test failures...

Starting test execution, please wait...
[xUnit.net 00:00:01.8300459]     AsyncDebouncerFlushesAfterInterval [SKIP]
Skipped  AsyncDebouncerFlushesAfterInterval
[xUnit.net 00:00:06.7751709]     CorrectlyResolvesPaths(givenPath: "file:///path/wi[th]/squ[are/brackets/", expectedPath: "C:\\path\\wi[th]\\squ[are\\brackets\\") [FAIL]
Failed   CorrectlyResolvesPaths(givenPath: "file:///path/wi[th]/squ[are/brackets/", expectedPath: "C:\\path\\wi[th]\\squ[are\\brackets\\")
Error Message:
 Assert.Equal() Failure
          � (pos 0)
Expected: C:\path\wi[th]\squ[are\brackets\
Actual:   D:\path\wi[th]\squ[are\brackets\
          � (pos 0)
Stack Trace:
   at Microsoft.PowerShell.EditorServices.Test.Session.WorkspaceTests.CorrectlyResolvesPaths(String givenPath, String expectedPath) in D:\dev\repos\PowerShellEditorServices\test\PowerShellEditorServices.Test\Session\WorkspaceTests.cs:line 88
[xUnit.net 00:00:06.7801918]     CorrectlyResolvesPaths(givenPath: "file:///home/maxim/test%20folder/%D0%9F%D0%B0%D0%B"..., expectedPath: "C:\\home\\maxim\\test folder\\?????\\helloworld.ps"...) [FAIL]
[xUnit.net 00:00:06.7815654]     CorrectlyResolvesPaths(givenPath: "file:///Users/barnaby/%E8%84%9A%E6%9C%AC/Reduce-Di"..., expectedPath: "C:\\Users\\barnaby\\??\\Reduce-Directory") [FAIL]
[xUnit.net 00:00:06.7829945]     CorrectlyResolvesPaths(givenPath: "file:///Carrots/A%5Ere/Good/", expectedPath: "C:\\Carrots\\A^re\\Good\\") [FAIL]
[xUnit.net 00:00:06.7849927]     CorrectlyResolvesPaths(givenPath: "file:///path/with/no/drive", expectedPath: "C:\\path\\with\\no\\drive") [FAIL]
Failed   CorrectlyResolvesPaths(givenPath: "file:///home/maxim/test%20folder/%D0%9F%D0%B0%D0%B"..., expectedPath: "C:\\home\\maxim\\test folder\\?????\\helloworld.ps"...)
Error Message:
 Assert.Equal() Failure
          � (pos 0)
Expected: C:\home\maxim\test folder\?????\helloworl···
Actual:   D:\home\maxim\test folder\?????\helloworl···
          � (pos 0)
Stack Trace:
   at Microsoft.PowerShell.EditorServices.Test.Session.WorkspaceTests.CorrectlyResolvesPaths(String givenPath, String expectedPath) in D:\dev\repos\PowerShellEditorServices\test\PowerShellEditorServices.Test\Session\WorkspaceTests.cs:line 88
Failed   CorrectlyResolvesPaths(givenPath: "file:///Users/barnaby/%E8%84%9A%E6%9C%AC/Reduce-Di"..., expectedPath: "C:\\Users\\barnaby\\??\\Reduce-Directory")
Error Message:
 Assert.Equal() Failure
          � (pos 0)
Expected: C:\Users\barnaby\??\Reduce-Directory
Actual:   D:\Users\barnaby\??\Reduce-Directory
          � (pos 0)
Stack Trace:
   at Microsoft.PowerShell.EditorServices.Test.Session.WorkspaceTests.CorrectlyResolvesPaths(String givenPath, String expectedPath) in D:\dev\repos\PowerShellEditorServices\test\PowerShellEditorServices.Test\Session\WorkspaceTests.cs:line 88
Failed   CorrectlyResolvesPaths(givenPath: "file:///Carrots/A%5Ere/Good/", expectedPath: "C:\\Carrots\\A^re\\Good\\")
Error Message:
 Assert.Equal() Failure
          � (pos 0)
Expected: C:\Carrots\A^re\Good\
Actual:   D:\Carrots\A^re\Good\
          � (pos 0)
Stack Trace:
   at Microsoft.PowerShell.EditorServices.Test.Session.WorkspaceTests.CorrectlyResolvesPaths(String givenPath, String expectedPath) in D:\dev\repos\PowerShellEditorServices\test\PowerShellEditorServices.Test\Session\WorkspaceTests.cs:line 88
Failed   CorrectlyResolvesPaths(givenPath: "file:///path/with/no/drive", expectedPath: "C:\\path\\with\\no\\drive")
Error Message:
 Assert.Equal() Failure
          � (pos 0)
Expected: C:\path\with\no\drive
Actual:   D:\path\with\no\drive
          � (pos 0)
Stack Trace:
   at Microsoft.PowerShell.EditorServices.Test.Session.WorkspaceTests.CorrectlyResolvesPaths(String givenPath, String expectedPath) in D:\dev\repos\PowerShellEditorServices\test\PowerShellEditorServices.Test\Session\WorkspaceTests.cs:line 88

@Benny1007
Copy link
Contributor

I may just delete my fork and re-clone?

@Benny1007
Copy link
Contributor

If I update the following lines:-
PowerShellContext.cs
ln 1963
ln 1984
ln 2135
to call 'AddScript' instead of 'AddCommand' all of the tests then run from Visual Studio 2017 test explorer. However I am left with 38 failing tests.

@TylerLeonhardt
Copy link
Member

@Benny1007 did you try re-cloning?

@Benny1007
Copy link
Contributor

@Benny1007 did you try re-cloning?

yeah that was from the clone

@rjmholt
Copy link
Contributor Author

rjmholt commented Apr 1, 2019

Sorry, meant to get back sooner. The problem here is that the tests use a hosted PowerShell instance (since xUnit doesn't play well with PowerShell), so it depends on some setup in the build. It looks like you solved that one.

The test problems look like those path ones could be test bugs; if you're executing the code from D:, then .NET's URI resolver might resolve file:/// to there rather than C:, which is the assumed root location built into tests. We'd need to fix that by rewriting those tests to be aware of the executing drive unfortunately.

An interim solution might be to try running the build/tests from the C: drive

@rjmholt
Copy link
Contributor Author

rjmholt commented May 3, 2019

This is likely a duplicate of #261

@ClassyCircuit
Copy link

@Benny1007 @rjmholt Any updates on this feature? I would greatly appreciate if I could rename variables only in their respective scope.

@ghost ghost added the Needs: Maintainer Attention Maintainer attention needed! label Oct 15, 2020
@TylerLeonhardt TylerLeonhardt removed the Needs: Maintainer Attention Maintainer attention needed! label Oct 15, 2020
@TylerLeonhardt
Copy link
Member

From the PowerShell team's perspective, currently we are not actively working on this feature due to some other work. However, if anyone (like @Benny1007) does want to pick it back up, we are happy to review PRs. Especially with it being Hacktoberfest :)

@rkeithhill
Copy link
Contributor

I was under the impression that this would be hard (impossible?) to implement since PowerShell uses dynamic instead of lexical scoping. For instance, given this script:

$foo = "bar"

function useScriptFoo {
    "script level foo is $foo"
}

function innerFunc {
    "outerFunc foo is $foo"
}

function outerFunc {
    $foo = "baz" # this is essentially declaring a new variable named foo
    innerFunc
}

useScriptFoo
outerFunc

If you were to rename $foo on line 1, it is not entirely clear which of the other $foo refs should be renamed unless you could analyze the runtime behavior of the script (who calls who) somehow. Even then it is not clear that the $foo assignment in outerFunc is meant to override the script level variable or if it's just declaring a new variable that happens to have the same name.

@ghost ghost added the Needs: Maintainer Attention Maintainer attention needed! label Oct 16, 2020
@SeeminglyScience
Copy link
Collaborator

SeeminglyScience commented Oct 16, 2020

Adding onto @rkeithhill's example:

& { $foo = 'test' }

. { $foo = 'test' }

& { $foo }

0..10 | ForEach-Object { $foo }

0..10 | Where-Object { $_ -eq $foo }

Invoke-Command -ComputerName dc1 { $foo }

{ $foo } | ForEach-Object { & $_ }

{ $foo } | Out-File ./script.ps1

@rjmholt
Copy link
Contributor Author

rjmholt commented Oct 16, 2020

Yes I think our desire here is to implement renaming only in the current scope, so not trying to infer any scope sharing. So if you were to rename $foo in any of the examples given, it would only rename the single instance, but if you reused the variable multiple times in the same block it would rename all of those (without touching variables of the same name in other blocks above or below).

@TylerLeonhardt
Copy link
Member

That could give folks false hope that it worked though... gotta be careful...

@rjmholt
Copy link
Contributor Author

rjmholt commented Oct 17, 2020

Oh I agree, but at the end of the day we can:

  1. Give people nothing
  2. Implement a safe, restrained feature
  3. Make a dangerous decision that does invalid things in various scenarios

So far we've done (1) and I don't think we should do (3).

I get that this is tricky to reason about, and frankly it's hard explaining it to people repeatedly, but there's clearly some demand and there's something we can do and it probably would help in most of the common scenarios.

@ClassyCircuit
Copy link

ClassyCircuit commented Oct 17, 2020 via email

@SeeminglyScience
Copy link
Collaborator

@rjmholt Definitely agree that it would be useful. It's hard to guess what the ratio of confusion/frustration to benefit will be though. Most users won't understand the reason why some variables will be renamed and some won't. They may not even know to check if some weren't.

I don't really have a problem with trying it out in a preview. Say this gets implemented, which of these would you expect to be renamed?

& {
    # Rename this
    $var = 'thing'

    # 1
    $var

    # 2
    0..10 | % { $var }

    # 3
    { $var }

    # 4
    $var = Get-ChildItem

    # 5
    foreach ($var in (0..10)) {
        # 6, 7
        $var = 'Number: ' + $var
    }

    # 8
    $var
}

Yes I think our desire here is to implement renaming only in the current scope, so not trying to infer any scope sharing.

Just to clarify, current scope or current block?

I get that this is tricky to reason about, and frankly it's hard explaining it to people repeatedly, but there's clearly some demand and there's something we can do and it probably would help in most of the common scenarios.

Yeah. Can we think of a good way to alert the user that some variables might not be changed? I worry that we're at best sidegrading problems. Right now if we provide nothing, it doesn't cause problems. But adding it as a feature could end up with folks spending way more time debugging (or worse, end up deleting more than they mean to with a Remove-Item). Missing a single variable rename in a big script can be devastating 😕

What about adding it as an editor command with the name Dangerous Variable Rename or something 😄

@SydneyhSmith SydneyhSmith removed the Needs: Maintainer Attention Maintainer attention needed! label Oct 20, 2020
@andyleejordan
Copy link
Member

Ugh, I'd really like this too. I think it should just be best-effort. The editor already highlights matched variables, between that and source control I think it's probably enough to mitigate the danger of our scoping issues.

@andyleejordan andyleejordan added this to the Consider-vNext milestone Apr 6, 2021
@goblinfactory
Copy link

goblinfactory commented Dec 29, 2021

We're not trying to stop people from breaking code, we're trying to make development more efficient so that you don't stop in the middle of your coding with a ...Huh moment, and don't get interrupted when in the flow. If we replace ALL the instances, in most cases the code actually doesn't break. And it can be argued that is the intent. I have outter variable called X, inner variable I've named X as well, and maybe I want to rename both to Y. Manually fixing by hand the very few edge cases where the inner variable was accidentally named the same often only involved a few lines of code where the entire scope is visible.

@ghost ghost added the Needs: Maintainer Attention Maintainer attention needed! label Dec 29, 2021
@StevenBucher98 StevenBucher98 removed the Needs: Maintainer Attention Maintainer attention needed! label Jan 5, 2022
@starball5
Copy link

Related on Stack Overflow: PowerShell rename refactor (F2) does nothing in VS Code.

@JustinGrote
Copy link
Collaborator

Seems like this is best approached in a low-hanging fruit process:

  1. Simply rename things in the same block initially (1,4,8 in @SeeminglyScience example above) and every time they do it, pop up a warning message that explains the limitations with a "Don't Show Again" button that saves it in the settings.
  2. Add more renames that can be relatively "safe" (e.g. a directly executed scriptblock that isn't saved to a variable first so that it might be used in another scope and inherit a different var) and more detail to the warning message a link to a article with the limitations.
  3. Keep adding things that can be reasonably accurate.

The only annoying thing here would be maybe resetting the warning each time we add something which might get some users very annoyed.

@JustinGrote
Copy link
Collaborator

JustinGrote commented Nov 20, 2023

We're not trying to stop people from breaking code, we're trying to make development more efficient so that you don't stop in the middle of your coding with a ...Huh moment, and don't get interrupted when in the flow. If we replace ALL the instances, in most cases the code actually doesn't break. And it can be argued that is the intent. I have outter variable called X, inner variable I've named X as well, and maybe I want to rename both to Y. Manually fixing by hand the very few edge cases where the inner variable was accidentally named the same often only involved a few lines of code where the entire scope is visible.

Certainly I work very hard not to reuse var names in PS, and that's pretty easy since lambda functions aren't very common, but a lot of those edge cases like saving a Scriptblock to a variable and executing it in a different scope are more common than you'd think in some tricker modules. As @SeeminglyScience also mentioned we could have a setting for "dangerous renames" which could be more aggressive on renames within the same file only (cross file would be very unlikely to be effective.

Which is a greater interrupt of the flow: Your "huh" moment, or spending the next 4 hours trying to figure out why you're suddenly getting a weird error message nowhere near what you were working on only to find the rename grabbed something you completely didn't expect (or worse, finding out at release runtime from users because you didn't have tests).

@andyleejordan andyleejordan added the Needs: Triage Maintainer attention needed! label Nov 20, 2023
@JustinGrote
Copy link
Collaborator

JustinGrote commented Nov 20, 2023

Sure it's reasonable, but you'd need some kind of visitor process for heavily nested scopes, and there's lots of scope cases that fall outside of this. There also shouldn't be an expectation for cross-file rename.

The point is to implement it where it doesn't clobber incorrectly is going to be a difficult setup.

I think you also underestimate the number of people who will wildly complain that F2 is "broken" if it doesn't work perfectly every time and doesn't screw up their script, hence why it needs to be gated behind a warning that it is best effort. Level-set expectations.

It's still an up-for-grabs item so you could go ahead and try a PR, it's on the radar but not committed on anyone's roadmap at the moment.

@goblinfactory
Copy link

goblinfactory commented Nov 20, 2023

@JustinGrote sorry, I think you replied to a comment I deleted. I re-read everyones recent comments and realised they had basically said similar things. (already been covered) My delete and your comment crossed paths, you're just too darned fast! :D

@goblinfactory
Copy link

goblinfactory commented Nov 21, 2023

ooh, what about we create an F# plugin that simply calls ChatGPT API to refactor any code you hilight (as you require), so F2 will pop up a select from preselects. You store your openAPI api key in preferences?
[ rename ] * <- default, press enter, or press shift+F2
[ rename dangerously ]
[ suggest refactorings ]
[ code review for security issues ]
[ other, create new ... ]

When you select the "new" refactoring select choice, you could be presented with a dialog that asks you for title,and chatgpt refactoring prompt. The dialog can also show the "final" prompt that will be sent to chatGPT in a kind of live preview mode, which will most likely consist of some generic prefix (F# project in vscode yada yada context) and suffix (post processing, reconsider the response before responding and if necessary run again ) prompt text etc

Just some fun ideas :D (it's trendy and will make bosses love the many productivity options!)
I've already done a proof of concept to see how chatGPT responds to the tricky rename that was suggested further above, and just for completeness, this was it's reply

I asked ChatGPT, what happens if you press F2 when on topmost $foo and rename it?

ChatGPT replied, (below)

If you press F2 while your cursor is on the topmost $foo in a PowerShell script in an integrated development environment (IDE) that supports refactoring (like Visual Studio Code), and rename it, the IDE will typically attempt to rename all references to $foo within the same scope.

In your script, there are different scopes involved. The topmost $foo is declared at the script level. The $foo inside outerFunc is a different variable with the same name but in a different scope. Renaming the topmost $foo would not automatically rename the $foo inside outerFunc, because they are considered separate variables due to their scope.

So, if you renamed the topmost $foo to, say, $fooScriptLevel, the IDE should rename the occurrences of $foo in useScriptFoo to $fooScriptLevel, but the $foo inside outerFunc and the $foo referenced in innerFunc (which is the one from outerFunc) should remain unchanged.

The script after renaming might look like this:

$fooScriptLevel = "bar"

function useScriptFoo {
    "script level foo is $fooScriptLevel"
}

function innerFunc {
    "outerFunc foo is $foo"
}

function outerFunc {
    $foo = "baz" # this is essentially declaring a new variable named foo
    innerFunc
}

useScriptFoo
outerFunc

That's very usable I rekon? I can see what @JustinGrote is saying, that the hassle of coding this up using something that parses the actual code, or .net code graphs et al, are seriously non trivial and the effort might be better spent elsewhere?... perhaps like something fun like this? (grin!)
What do you all think?

@JustinGrote
Copy link
Collaborator

@goblinfactory feel free to make a separate extension like that, it wouldn't require the PS extension, but it probably will never be in the official extension because:

  1. Generative AIs hallucinate
  2. The feature has a dependency on an API key and a model that would require long term support that PS team has no control over.

@JustinGrote JustinGrote removed the Needs: Triage Maintainer attention needed! label Dec 30, 2023
@JustinGrote
Copy link
Collaborator

I'm closing this issue in favor of duplicate #261 as it has better details on potential implementation

@JustinGrote JustinGrote closed this as not planned Won't fix, can't repro, duplicate, stale Dec 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Symbols & References Issue-Enhancement A feature request (enhancement). Up for Grabs Will shepherd PRs.
Projects
None yet
Development

No branches or pull requests