Remove LINQ usage in language service methods #737
Conversation
| foundSymbol, | ||
| false); | ||
|
|
||
| return |
There was a problem hiding this comment.
nit: put new FindOccurrencesResult on the same line as return?
There was a problem hiding this comment.
Yes I prefer that too
| if (funcDefnAst.Body.Extent.StartLineNumber == lineNumber - 1) | ||
| // of all the function definitions found, return the innermost function | ||
| // definition that contains `lineNumber` | ||
| foreach (FunctionDefinitionAst foundAst in foundAsts.Cast<FunctionDefinitionAst>()) |
There was a problem hiding this comment.
Do you need to use Cast? Could you just use something like as?
There was a problem hiding this comment.
I don't think as will work since it's an IEnumerable<Ast>. Even with covariant collections, every IEnumerable<Ast> is not an IEnumerable<FunctionDefintionAst>. As far as I know that means we need to use LINQ's Cast<T> or OrType<T>. Since we are expecting all the Asts returned, I chose Cast<T> in this instance. It all stems from the fact that Ast.FindAll() can't return a IEnumerable<T> where T : Ast, just IEnumerable<Ast>, which I suppose is fair enough -- the kind of limitation I constantly see in C#'s type system (trying to be clever about these types using generic constraints has only ever bitten me later on, sadly)
There was a problem hiding this comment.
The as happens on line 642.
| { | ||
| if (!_cmdletToAliasDictionary.ContainsKey(aliasInfo.Definition)) | ||
| { | ||
| _cmdletToAliasDictionary.Add(aliasInfo.Definition, new List<String>() { aliasInfo.Name }); |
There was a problem hiding this comment.
nit: don't need the () :) - actually if .Add can accept an array we can refactor this to:
new[] { aliasInfo.Name } I believe
There was a problem hiding this comment.
Personally I don't like omitting the () on the basis that () syntactically indicates a "call" and is consistent with other constructor usage, but happy to do whatever there.
Has to be a List though I think, since in the else branch we are adding to that list. The logic is basically:
if dictionary does not have entry:
add new dictionary entry with singleton list
else:
add new item to existing list
| foundDefinition.FilePath = nestedModuleFiles[index].FilePath; | ||
| } | ||
| SymbolReference foundDefinition = null; | ||
| for (int i = 0; i < nestedModuleFiles.Length; i++) |
There was a problem hiding this comment.
did this need to be a for instead of a foreach?
There was a problem hiding this comment.
I think you're right. I was changing from a while, but was probably too conservative
|
Ooooh, I think the tests have actually picked up a breakage here! |
|
Yay I fixed it! The fact that I fixed it makes me happy, but the fact that testing actually found it makes me much much happier! |
| @@ -55,14 +64,14 @@ public class LanguageService | |||
| PowerShellContext powerShellContext, | |||
| ILogger logger) | |||
| { | |||
| Validate.IsNotNull("powerShellContext", powerShellContext); | |||
| Validate.IsNotNull(nameof(powerShellContext), powerShellContext); | |||
| @@ -426,8 +437,8 @@ public FindOccurrencesResult FindSymbolsInFile(ScriptFile scriptFile) | |||
| if (foundDefinition == null) | |||
| { | |||
| // Get a list of all powershell files in the workspace path | |||
| var allFiles = workspace.EnumeratePSFiles(); | |||
| foreach (var file in allFiles) | |||
| IEnumerable<string> allFiles = workspace.EnumeratePSFiles(); | |||
| { | ||
| // For some commands there are no paramsets (like applications). Until | ||
| // the valid command types are better understood, catch this exception | ||
| // which gets raised when there are no ParameterSets for the command type. |
There was a problem hiding this comment.
I wonder if we should log this as a diagnostic level message - we expect this exception in some cases but just in case we're getting this for cases we didn't expect, it might be good to have a log entry when log level set to Diagnostic.
| { | ||
| return ast is StatementAst && ast.Extent.Contains(lineNumber, columnNumber); | ||
| }, true); | ||
|
|
||
| // Find ast with the smallest extent |
There was a problem hiding this comment.
Would be nice not to lose this comment.
Removes most LINQ calls in the language service methods.
There are also some style changes. They should be contained to commits marked
[Style].