Skip to content
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.

Sign up for GitHub

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

Fix crash when setBreakpoint from VSCode sends a git:/ URI... #1000

Merged
merged 2 commits into from
Aug 19, 2019
Merged

Fix crash when setBreakpoint from VSCode sends a git:/ URI... #1000

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
34c7692
Fix git uri problem
Aug 1, 2019
a90e8e2
Remove unused imports
Aug 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion src/PowerShellEditorServices/Workspace/Workspace.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
using System.Text;
using System.Runtime.InteropServices;
using Microsoft.Extensions.FileSystemGlobbing;
using Microsoft.Extensions.FileSystemGlobbing.Abstractions;

namespace Microsoft.PowerShell.EditorServices
{
Expand Down Expand Up @@ -49,6 +48,13 @@ public class Workspace
"**/*"
};

private static readonly string[] s_supportedUriSchemes = new[]
{
"file",
"untitled",
"inmemory"
};

private ILogger logger;
private Version powerShellVersion;
private Dictionary<string, ScriptFile> workspaceFiles = new Dictionary<string, ScriptFile>();
Expand Down Expand Up @@ -174,6 +180,20 @@ public ScriptFile GetFile(string filePath)
/// <param name="scriptFile">The out parameter that will contain the ScriptFile object.</param>
public bool TryGetFile(string filePath, out ScriptFile scriptFile)
{
try
{
if (filePath.Contains(":/") // Quick heuristic to determine if we might have a URI
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I had suggested a code change like this before but we eventually decided that the GetFile() call on line 199 would throw in this case and we'd return false. That said, I'd rather test this in a way that doesn't rely on catching an exception.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @rkeithhill, are you for this change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My worry is that there seems to be a test that exists already for a git:/ schemed file. My change would just reject those (and anything else that isn't file, untitled or inmemory). The fact that we already have a test for this makes me feel like there is an expectation that we can understand and handle a URI like this -- does that seem right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those tests for git:/ and gitlens-git:/ look like my tests. I think the intent here was to have those uri schemes return true for IsInMemory which we used to signal that you can't do things like set breakpoints with an "in memory" document. In retrospect, perhaps that is not the best approach RE the way we used IsInMemory but the basic idea was to determine whether or not we had a file path we could provide to Set-PSBreakpoint.

Copy link
Contributor

@rkeithhill rkeithhill Aug 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In an ideal design, I could walk up to the ScriptFile/EditorDocument and query a property like CanHaveBreakpoints or something like that to determine whether or not an LSP text document could have a breakpoint set on it. But I think this is good as a stop-gap. Ship it! :shipit:

&& !s_supportedUriSchemes.Contains(new Uri(filePath).Scheme))
{
scriptFile = null;
return false;
}
}
catch
{
// If something goes wrong trying to check for URIs, just proceed to normal logic
}

try
{
scriptFile = GetFile(filePath);
Expand Down