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

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.

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

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

rjmholt

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

src/PowerShellEditorServices/Language/LanguageService.cs

rjmholt

This looks good to me. Thanks @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 @dee-see :)

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!).

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.

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. :)

src/PowerShellEditorServices/Utility/PathUtils.cs

TylerLeonhardt

Looking good! Just the one comment 🙂

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.

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.

rjmholt · 2018-11-28T00:27:35Z

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

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.

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

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 :)

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.

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

@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.

rjmholt

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

src/PowerShellEditorServices/Language/AstOperations.cs

src/PowerShellEditorServices/Language/FindDotSourcedVisitor.cs

src/PowerShellEditorServices/Language/LanguageService.cs

src/PowerShellEditorServices/Workspace/ScriptFile.cs

src/PowerShellEditorServices/Language/FindDotSourcedVisitor.cs

rkeithhill

LGTM

rjmholt

Re-approving 😉

rjmholt · 2018-12-02T05:53:21Z

src/PowerShellEditorServices/Language/LanguageService.cs

+        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.

rjmholt · 2018-12-02T05:56:50Z

src/PowerShellEditorServices/Language/FindDotSourcedVisitor.cs

+                {
+                    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);

rjmholt · 2018-12-02T18:27:52Z

Just want @TylerLeonhardt to approve before we merge

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

src/PowerShellEditorServices/Language/FindDotSourcedVisitor.cs

dee-see requested review from rjmholt and TylerLeonhardt 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

rjmholt reviewed Nov 8, 2018

View reviewed changes

src/PowerShellEditorServices/Language/LanguageService.cs Outdated Show resolved Hide resolved

src/PowerShellEditorServices/Language/LanguageService.cs Outdated Show resolved Hide resolved

src/PowerShellEditorServices/Language/LanguageService.cs Outdated Show resolved Hide resolved

Fix code review issues

a2fc548

rjmholt approved these changes Nov 13, 2018

View reviewed changes

TylerLeonhardt reviewed Nov 22, 2018

View reviewed changes

src/PowerShellEditorServices/Utility/PathUtils.cs Outdated Show resolved Hide resolved

TylerLeonhardt reviewed Nov 22, 2018

View reviewed changes

Replace preprocessor directive in PathUtils

c774e7c

Support $PSScriptRoot in dot-sourced files

7507fec

rjmholt reviewed Nov 30, 2018

View reviewed changes

rjmholt requested review from SeeminglyScience and rkeithhill November 30, 2018 00:26

Apply review suggestions

11a3107

rkeithhill reviewed Nov 30, 2018

View reviewed changes

src/PowerShellEditorServices/Language/FindDotSourcedVisitor.cs Outdated Show resolved Hide resolved

rkeithhill reviewed Nov 30, 2018

View reviewed changes

src/PowerShellEditorServices/Language/FindDotSourcedVisitor.cs Outdated Show resolved Hide resolved

Fix review issues

5cd5061

rkeithhill approved these changes Dec 1, 2018

View reviewed changes

rjmholt approved these changes Dec 2, 2018

View reviewed changes

Apply review suggestions

0c1ea1f

TylerLeonhardt approved these changes Dec 3, 2018

View reviewed changes

src/PowerShellEditorServices/Language/FindDotSourcedVisitor.cs Outdated Show resolved Hide resolved

Remove unused using

49ec4a7

rjmholt merged commit 99f01ef into PowerShell:master Dec 3, 2018

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

SeeminglyScience mentioned this pull request Dec 3, 2018

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dee-see commented Oct 27, 2018

dee-see commented Oct 27, 2018 •

edited

Loading

TylerLeonhardt commented Nov 7, 2018

rjmholt left a comment

rjmholt left a comment

TylerLeonhardt commented Nov 15, 2018

TylerLeonhardt commented Nov 21, 2018

dee-see commented Nov 21, 2018 •

edited

Loading

rjmholt commented Nov 22, 2018

dee-see commented Nov 22, 2018

TylerLeonhardt left a comment

dee-see commented Nov 24, 2018 •

edited

Loading

rjmholt commented Nov 24, 2018

rjmholt commented Nov 28, 2018

dee-see commented Nov 28, 2018

SeeminglyScience commented Nov 28, 2018

rjmholt commented Nov 28, 2018

dee-see commented Nov 28, 2018

dee-see commented Nov 29, 2018 •

edited

Loading

rjmholt left a comment

rkeithhill left a comment

rjmholt left a comment

rjmholt Dec 2, 2018

dee-see Dec 2, 2018

rjmholt Dec 2, 2018

rjmholt commented Dec 2, 2018

rjmholt commented Dec 2, 2018

TylerLeonhardt left a comment

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

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

Conversation

dee-see commented Oct 27, 2018

dee-see commented Oct 27, 2018 • edited Loading

TylerLeonhardt commented Nov 7, 2018

rjmholt left a comment

Choose a reason for hiding this comment

rjmholt left a comment

Choose a reason for hiding this comment

TylerLeonhardt commented Nov 15, 2018

TylerLeonhardt commented Nov 21, 2018

dee-see commented Nov 21, 2018 • edited Loading

rjmholt commented Nov 22, 2018

dee-see commented Nov 22, 2018

TylerLeonhardt left a comment

Choose a reason for hiding this comment

dee-see commented Nov 24, 2018 • edited Loading

rjmholt commented Nov 24, 2018

rjmholt commented Nov 28, 2018

dee-see commented Nov 28, 2018

SeeminglyScience commented Nov 28, 2018

rjmholt commented Nov 28, 2018

dee-see commented Nov 28, 2018

dee-see commented Nov 29, 2018 • edited Loading

rjmholt left a comment

Choose a reason for hiding this comment

rkeithhill left a comment

Choose a reason for hiding this comment

rjmholt left a comment

Choose a reason for hiding this comment

rjmholt Dec 2, 2018

Choose a reason for hiding this comment

dee-see Dec 2, 2018

Choose a reason for hiding this comment

rjmholt Dec 2, 2018

Choose a reason for hiding this comment

rjmholt commented Dec 2, 2018

rjmholt commented Dec 2, 2018

TylerLeonhardt left a comment

Choose a reason for hiding this comment

dee-see commented Oct 27, 2018 •

edited

Loading

dee-see commented Nov 21, 2018 •

edited

Loading

dee-see commented Nov 24, 2018 •

edited

Loading

dee-see commented Nov 29, 2018 •

edited

Loading