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 codelens crash when a file does not exist #732

Merged
merged 4 commits into from
Aug 28, 2018
Merged

Fix codelens crash when a file does not exist #732

Changes from 3 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,9 +8,11 @@
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;
using System.Runtime.InteropServices;
using System.Threading;
using System.Threading.Tasks;

Expand Down Expand Up @@ -294,72 +296,83 @@ 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
// TODO: File system case-sensitivity is based on filesystem not OS, but OS is a much cheaper heuristic
var fileMap = RuntimeInformation.IsOSPlatform(OSPlatform.Linux)
? new OrderedDictionary()
: new OrderedDictionary(StringComparer.OrdinalIgnoreCase);
foreach (ScriptFile file in referencedFiles)
{
fileMap.Add(file.FilePath, file);
}

foreach (string file in workspace.EnumeratePSFiles())
{
if (!fileMap.Contains(file))
{
if (!fileMap.Contains(file))
ScriptFile scriptFile;
try
{
fileMap.Add(file, new ScriptFile(file, null, this.powerShellContext.LocalPowerShellVersion.Version));
scriptFile = workspace.GetFile(file);
Copy link
Member

Choose a reason for hiding this comment

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

this is the change that will increase perf, yeah @SeeminglyScience?

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 that constructor for ScriptFile (which is now unused, but is public, so...) does a call to File.ReadAllText() -- so it always reads off the file system. I'm always averse to something like touching the filesystem in a constructor (and don't really understand why it's done in this constructor, since there's another one that takes a full string), but the real problem is that it's likely we already have the script file contents loaded, and if we don't, this will cache them too

}
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