Skip to content

Fix codelens crash when a file does not exist #732

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

Merged
merged 4 commits into from
Aug 28, 2018
Merged

Fix codelens crash when a file does not exist #732

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5ac8362
[Style] Refactor FindReferencesOfSymbol code
Aug 23, 2018
700d4f6
Check that file exists before parsing for symbols
Aug 23, 2018
5ba4f30
Fix unneccessary file read for workspace script files
rjmholt Aug 24, 2018
b5ad8cd
Fix unsupported API usage in .NET Framework
rjmholt Aug 28, 2018
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
121 changes: 67 additions & 54 deletions src/PowerShellEditorServices/Language/LanguageService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System;
using System.Collections.Generic;
using System.Collections.Specialized;
using System.IO;
using System.Linq;
using System.Management.Automation;
using System.Management.Automation.Language;
Expand Down Expand Up @@ -294,72 +295,84 @@ public async Task<FindReferencesResult> FindReferencesOfSymbol(
ScriptFile[] referencedFiles,
Workspace workspace)
{
if (foundSymbol != null)
if (foundSymbol == null)
{
int symbolOffset = referencedFiles[0].GetOffsetAtPosition(
foundSymbol.ScriptRegion.StartLineNumber,
foundSymbol.ScriptRegion.StartColumnNumber);
return null;
}

// Make sure aliases have been loaded
await GetAliases();
int symbolOffset = referencedFiles[0].GetOffsetAtPosition(
foundSymbol.ScriptRegion.StartLineNumber,
foundSymbol.ScriptRegion.StartColumnNumber);

// We want to look for references first in referenced files, hence we use ordered dictionary
var fileMap = new OrderedDictionary(StringComparer.OrdinalIgnoreCase);
foreach (ScriptFile file in referencedFiles)
{
fileMap.Add(file.FilePath, file);
}
// Make sure aliases have been loaded
await GetAliases();

var allFiles = workspace.EnumeratePSFiles();
foreach (var file in allFiles)
// We want to look for references first in referenced files, hence we use ordered dictionary
var fileMap = new OrderedDictionary(StringComparer.OrdinalIgnoreCase);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this is existing code, but wouldn't this be an issue on Linux?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this seems suspect...

foreach (ScriptFile file in referencedFiles)
{
fileMap.Add(file.FilePath, file);
}

IEnumerable<string> allFiles = workspace.EnumeratePSFiles();
foreach (string file in allFiles)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Possibly inline workspace.EnumeratePSFiles() if allFiles is not used somewhere down the line.

{
if (!fileMap.Contains(file))
{
if (!fileMap.Contains(file))
ScriptFile scriptFile;
try
{
fileMap.Add(file, new ScriptFile(file, null, this.powerShellContext.LocalPowerShellVersion.Version));
scriptFile = new ScriptFile(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh? This doesn't try to pull an existing ScriptFile instance? That explains why find references code lens is slow...

Don't know if you want to try this in this PR, but if this could be replaced with Workspace.GetFile() it would speed up finding references significantly. Right now it's only looking at the cache for files that are explicitly referenced (which is pretty rare)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was a bit skeptical of that constructor. I'll see if changing it over works

file,
clientFilePath: null,
powerShellVersion: this.powerShellContext.LocalPowerShellVersion.Version);
}
catch (IOException)
{
// If the file has ceased to exist for some reason, we just skip it
continue;
}

fileMap.Add(file, scriptFile);
}
}

List<SymbolReference> symbolReferences = new List<SymbolReference>();
foreach (var fileName in fileMap.Keys)
{
var file = (ScriptFile)fileMap[fileName];
IEnumerable<SymbolReference> symbolReferencesinFile =
AstOperations
.FindReferencesOfSymbol(
file.ScriptAst,
foundSymbol,
CmdletToAliasDictionary,
AliasToCmdletDictionary)
.Select(
reference =>
var symbolReferences = new List<SymbolReference>();
foreach (object fileName in fileMap.Keys)
{
var file = (ScriptFile)fileMap[fileName];
IEnumerable<SymbolReference> symbolReferencesinFile =
AstOperations
.FindReferencesOfSymbol(
file.ScriptAst,
foundSymbol,
CmdletToAliasDictionary,
AliasToCmdletDictionary)
.Select(reference =>
Copy link
Member

Choose a reason for hiding this comment

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

did you want to try to clean up the LINQ usage here? We can call that out of scope if you want.

Copy link
Contributor Author

@rjmholt rjmholt Aug 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I'm game!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remember when I hit this before I went and searched if we can avoid the closure allocation by turning this into a static (or instance...) method call, but I didn't find any good information on it... Would be nice if we could retain the readable composability of LINQ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I'll open a new PR on the LINQ refactoring here, since there are other methods that would benefit. I'll see if I can do a quick benchmark before we merge

{
try
{
try
{
reference.SourceLine =
file.GetLine(reference.ScriptRegion.StartLineNumber);
}
catch (ArgumentOutOfRangeException e)
{
reference.SourceLine = string.Empty;
this.logger.WriteException("Found reference is out of range in script file", e);
}

reference.FilePath = file.FilePath;
return reference;
});

symbolReferences.AddRange(symbolReferencesinFile);
}
reference.SourceLine = file.GetLine(reference.ScriptRegion.StartLineNumber);
}
catch (ArgumentOutOfRangeException e)
{
reference.SourceLine = string.Empty;
this.logger.WriteException("Found reference is out of range in script file", e);
}

return
new FindReferencesResult
{
SymbolFileOffset = symbolOffset,
SymbolName = foundSymbol.SymbolName,
FoundReferences = symbolReferences
};
reference.FilePath = file.FilePath;
return reference;
});

symbolReferences.AddRange(symbolReferencesinFile);
}
else { return null; }

return new FindReferencesResult
{
SymbolFileOffset = symbolOffset,
SymbolName = foundSymbol.SymbolName,
FoundReferences = symbolReferences
};
}

/// <summary>
Expand Down