Fix FileNotFoundEx crash when Fold happens on untitled: scheme doc

[rkeithhill]

Fix PowerShell/vscode-powershell#1676

One change here that makes me a bit nervous is the added IsPathInMemory() test to the GetFile() method. However, I observed GetFile() crashing when it was passed an untitled: path as part of the repro for vscode 1676:

    System.AggregateException: One or more errors occurred. (Could not find file 'C:\Users\Keith\GitHub\PowerShell\Plaster\untitled:Untitled-2'.) ---> System.IO.FileNotFoundException: Could not find file 'C:\Users\Keith\GitHub\PowerShell\Plaster\untitled:Untitled-2'.
       at System.IO.FileStream.ValidateFileHandle(SafeFileHandle fileHandle)
       at System.IO.FileStream.CreateFileOpenHandle(FileMode mode, FileShare share, FileOptions options)
       at System.IO.FileStream..ctor(String path, FileMode mode, FileAccess access, FileShare share, Int32 bufferSize, FileOptions options)
       at System.IO.FileStream..ctor(String path, FileMode mode, FileAccess access)
       at Microsoft.PowerShell.EditorServices.Workspace.GetFile(String filePath) in C:\Users\Keith\GitHub\rkeithhill\PowerShellEditorServices\src\PowerShellEditorServices\Workspace\Workspace.cs:line 118

So it seems prudent to ensure we never try to create a FileStream given an in-memory document uri. However, the IsPathInMemory() method utilizes exceptions in some cases to determine if the path is in-memory. That is why I put it second in the short-circuiting &&. So we would only do this test at the point we would actually try to open the file.

Another option would be to do the try/catch FileNotFoundEx in this method and then we could skip the IsPathInMemory() test. Thoughts?

[glennsarti]

+1

[SeeminglyScience]

LGTM! :)

Another option would be to do the try/catch FileNotFoundEx in this method and then we could skip the IsPathInMemory() test. Thoughts?

I think it makes sense where you have it. As you said we should never try to create a file stream for in memory files, so adding that protection makes sense to me. I'd like to see IsPathInMemory changed to use a mix of Path.IsPathFullyQualified and Uri.TryCreate to make the call a little cheaper, but it's probably not a huge difference.

[TylerLeonhardt]

I recently added a TryGetFile function that would do the soft check:

PowerShellEditorServices/src/PowerShellEditorServices/Workspace/Workspace.cs

Line 142 in 99f01ef

public bool TryGetFile(string filePath, out ScriptFile scriptFile)

maybe that's better to use here?

[rkeithhill]

We could (perhaps should) change the call site but there are 23 references to GetFile() so maybe we should still try to make GetFile() a little more robust? Either that OR we nuke make GetFile() private and replace all 23 references with calls to TryGetFile(). Longer term, that may be the better strategy because it makes it clearer you may not get a file back. GetFile() behaves similarly except that it returns a null when there is no file to return - which is way less obvious and easier to ignore the null case - kind of like ignoring error return codes in C code. :-)

[TylerLeonhardt]

Actually I don't think GetFile() handle's File exceptions:

PowerShellEditorServices/src/PowerShellEditorServices/Workspace/Workspace.cs

Lines 118 to 119 in 99f01ef

	using (FileStream fileStream = new FileStream(resolvedFilePath, FileMode.Open, FileAccess.Read))
	using (StreamReader streamReader = new StreamReader(fileStream, Encoding.UTF8))

If FileStream or StreamReader throws an exception, that will bubble up I'm pretty sure, no?

Where as TryGetFile actually does catch them.

I think we should eventually move to using TryGetFile everywhere it makes sense... but we can call that out of scope for this PR if you'd like.

[rkeithhill]

I'll make the change to use TryGetFile() later tonight. Getting ready to head out to see Aquaman right now. :-)

[TylerLeonhardt]

Have a great time! I want to hear the full @rkeithhill review 😄

[rkeithhill]

OK, updated to use TryGetFile() instead - reverted change to GetFile(). Although I did an "important" note on GetFile() that shows up in Intellisense that recommends TryGetFile() instead. Also, fix an incorrect (copy/paste) log message.

P.S. Aquaman was pretty good. Not as good as Wonder Woman IMO but way better than Dawn of Pustice. Then again, I've been a Jason Momoa fan since Stargate Atlantis. :-)

[TylerLeonhardt]

LGTM :) potential nit in comments