From 5ef79aca004fe5c0afa28e2b3fd1c848bf37c34a Mon Sep 17 00:00:00 2001 From: Staffan Gustafsson Date: Wed, 13 May 2020 15:13:49 +0200 Subject: [PATCH 1/3] Nullable annotations for CommandSearcher --- .../engine/CommandPathSearch.cs | 9 +- .../engine/CommandSearcher.cs | 158 +++++++++--------- .../engine/MshSnapinQualifiedName.cs | 10 +- 3 files changed, 86 insertions(+), 91 deletions(-) diff --git a/src/System.Management.Automation/engine/CommandPathSearch.cs b/src/System.Management.Automation/engine/CommandPathSearch.cs index 415736b8561..7ba6476e26f 100644 --- a/src/System.Management.Automation/engine/CommandPathSearch.cs +++ b/src/System.Management.Automation/engine/CommandPathSearch.cs @@ -1,14 +1,14 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +#nullable enable + using System.Collections; using System.Collections.Generic; using System.Collections.ObjectModel; using System.IO; using System.Linq; -#nullable enable - namespace System.Management.Automation { /// @@ -18,11 +18,12 @@ namespace System.Management.Automation internal class CommandPathSearch : IEnumerable, IEnumerator { [TraceSource("CommandSearch", "CommandSearch")] - private static PSTraceSource s_tracer = PSTraceSource.GetTracer("CommandSearch", "CommandSearch"); + private static readonly PSTraceSource s_tracer = PSTraceSource.GetTracer("CommandSearch", "CommandSearch"); /// /// Constructs a command searching enumerator that resolves the location /// of a command using the PATH environment variable. + /// of a command using the PATH environment variable. /// /// /// The command name to search for in the path. @@ -44,7 +45,7 @@ internal CommandPathSearch( string commandName, LookupPathCollection lookupPaths, ExecutionContext context, - Collection acceptableCommandNames, + Collection? acceptableCommandNames, bool useFuzzyMatch) { _useFuzzyMatch = useFuzzyMatch; diff --git a/src/System.Management.Automation/engine/CommandSearcher.cs b/src/System.Management.Automation/engine/CommandSearcher.cs index cb258dfcf06..a7ed3bd63b7 100644 --- a/src/System.Management.Automation/engine/CommandSearcher.cs +++ b/src/System.Management.Automation/engine/CommandSearcher.cs @@ -1,9 +1,12 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +#nullable enable + using System.Collections; using System.Collections.Generic; using System.Collections.ObjectModel; +using System.Diagnostics.CodeAnalysis; using System.IO; using System.Management.Automation.Internal; @@ -242,9 +245,9 @@ public bool MoveNext() return false; } - private CommandInfo SearchForAliases() + private CommandInfo? SearchForAliases() { - CommandInfo currentMatch = null; + CommandInfo? currentMatch = null; if (_context.EngineSessionState != null && (_commandTypes & CommandTypes.Alias) != 0) @@ -255,9 +258,9 @@ private CommandInfo SearchForAliases() return currentMatch; } - private CommandInfo SearchForFunctions() + private CommandInfo? SearchForFunctions() { - CommandInfo currentMatch = null; + CommandInfo? currentMatch = null; if (_context.EngineSessionState != null && (_commandTypes & (CommandTypes.Function | CommandTypes.Filter | CommandTypes.Configuration)) != 0) @@ -268,9 +271,9 @@ private CommandInfo SearchForFunctions() return currentMatch; } - private CommandInfo SearchForCmdlets() + private CommandInfo? SearchForCmdlets() { - CommandInfo currentMatch = null; + CommandInfo? currentMatch = null; if ((_commandTypes & CommandTypes.Cmdlet) != 0) { @@ -280,9 +283,9 @@ private CommandInfo SearchForCmdlets() return currentMatch; } - private CommandInfo ProcessBuiltinScriptState() + private CommandInfo? ProcessBuiltinScriptState() { - CommandInfo currentMatch = null; + CommandInfo? currentMatch = null; // Check to see if the path is qualified @@ -296,9 +299,9 @@ private CommandInfo ProcessBuiltinScriptState() return currentMatch; } - private CommandInfo ProcessPathResolutionState() + private CommandInfo? ProcessPathResolutionState() { - CommandInfo currentMatch = null; + CommandInfo? currentMatch = null; try { @@ -338,7 +341,7 @@ private CommandInfo ProcessPathResolutionState() return currentMatch; } - private CommandInfo ProcessQualifiedFileSystemState() + private CommandInfo? ProcessQualifiedFileSystemState() { try { @@ -355,13 +358,13 @@ private CommandInfo ProcessQualifiedFileSystemState() throw; } - CommandInfo currentMatch = null; + CommandInfo? currentMatch = null; _currentState = SearchState.PathSearch; if (_canDoPathLookup) { try { - while (currentMatch == null && _pathSearcher.MoveNext()) + while (currentMatch == null && _pathSearcher!.MoveNext()) { currentMatch = GetInfoFromPath(((IEnumerator)_pathSearcher).Current); } @@ -375,10 +378,10 @@ private CommandInfo ProcessQualifiedFileSystemState() return currentMatch; } - private CommandInfo ProcessPathSearchState() + private CommandInfo? ProcessPathSearchState() { - CommandInfo currentMatch = null; - string path = DoPowerShellRelativePathLookup(); + CommandInfo? currentMatch = null; + string? path = DoPowerShellRelativePathLookup(); if (!string.IsNullOrEmpty(path)) { @@ -443,9 +446,9 @@ public void Dispose() /// /// A CommandInfo for the next command if it exists as a path, or null otherwise. /// - private CommandInfo GetNextFromPath() + private CommandInfo? GetNextFromPath() { - CommandInfo result = null; + CommandInfo? result = null; do // false loop { @@ -478,7 +481,7 @@ private CommandInfo GetNextFromPath() if (!_commandResolutionOptions.HasFlag(SearchResolutionOptions.ResolveLiteralThenPathPatterns) && resolvedPaths.Count == 0) { - string path = GetNextLiteralPathThatExistsAndHandleExceptions(_commandName, out _); + string? path = GetNextLiteralPathThatExistsAndHandleExceptions(_commandName, out _); if (path != null) { @@ -520,7 +523,7 @@ private CommandInfo GetNextFromPath() /// /// A collection of full paths to the commands which were found. /// - private Collection GetNextFromPathUsingWildcards(string command, out ProviderInfo provider) + private Collection GetNextFromPathUsingWildcards(string? command, out ProviderInfo? provider) { try { @@ -594,9 +597,9 @@ private static bool checkPath(string path, string commandName) /// If refers to a cmdlet file that /// contains invalid metadata. /// - private CommandInfo GetInfoFromPath(string path) + private CommandInfo? GetInfoFromPath(string path) { - CommandInfo result = null; + CommandInfo? result = null; do // false loop { @@ -607,7 +610,7 @@ private CommandInfo GetInfoFromPath(string path) } // Now create the appropriate CommandInfo using the extension - string extension = null; + string? extension = null; try { @@ -681,9 +684,9 @@ private CommandInfo GetInfoFromPath(string path) /// /// A CommandInfo representing the next matching alias if found, otherwise null. /// - private CommandInfo GetNextAlias() + private CommandInfo? GetNextAlias() { - CommandInfo result = null; + CommandInfo? result = null; if ((_commandResolutionOptions & SearchResolutionOptions.ResolveAliasPatterns) != 0) { @@ -709,7 +712,7 @@ private CommandInfo GetNextAlias() } // Process alias from modules - AliasInfo c = GetAliasFromModules(_commandName); + AliasInfo? c = GetAliasFromModules(_commandName); if (c != null) { matchingAliases.Add(c); @@ -762,15 +765,15 @@ private CommandInfo GetNextAlias() /// /// A CommandInfo representing the next matching function if found, otherwise null. /// - private CommandInfo GetNextFunction() + private CommandInfo? GetNextFunction() { - CommandInfo result = null; + CommandInfo? result = null; if (_commandResolutionOptions.HasFlag(SearchResolutionOptions.ResolveFunctionPatterns)) { if (_matchingFunctionEnumerator == null) { - Collection matchingFunction = new Collection(); + Collection matchingFunction = new Collection(); // Generate the enumerator of matching function names WildcardPattern functionMatcher = @@ -796,7 +799,7 @@ private CommandInfo GetNextFunction() } // Process functions from modules - CommandInfo cmdInfo = GetFunctionFromModules(_commandName); + CommandInfo? cmdInfo = GetFunctionFromModules(_commandName); if (cmdInfo != null) { matchingFunction.Add(cmdInfo); @@ -838,7 +841,7 @@ private CommandInfo GetNextFunction() // Don't return commands to the user if that might result in: // - Trusted commands calling untrusted functions that the user has overridden // - Debug prompts calling internal functions that are likely to have code injection - private bool ShouldSkipCommandResolutionForConstrainedLanguage(CommandInfo result, ExecutionContext executionContext) + private bool ShouldSkipCommandResolutionForConstrainedLanguage(CommandInfo? result, ExecutionContext executionContext) { if (result == null) { @@ -870,70 +873,61 @@ private bool ShouldSkipCommandResolutionForConstrainedLanguage(CommandInfo resul return false; } - private AliasInfo GetAliasFromModules(string command) + private AliasInfo? GetAliasFromModules(string command) { - AliasInfo result = null; + AliasInfo? result = null; if (command.IndexOf('\\') > 0) { // See if it's a module qualified alias... - PSSnapinQualifiedName qualifiedName = PSSnapinQualifiedName.GetInstance(command); + PSSnapinQualifiedName? qualifiedName = PSSnapinQualifiedName.GetInstance(command); if (qualifiedName != null && !string.IsNullOrEmpty(qualifiedName.PSSnapInName)) { - PSModuleInfo module = GetImportedModuleByName(qualifiedName.PSSnapInName); + PSModuleInfo? module = GetImportedModuleByName(qualifiedName.PSSnapInName); - if (module != null) - { - module.ExportedAliases.TryGetValue(qualifiedName.ShortName, out result); - } + module?.ExportedAliases.TryGetValue(qualifiedName.ShortName, out result); } } return result; } - private CommandInfo GetFunctionFromModules(string command) + private CommandInfo? GetFunctionFromModules(string command) { - FunctionInfo result = null; + FunctionInfo? result = null; if (command.IndexOf('\\') > 0) { // See if it's a module qualified function call... - PSSnapinQualifiedName qualifiedName = PSSnapinQualifiedName.GetInstance(command); + PSSnapinQualifiedName? qualifiedName = PSSnapinQualifiedName.GetInstance(command); if (qualifiedName != null && !string.IsNullOrEmpty(qualifiedName.PSSnapInName)) { - PSModuleInfo module = GetImportedModuleByName(qualifiedName.PSSnapInName); + PSModuleInfo? module = GetImportedModuleByName(qualifiedName.PSSnapInName); - if (module != null) - { - module.ExportedFunctions.TryGetValue(qualifiedName.ShortName, out result); - } + module?.ExportedFunctions.TryGetValue(qualifiedName.ShortName, out result); } } return result; } - private PSModuleInfo GetImportedModuleByName(string moduleName) + private PSModuleInfo? GetImportedModuleByName(string moduleName) { - PSModuleInfo module = null; + PSModuleInfo? module = null; List modules = _context.Modules.GetModules(new string[] { moduleName }, false); if (modules != null && modules.Count > 0) { foreach (PSModuleInfo m in modules) { - if (_context.previousModuleImported.ContainsKey(m.Name) && ((string)_context.previousModuleImported[m.Name] == m.Path)) + if (_context.previousModuleImported.ContainsKey(m.Name) && ((string?)_context.previousModuleImported[m.Name] == m.Path)) { module = m; break; } } - if (module == null) - { - module = modules[0]; - } + module ??= modules[0]; } return module; @@ -949,9 +943,9 @@ private PSModuleInfo GetImportedModuleByName(string moduleName) /// A FunctionInfo if the function name exists and is a function, a FilterInfo if /// the filter name exists and is a filter, or null otherwise. /// - private CommandInfo GetFunction(string function) + private CommandInfo? GetFunction(string function) { - CommandInfo result = _context.EngineSessionState.GetFunction(function); + CommandInfo? result = _context.EngineSessionState.GetFunction(function); if (result != null) { @@ -991,9 +985,9 @@ private CommandInfo GetFunction(string function) /// A CmdletInfo for the next matching Cmdlet or null if there are /// no more matches. /// - private CmdletInfo GetNextCmdlet() + private CmdletInfo? GetNextCmdlet() { - CmdletInfo result = null; + CmdletInfo? result = null; bool useAbbreviationExpansion = _commandResolutionOptions.HasFlag(SearchResolutionOptions.UseAbbreviationExpansion); if (_matchingCmdlet == null) @@ -1002,7 +996,7 @@ private CmdletInfo GetNextCmdlet() { Collection matchingCmdletInfo = new Collection(); - PSSnapinQualifiedName PSSnapinQualifiedCommandName = + PSSnapinQualifiedName? PSSnapinQualifiedCommandName = PSSnapinQualifiedName.GetInstance(_commandName); if (!useAbbreviationExpansion && PSSnapinQualifiedCommandName == null) @@ -1010,10 +1004,10 @@ private CmdletInfo GetNextCmdlet() return null; } - string moduleName = PSSnapinQualifiedCommandName?.PSSnapInName; + string? moduleName = PSSnapinQualifiedCommandName?.PSSnapInName; var cmdletShortName = PSSnapinQualifiedCommandName?.ShortName; - WildcardPattern cmdletMatcher = cmdletShortName != null + WildcardPattern? cmdletMatcher = cmdletShortName != null ? WildcardPattern.Get(cmdletShortName, WildcardOptions.IgnoreCase) : null; @@ -1068,9 +1062,10 @@ private CmdletInfo GetNextCmdlet() return traceResult(result); } - private IEnumerator _matchingCmdlet; + private IEnumerator? _matchingCmdlet; - private static CmdletInfo traceResult(CmdletInfo result) + [return: NotNullIfNotNull("result")] + private static CmdletInfo? traceResult(CmdletInfo? result) { if (result != null) { @@ -1083,9 +1078,9 @@ private static CmdletInfo traceResult(CmdletInfo result) return result; } - private string DoPowerShellRelativePathLookup() + private string? DoPowerShellRelativePathLookup() { - string result = null; + string? result = null; if (_context.EngineSessionState != null && _context.EngineSessionState.ProviderCount > 0) @@ -1125,14 +1120,14 @@ private string DoPowerShellRelativePathLookup() /// The path that was resolved. Null if the path couldn't be resolved or was /// not resolved by the FileSystemProvider. /// - private string ResolvePSPath(string path) + private string? ResolvePSPath(string? path) { - string result = null; + string? result = null; try { - ProviderInfo provider = null; - string resolvedPath = null; + ProviderInfo? provider = null; + string? resolvedPath = null; // Try literal path resolution if it is set to run first if (_commandResolutionOptions.HasFlag(SearchResolutionOptions.ResolveLiteralThenPathPatterns)) @@ -1235,7 +1230,7 @@ private string ResolvePSPath(string path) /// /// Full path to the command. /// - private string GetNextLiteralPathThatExistsAndHandleExceptions(string command, out ProviderInfo provider) + private string? GetNextLiteralPathThatExistsAndHandleExceptions(string command, out ProviderInfo? provider) { try { @@ -1297,7 +1292,7 @@ private string GetNextLiteralPathThatExistsAndHandleExceptions(string command, o /// /// Full path to the command. /// - private string GetNextLiteralPathThatExists(string command, out ProviderInfo provider) + private string? GetNextLiteralPathThatExists(string? command, out ProviderInfo? provider) { string resolvedPath = _context.LocationGlobber.GetProviderPath(command, out provider); @@ -1484,7 +1479,7 @@ private static CanDoPathLookupResult CanDoPathLookup(string possiblePath) /// /// Determines which command types will be globbed. /// - private SearchResolutionOptions _commandResolutionOptions; + private readonly SearchResolutionOptions _commandResolutionOptions; /// /// Determines which types of commands to look for. @@ -1495,12 +1490,12 @@ private static CanDoPathLookupResult CanDoPathLookup(string possiblePath) /// The enumerator that uses the Path to /// search for commands. /// - private CommandPathSearch _pathSearcher; + private CommandPathSearch? _pathSearcher; /// /// The execution context instance for the current engine... /// - private ExecutionContext _context; + private readonly ExecutionContext _context; /// /// A routine to initialize the path searcher... @@ -1556,7 +1551,7 @@ private void setupPathSearcher() { _canDoPathLookup = true; - string directory = Path.GetDirectoryName(_commandName); + string? directory = Path.GetDirectoryName(_commandName); var directoryCollection = new LookupPathCollection { directory }; CommandDiscovery.discoveryTracer.WriteLine( @@ -1588,7 +1583,7 @@ private void setupPathSearcher() // We must try to resolve the path as an PSPath or else we can't do // path lookup for relative paths. - string directory = Path.GetDirectoryName(_commandName); + string? directory = Path.GetDirectoryName(_commandName); directory = ResolvePSPath(directory); CommandDiscovery.discoveryTracer.WriteLine( @@ -1641,10 +1636,7 @@ public void Reset() _commandTypes &= ~CommandTypes.ExternalScript; } - if (_pathSearcher != null) - { - _pathSearcher.Reset(); - } + _pathSearcher?.Reset(); _currentMatch = null; _currentState = SearchState.SearchingAliases; @@ -1664,17 +1656,17 @@ internal CommandOrigin CommandOrigin /// /// An enumerator of the matching aliases. /// - private IEnumerator _matchingAlias; + private IEnumerator? _matchingAlias; /// /// An enumerator of the matching functions. /// - private IEnumerator _matchingFunctionEnumerator; + private IEnumerator? _matchingFunctionEnumerator; /// /// The CommandInfo that references the command that matches the pattern. /// - private CommandInfo _currentMatch; + private CommandInfo? _currentMatch; private bool _canDoPathLookup; private CanDoPathLookupResult _canDoPathLookupResult = CanDoPathLookupResult.Yes; diff --git a/src/System.Management.Automation/engine/MshSnapinQualifiedName.cs b/src/System.Management.Automation/engine/MshSnapinQualifiedName.cs index 42f73df2a46..0b2a71939a5 100644 --- a/src/System.Management.Automation/engine/MshSnapinQualifiedName.cs +++ b/src/System.Management.Automation/engine/MshSnapinQualifiedName.cs @@ -1,6 +1,8 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +#nullable enable + using Dbg = System.Management.Automation.Diagnostics; namespace System.Management.Automation @@ -62,11 +64,11 @@ private PSSnapinQualifiedName(string[] splitName) /// /// An instance of the Name class. /// - internal static PSSnapinQualifiedName GetInstance(string name) + internal static PSSnapinQualifiedName? GetInstance(string name) { if (name == null) return null; - PSSnapinQualifiedName result = null; + PSSnapinQualifiedName? result = null; string[] splitName = name.Split(Utils.Separators.Backslash); if (splitName.Length == 0 || splitName.Length > 2) return null; @@ -96,7 +98,7 @@ internal string FullName /// /// Gets the command's PSSnapin name. /// - internal string PSSnapInName + internal string? PSSnapInName { get { @@ -104,7 +106,7 @@ internal string PSSnapInName } } - private string _psSnapinName; + private string? _psSnapinName; /// /// Gets the command's short name. From c5f7fb73913fed15339c1b6a8fcbde6ec6f95a26 Mon Sep 17 00:00:00 2001 From: Staffan Gustafsson Date: Wed, 20 May 2020 16:55:01 +0200 Subject: [PATCH 2/3] Addressing xtqqczze review comments --- .../engine/CommandSearcher.cs | 1 + .../engine/MshSnapinQualifiedName.cs | 11 +++++------ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/System.Management.Automation/engine/CommandSearcher.cs b/src/System.Management.Automation/engine/CommandSearcher.cs index a7ed3bd63b7..a350ec74918 100644 --- a/src/System.Management.Automation/engine/CommandSearcher.cs +++ b/src/System.Management.Automation/engine/CommandSearcher.cs @@ -364,6 +364,7 @@ public bool MoveNext() { try { + // the previous call to setupPathSearcher ensures _pathSearcher != null while (currentMatch == null && _pathSearcher!.MoveNext()) { currentMatch = GetInfoFromPath(((IEnumerator)_pathSearcher).Current); diff --git a/src/System.Management.Automation/engine/MshSnapinQualifiedName.cs b/src/System.Management.Automation/engine/MshSnapinQualifiedName.cs index 0b2a71939a5..b23df2202eb 100644 --- a/src/System.Management.Automation/engine/MshSnapinQualifiedName.cs +++ b/src/System.Management.Automation/engine/MshSnapinQualifiedName.cs @@ -64,15 +64,14 @@ private PSSnapinQualifiedName(string[] splitName) /// /// An instance of the Name class. /// - internal static PSSnapinQualifiedName? GetInstance(string name) + internal static PSSnapinQualifiedName? GetInstance(string? name) { if (name == null) return null; - PSSnapinQualifiedName? result = null; string[] splitName = name.Split(Utils.Separators.Backslash); if (splitName.Length == 0 || splitName.Length > 2) return null; - result = new PSSnapinQualifiedName(splitName); + var result = new PSSnapinQualifiedName(splitName); // If the shortname is empty, then return null... if (string.IsNullOrEmpty(result.ShortName)) { @@ -93,7 +92,7 @@ internal string FullName } } - private string _fullName; + private readonly string _fullName; /// /// Gets the command's PSSnapin name. @@ -106,7 +105,7 @@ internal string? PSSnapInName } } - private string? _psSnapinName; + private readonly string? _psSnapinName; /// /// Gets the command's short name. @@ -119,7 +118,7 @@ internal string ShortName } } - private string _shortName; + private readonly string _shortName; /// /// The full name. From 24326d95748e6023acb7da22ce7d0d6ffddb2298 Mon Sep 17 00:00:00 2001 From: Staffan Gustafsson Date: Wed, 20 May 2020 21:17:16 +0200 Subject: [PATCH 3/3] Addressing Ilya's review comments --- src/System.Management.Automation/engine/CommandPathSearch.cs | 1 - src/System.Management.Automation/engine/CommandSearcher.cs | 5 ++++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/System.Management.Automation/engine/CommandPathSearch.cs b/src/System.Management.Automation/engine/CommandPathSearch.cs index 7ba6476e26f..96c6c8c1953 100644 --- a/src/System.Management.Automation/engine/CommandPathSearch.cs +++ b/src/System.Management.Automation/engine/CommandPathSearch.cs @@ -23,7 +23,6 @@ internal class CommandPathSearch : IEnumerable, IEnumerator /// /// Constructs a command searching enumerator that resolves the location /// of a command using the PATH environment variable. - /// of a command using the PATH environment variable. /// /// /// The command name to search for in the path. diff --git a/src/System.Management.Automation/engine/CommandSearcher.cs b/src/System.Management.Automation/engine/CommandSearcher.cs index a350ec74918..d3a1033c932 100644 --- a/src/System.Management.Automation/engine/CommandSearcher.cs +++ b/src/System.Management.Automation/engine/CommandSearcher.cs @@ -928,7 +928,10 @@ private bool ShouldSkipCommandResolutionForConstrainedLanguage(CommandInfo? resu } } - module ??= modules[0]; + if (module == null) + { + module = modules[0]; + } } return module;