Fix #17: Add go to definition support for dot sourced file paths by dee-see · Pull Request #786 · PowerShell/PowerShellEditorServices

Dominic (dee-see) · 2018-10-27T21:29:53Z

Fix #17: Add go to definition support for dot sourced file paths

I would have liked to implement this with FindDeclarationVisitor but it didn't really make sense as the definition of a dot-sourcing is the file itself and there is no VisitFileAsAWhole method on the visitor!

I think the current implementation is a decent compromise though, let me know what you think.

As always, I'm working on Linux and I can't run release build/tests on my machine so I'll keep an eye on the AppVeyor builds.

Side note: I discovered an existing issue with Go To Definition when references were dot-sourced using Windows directory separators when running on Linux.

In this file:

. .\ReferenceFileA.ps1 # notice the backwards slash
Get-ChildItem

My-Function "testc"

You could F12 on My-Function and it would work, but in fact PSES didn't find anything in the "ReferencedFiles" because of the Windows directory separator and had to go over every file in the workspace to find the definition. This bug happens to be fixed by my implementation of Go-To-Dotsourced-Files.

Dominic (dee-see) · 2018-10-27T21:33:40Z

Also, it's a bit strange that a dot-sourcing Symbol has a "Function" SymbolType. It causes a couple of quirks, the hover of the dot-source being "Function ./path.ps1" being one of them. Introducing a new
SymbolType seemed like an interesting idea to explore but I felt like it was a big enough job to be a PR by itself without any new features coming with it.

Fix build issues (net451 compatibility and public method comments) Fix test Make sure test is not platform-specific

TylerLeonhardt · 2018-11-07T22:30:28Z

Dominic (@dee-see) thanks for this PR! Sorry for the delay. I was on vacation for a bit 😄

Rob Holt (rjmholt)

This looks pretty good Dominic (@dee-see). Left a couple of comments. Thanks for your contribution!

Rob Holt (rjmholt)

This looks good to me. Thanks Dominic (@dee-see)!

TylerLeonhardt · 2018-11-15T03:43:30Z

question - does this handle things like $PSScriptRoot? That would be a very common usecase of dot sourcing in my opinion.

TylerLeonhardt · 2018-11-21T17:19:13Z

ping Dominic (@dee-see) :)

Dominic (dee-see) · 2018-11-21T23:52:31Z

Sorry I missed the notification!

It searches the dot-sourced file relative to the file where the dot-sourcing happens (relative to $PSScriptRoot).

Is that what you meant? Or are you asking if it handles . "$PSScriptRoot/SomeScript.ps1"? In that case it doesn't (yet!).

Rob Holt (rjmholt) · 2018-11-22T00:35:58Z

I think the second case. But if that's a significant work item in its own respect, I'm all for incremental improvement.

Dominic (dee-see) · 2018-11-22T01:08:11Z

Ok, I'll look into it anyway. Up to you guys to decide if you prefer having it as a second pull request or as part of this one in the meantime. :)

TylerLeonhardt

Looking good! Just the one comment 🙂

Dominic (dee-see) · 2018-11-24T13:36:29Z

Done!

I'll probably be able to handle $PSScriptRoot with a small change to FindDotSourcedVisitor. I just need to look around in the other projects (the analyzer and PowerShell itself) to see how automatic variables are handled so I don't reinvent the wheel here. If you want to wait for it, it will probably be done this weekend or the next.

Rob Holt (rjmholt) · 2018-11-24T18:25:17Z

As far as I know, automatic variables are handled like any other variables -- they come from the ExecutionContext, which will come from the PowerShellContext in this project. Usually I'd imagine our best way of performing that extraction would just be (despite the overhead of needing to create a pipeline, it handles complexities like looking up the execution stack and centralises thread handling):

object x = null;
using (var pwsh = PowerShell.Create())
{
    x = pwsh.AddScript("$variable").Invoke();
}

BUT, $PSScriptRoot is only populated during the execution of a script, so by the time we get around to parsing the script, $PSScriptRoot won't exist anymore.

The nice thing is that $PSScriptRoot is for all intents and purposes a static constant (you can re-set it, but I doubt we'd be the only thing to break if you did). So it might be easiest to just substitute it for the absolute path of the script.

Rob Holt (rjmholt) · 2018-11-28T00:27:35Z

Dominic (@dee-see) We can probably merge this as-is if you're happy? And then come back to $PSScriptRoot another time?

Dominic (dee-see) · 2018-11-28T02:33:16Z

If you don't mind I'd keep this open until the next week-end. I'll continue working on $PSScriptRoot and wrap everything up then.

Patrick Meinecke (SeeminglyScience) · 2018-11-28T04:31:54Z

BUT, $PSScriptRoot is only populated during the execution of a script, so by the time we get around to parsing the script, $PSScriptRoot won't exist anymore.

The nice thing is that $PSScriptRoot is for all intents and purposes a static constant (you can re-set it, but I doubt we'd be the only thing to break if you did). So it might be easiest to just substitute it for the absolute path of the script.

Yeah iirc it's set in the locals tuple every time a new script block invocation is started. It's set to the equivalent of something like Split-Path $MyInvocation.MyCommand.ScriptBlock.Ast.Extent.File

Rob Holt (rjmholt) · 2018-11-28T06:02:08Z

If you don't mind I'd keep this open until the next week-end. I'll continue working on $PSScriptRoot and wrap everything up then.

Okie doke. We're overdue for a release and given this spate of PRs, I want to run a build at the end of this week, so it would be a good opportunity to test and get a fix out. But I don't want to force anything — just let me know when you're ready for review again :)

Dominic (dee-see) · 2018-11-28T14:04:37Z

Ok I see! I have some free time tonight, I'll try to complete the $PSScriptRoot support. I've made quite a bit of progress last weekend so it isn't completely out of reach.

Dominic (dee-see) · 2018-11-29T01:19:47Z

Rob Holt (@rjmholt) There you go. I had a cool solution with an AstVisitor that handled situations a bit more complex than a simple "$PSScriptRoot\SomeScript.ps1", but I had trouble fitting it in the LanguageService side and I was going down an unending rabbit hole. Sooo I stepped back and only handled the most simple (and by far most common!) case.

Those .Replace don't make me very proud but they work I guess.

One cool side effect of this is that Go to definition on functions now works across files that were dot-sourced using $PSScriptRoot.

Rob Holt (rjmholt)

Looks really great Dominic (@dee-see). Left a couple of small comments

Keith Hill (rkeithhill)

LGTM

Rob Holt (rjmholt)

Re-approving 😉

Rob Holt (rjmholt) · 2018-12-02T05:53:21Z

+        private static string GetDotSourcedPath(SymbolReference symbol, Workspace workspace, ScriptFile scriptFile)
+        {
+            string cleanedUpSymbol = PathUtils.NormalizePathSeparators(symbol.SymbolName.Trim('\'', '"'));
+            return workspace.ResolveRelativeScriptPath(Path.GetDirectoryName(scriptFile.FilePath),


Might be worth factoring out the Path.GetDirectoryName(scriptFile.FilePath) as a variable here -- saves an allocation at least.

Ooop yes definitely, that was sloppy.

Rob Holt (rjmholt) · 2018-12-02T05:56:50Z

+                {
+                    path = path.Replace(variableExpressionAst.ToString(), _psScriptRoot);
+                }
+                else


Rather than have an else clause, I would negate the top condition:

if (!(nestedExpression is VariableExpressionAst variableAst && variableAst.VariablePath.UserPath.Equals("PSScriptRoot", StringComparison.OrdinalIgnoreCase))) { return null; } path = path.Replace(variableAst.ToString(), _psScriptRoot);

Rob Holt (rjmholt) · 2018-12-02T18:27:52Z

Just want TylerLeonhardt to approve before we merge

Rob Holt (rjmholt) · 2018-12-02T23:15:41Z

This may aid PowerShell/vscode-powershell#144 btw. That issue wants full intellisense I think (which would essentially require us to pre-emptively run the script or otherwise reimplement our completions), but will still help.

TylerLeonhardt

LGTM 👍 left a nit

Dominic (dee-see) requested review from TylerLeonhardt and Rob Holt (rjmholt) as code owners October 27, 2018 21:29

Implement go-to-definition for dot-sourced files

c1568fa

Fix build issues (net451 compatibility and public method comments) Fix test Make sure test is not platform-specific

Merge branch 'master' into go-to-dot-sourced

d865f3e

Rob Holt (rjmholt) reviewed Nov 8, 2018

View reviewed changes

Comment thread src/PowerShellEditorServices/Language/LanguageService.cs Outdated

Comment thread src/PowerShellEditorServices/Language/LanguageService.cs Outdated

Comment thread src/PowerShellEditorServices/Language/LanguageService.cs Outdated

Fix code review issues

a2fc548

Rob Holt (rjmholt) approved these changes Nov 13, 2018

View reviewed changes

TylerLeonhardt reviewed Nov 22, 2018

View reviewed changes

Comment thread src/PowerShellEditorServices/Utility/PathUtils.cs Outdated

TylerLeonhardt reviewed Nov 22, 2018

View reviewed changes

Replace preprocessor directive in PathUtils

c774e7c

Support $PSScriptRoot in dot-sourced files

7507fec

Rob Holt (rjmholt) reviewed Nov 30, 2018

View reviewed changes

Rob Holt (rjmholt) requested review from Patrick Meinecke (SeeminglyScience) and Keith Hill (rkeithhill) November 30, 2018 00:26

Apply review suggestions

11a3107

Keith Hill (rkeithhill) reviewed Nov 30, 2018

View reviewed changes

Comment thread src/PowerShellEditorServices/Language/FindDotSourcedVisitor.cs Outdated

Keith Hill (rkeithhill) reviewed Nov 30, 2018

View reviewed changes

Comment thread src/PowerShellEditorServices/Language/FindDotSourcedVisitor.cs Outdated

Fix review issues

5cd5061

Keith Hill (rkeithhill) approved these changes Dec 1, 2018

View reviewed changes

Rob Holt (rjmholt) approved these changes Dec 2, 2018

View reviewed changes

Apply review suggestions

0c1ea1f

TylerLeonhardt approved these changes Dec 3, 2018

View reviewed changes

Comment thread src/PowerShellEditorServices/Language/FindDotSourcedVisitor.cs Outdated

Remove unused using

49ec4a7

Rob Holt (rjmholt) merged commit 99f01ef into PowerShell:master Dec 3, 2018

Dominic (dee-see) deleted the go-to-dot-sourced branch December 3, 2018 17:29

Patrick Meinecke (SeeminglyScience) mentioned this pull request Dec 3, 2018

IntelliSense should work across dot-sourced file references PowerShell/vscode-powershell#144

Open

Conversation

Dominic (dee-see) commented Oct 27, 2018

Uh oh!

Dominic (dee-see) commented Oct 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TylerLeonhardt commented Nov 7, 2018

Uh oh!

Rob Holt (rjmholt) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Rob Holt (rjmholt) left a comment

Choose a reason for hiding this comment

Uh oh!

TylerLeonhardt commented Nov 15, 2018

Uh oh!

TylerLeonhardt commented Nov 21, 2018

Uh oh!

Dominic (dee-see) commented Nov 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rob Holt (rjmholt) commented Nov 22, 2018

Uh oh!

Dominic (dee-see) commented Nov 22, 2018

Uh oh!

Uh oh!

TylerLeonhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Dominic (dee-see) commented Nov 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rob Holt (rjmholt) commented Nov 24, 2018

Uh oh!

Rob Holt (rjmholt) commented Nov 28, 2018

Uh oh!

Dominic (dee-see) commented Nov 28, 2018

Uh oh!

Patrick Meinecke (SeeminglyScience) commented Nov 28, 2018

Uh oh!

Rob Holt (rjmholt) commented Nov 28, 2018

Uh oh!

Dominic (dee-see) commented Nov 28, 2018

Uh oh!

Dominic (dee-see) commented Nov 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rob Holt (rjmholt) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Keith Hill (rkeithhill) left a comment

Choose a reason for hiding this comment

Uh oh!

Rob Holt (rjmholt) left a comment

Choose a reason for hiding this comment

Uh oh!

Rob Holt (rjmholt) Dec 2, 2018

Choose a reason for hiding this comment

Uh oh!

Dominic (dee-see) Dec 2, 2018

Choose a reason for hiding this comment

Uh oh!

Rob Holt (rjmholt) Dec 2, 2018

Choose a reason for hiding this comment

Uh oh!

Rob Holt (rjmholt) commented Dec 2, 2018

Uh oh!

Rob Holt (rjmholt) commented Dec 2, 2018

Uh oh!

TylerLeonhardt left a comment

Dominic (dee-see) commented Oct 27, 2018 •

edited

Loading

Dominic (dee-see) commented Nov 21, 2018 •

edited

Loading

Dominic (dee-see) commented Nov 24, 2018 •

edited

Loading

Dominic (dee-see) commented Nov 29, 2018 •

edited

Loading