From f8f808370aaa07a7309e5e7cc9cb7cb9a6b4b3a1 Mon Sep 17 00:00:00 2001 From: "Travis C. LaGrone" <22419287+travis-c-lagrone@users.noreply.github.com> Date: Mon, 17 Jun 2019 21:41:20 -0500 Subject: [PATCH 1/8] Annotatively research current state of exception handling surrounding settings file load invocation --- .../Commands/InvokeScriptAnalyzerCommand.cs | 12 +++++++++++ Engine/Helper.cs | 6 ++++++ Engine/Settings.cs | 21 ++++++++++++++++++- 3 files changed, 38 insertions(+), 1 deletion(-) diff --git a/Engine/Commands/InvokeScriptAnalyzerCommand.cs b/Engine/Commands/InvokeScriptAnalyzerCommand.cs index c51fc9f38..af9b06386 100644 --- a/Engine/Commands/InvokeScriptAnalyzerCommand.cs +++ b/Engine/Commands/InvokeScriptAnalyzerCommand.cs @@ -268,6 +268,7 @@ protected override void BeginProcessing() Helper.Instance = new Helper( SessionState.InvokeCommand, this); + // NOTE Helper.Instance.Initialize() does *not* modify this.settings. Helper.Instance.Initialize(); var psVersionTable = this.SessionState.PSVariable.GetValue("PSVersionTable") as Hashtable; @@ -276,6 +277,7 @@ protected override void BeginProcessing() Helper.Instance.SetPSVersionTable(psVersionTable); } + // NOTE Helper.ProcessCustomRulePaths does *not* modify this.settings. string[] rulePaths = Helper.ProcessCustomRulePaths( customRulePath, this.SessionState, @@ -284,6 +286,7 @@ protected override void BeginProcessing() if (IsFileParameterSet() && Path != null) { // just used to obtain the directory to use to find settings below + // NOTE ProcessPath() does *not* modify this.settings. ProcessPath(); } @@ -292,17 +295,24 @@ protected override void BeginProcessing() var combIncludeDefaultRules = IncludeDefaultRules.IsPresent; try { + // THROW PSSASettings.Create(object, string, IOutputWriter, GetResolvedProviderPathFromPSPath) throws if TODO + // NOTE Microsoft.Windows.PowerShell.ScriptAnalyzer.Settings.Create(...) types the input, gets the input (if necessary), and parses it (if any). var settingsObj = PSSASettings.Create( + // NOTHROW A null this.settings results in returning an "empty" (but not null) settingsObj without exception. + // NOTE this.settings is unmodified since start of BeginProcessing(). Thus, it is exactly the raw argument value, if any. settings, processedPaths == null || processedPaths.Count == 0 ? CurrentProviderLocation("FileSystem").ProviderPath : processedPaths[0], this, GetResolvedProviderPathFromPSPath); + // NOTE settingsObj cannot be null here since PSSASettings.Create(...) returns exactly `new Settings(settingsFound)`, which can never be null (but can throw). if (settingsObj != null) { + // NOTHROW UpdateSettings can throw an ArgumentNullException, but that will never happen since settingsObj is tested for non-nullity immediately above. ScriptAnalyzer.Instance.UpdateSettings(settingsObj); // For includeDefaultRules and RecurseCustomRulePath we override the value in the settings file by // command line argument. + // NOTHROW InvokeScriptAnalyzerCommand.OverrideSwitchParam(bool, string) combRecurseCustomRulePath = OverrideSwitchParam( settingsObj.RecurseCustomRulePath, "RecurseCustomRulePath"); @@ -315,6 +325,7 @@ protected override void BeginProcessing() // simultaneously. But since, this was done before with IncludeRules, ExcludeRules and Severity, // we use the same strategy for CustomRulePath. So, we take the union of CustomRulePath provided in // the settings file and if provided on command line. + // THROW Helper.ProcessCustomRulePaths(string[], SessionState, bool) throws one of six different exceptions if a settings' custom rule path is invalid somehow (e.g. drive doesn't exit; no wildcards but item doesn't exist; provider throws a lower-level exception; etc.). See the implementation of Helper.ProcessCustomRulePaths(string[], SessionState, bool) for details. var settingsCustomRulePath = Helper.ProcessCustomRulePaths( settingsObj?.CustomRulePath?.ToArray(), this.SessionState, @@ -327,6 +338,7 @@ protected override void BeginProcessing() } catch { + // NOTE Any exception in resolving, getting, parsing, updating, etc. the settings herein results in an contextless WriteWarning(Strings.SettingsNotParsable), regardless of provenance. this.WriteWarning(String.Format(CultureInfo.CurrentCulture, Strings.SettingsNotParsable)); stopProcessing = true; return; diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 330547d02..e504868d4 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -1506,6 +1506,12 @@ public static string[] ProcessCustomRulePaths(string[] rulePaths, SessionState s Collection pathInfo = new Collection(); foreach (string rulePath in rulePaths) { + // THROW PathIntrinsics.GetResolvedPSPathFromPSPath(rulePath) throws [System.Management.Automation.ProviderNotFoundException] if path is a provider-qualified path and the specified provider does not exist. + // THROW PathIntrinsics.GetResolvedPSPathFromPSPath(rulePath) throws [System.Management.Automation.DriveNotFoundException] if path is a drive-qualified path and the specified drive does not exist. + // THROW PathIntrinsics.GetResolvedPSPathFromPSPath(rulePath) throws [System.Management.Automation.ProviderInvocationException] if the provider throws an exception when its MakePath gets called. + // THROW PathIntrinsics.GetResolvedPSPathFromPSPath(rulePath) throws [System.NotSupportedException] if the provider does not support multiple items. + // THROW PathIntrinsics.GetResolvedPSPathFromPSPath(rulePath) throws [System.InvalidOperationException] the home location for the provider is not set and path starts with a "~". + // THROW PathIntrinsics.GetResolvedPSPathFromPSPath(rulePath) throws [System.Management.Automation.ItemNotFoundException] if path does not contain wildcard characters and could not be found. Collection pathInfosForRulePath = sessionState.Path.GetResolvedPSPathFromPSPath(rulePath); if (null != pathInfosForRulePath) { diff --git a/Engine/Settings.cs b/Engine/Settings.cs index 506733cc2..74a357767 100644 --- a/Engine/Settings.cs +++ b/Engine/Settings.cs @@ -73,10 +73,12 @@ public Settings(object settings, Func presetResolver) if (File.Exists(settingsFilePath)) { filePath = settingsFilePath; + // QUESTION When does parseSettingsFile(string) throw? parseSettingsFile(settingsFilePath); } else { + // THROW ArgumentException(Strings.InvalidPath) if the resolution of the settings argument via GetResolvedProviderPathFromPSPathDelegate is non-null but not an existing file. (e.g. it may be a directory) throw new ArgumentException( String.Format( CultureInfo.CurrentCulture, @@ -89,10 +91,12 @@ public Settings(object settings, Func presetResolver) var settingsHashtable = settings as Hashtable; if (settingsHashtable != null) { + // QUESTION When does parseSettingsHashtable(Hashtable) throw? parseSettingsHashtable(settingsHashtable); } else { + // THROW ArgumentException(Strings.SettingsInvalidType) if settings is not convertible (`as` keyword) to Hashtable. throw new ArgumentException(Strings.SettingsInvalidType); } } @@ -179,11 +183,12 @@ public static string GetSettingPresetFilePath(string settingPreset) /// An output writer. /// The GetResolvedProviderPathFromPSPath method from PSCmdlet to resolve relative path including wildcard support. /// An object of Settings type. + // THROW Settings.Create(object, string, IOutputWriter, GetResolvedProviderPathFromPSPath) throws only because of the contained invocation to the Settings constructor, which does throw. internal static Settings Create(object settingsObj, string cwd, IOutputWriter outputWriter, PathResolver.GetResolvedProviderPathFromPSPath> getResolvedProviderPathFromPSPathDelegate) { object settingsFound; - var settingsMode = FindSettingsMode(settingsObj, cwd, out settingsFound); + var settingsMode = FindSettingsMode(settingsObj, cwd, out settingsFound); // NOTHROW switch (settingsMode) { @@ -205,6 +210,7 @@ internal static Settings Create(object settingsObj, string cwd, IOutputWriter ou var userProvidedSettingsString = settingsFound.ToString(); try { + // THROW ..Single() throws [System.InvalidOperationException] if the settings path does not resolve to exactly one item. (more or less) var resolvedPath = getResolvedProviderPathFromPSPathDelegate(userProvidedSettingsString, out ProviderInfo providerInfo).Single(); settingsFound = resolvedPath; outputWriter?.WriteVerbose( @@ -213,8 +219,11 @@ internal static Settings Create(object settingsObj, string cwd, IOutputWriter ou Strings.SettingsUsingFile, resolvedPath)); } + // NOTE This catch block effectively makes *any* exception resulting from resolving the settings file path reduce to the case of no settings (by way of null settingsFound). Only a WriteVerbose(Strings.SettingsCannotFindFile) identifies what happened. catch { + // TODO Upgrade WriteVerbose(Strings.SettingsCannotFindFile) to WriteWarning(Strings.SettingsCannotFindFile). + // TODO Perform a ShouldContinue (?) confirmation check in the event that an exception occurred while resolving the settings file path. outputWriter?.WriteVerbose( String.Format( CultureInfo.CurrentCulture, @@ -238,6 +247,7 @@ internal static Settings Create(object settingsObj, string cwd, IOutputWriter ou return null; } + // THROW new Settings(object) return new Settings(settingsFound); } @@ -456,16 +466,19 @@ private void parseSettingsHashtable(Hashtable settingsHashtable) } } + // NOTE parseSettingsFile concludes by calling parseSettingsHashtable if it (parseSettingsFile) is successful. private void parseSettingsFile(string settingsFilePath) { Token[] parserTokens = null; ParseError[] parserErrors = null; + // QUESTION Can Parser.ParseFile throw? Ast profileAst = Parser.ParseFile(settingsFilePath, out parserTokens, out parserErrors); IEnumerable hashTableAsts = profileAst.FindAll(item => item is HashtableAst, false); // no hashtable, raise warning if (hashTableAsts.Count() == 0) { + // THROW parseSettingsFile throws ArgumentException(Strings.InvalidProfile) if no hashTableAst is detected in settings file. throw new ArgumentException(string.Format(CultureInfo.CurrentCulture, Strings.InvalidProfile, settingsFilePath)); } @@ -475,15 +488,18 @@ private void parseSettingsFile(string settingsFilePath) { // ideally we should use HashtableAst.SafeGetValue() but since // it is not available on PSv3, we resort to our own narrow implementation. + // QUESTION When does GetSafeValueFromHashtableAst(hashTableAst) throw? hashtable = GetSafeValueFromHashtableAst(hashTableAst); } catch (InvalidOperationException e) { + // THROW parseSettingsFile throws ArgumentException(Strings.InvalidProfile) if GetSafeValueFromHashtableAst(hashTableAst) throws an InvalidOperationException. throw new ArgumentException(Strings.InvalidProfile, e); } if (hashtable == null) { + // THROW parseSettingsFile throws ArgumentException if GetSafeValueFromHashtableAst(hashTableAst) returns null. throw new ArgumentException( String.Format( CultureInfo.CurrentCulture, @@ -491,6 +507,7 @@ private void parseSettingsFile(string settingsFilePath) settingsFilePath)); } + // QUESTION When does parseSettingsHashtable(Hashtable) throw? parseSettingsHashtable(hashtable); } @@ -686,6 +703,7 @@ private static bool IsBuiltinSettingPreset(object settingPreset) return false; } + // NOTHROW FindSettingsMode(object, string, out object) internal static SettingsMode FindSettingsMode(object settings, string path, out object settingsFound) { var settingsMode = SettingsMode.None; @@ -706,6 +724,7 @@ internal static SettingsMode FindSettingsMode(object settings, string path, out { // if settings are not provided explicitly, look for it in the given path // check if pssasettings.psd1 exists + // COMBAK Refactor the magic string literal "PSScriptAnalyzerSettings.psd1" to a public const field on the class. (bare minimum... although will also help open possibility for user-configurable default name) var settingsFilename = "PSScriptAnalyzerSettings.psd1"; var settingsFilePath = Path.Combine(directory, settingsFilename); settingsFound = settingsFilePath; From b75dced398b7255a5645923e0effc290cf0a2084 Mon Sep 17 00:00:00 2001 From: "Travis C. LaGrone" <22419287+travis-c-lagrone@users.noreply.github.com> Date: Tue, 18 Jun 2019 05:05:14 -0500 Subject: [PATCH 2/8] Throw a contextful terminating error if processing the argument of 'Invoke-ScriptAnalyzer -Settings' results in an exception --- Engine/Commands/InvokeScriptAnalyzerCommand.cs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/Engine/Commands/InvokeScriptAnalyzerCommand.cs b/Engine/Commands/InvokeScriptAnalyzerCommand.cs index af9b06386..f34b856b1 100644 --- a/Engine/Commands/InvokeScriptAnalyzerCommand.cs +++ b/Engine/Commands/InvokeScriptAnalyzerCommand.cs @@ -295,7 +295,7 @@ protected override void BeginProcessing() var combIncludeDefaultRules = IncludeDefaultRules.IsPresent; try { - // THROW PSSASettings.Create(object, string, IOutputWriter, GetResolvedProviderPathFromPSPath) throws if TODO + // THROW PSSASettings.Create(object, string, IOutputWriter, GetResolvedProviderPathFromPSPath) throws if ... // NOTE Microsoft.Windows.PowerShell.ScriptAnalyzer.Settings.Create(...) types the input, gets the input (if necessary), and parses it (if any). var settingsObj = PSSASettings.Create( // NOTHROW A null this.settings results in returning an "empty" (but not null) settingsObj without exception. @@ -307,7 +307,7 @@ protected override void BeginProcessing() // NOTE settingsObj cannot be null here since PSSASettings.Create(...) returns exactly `new Settings(settingsFound)`, which can never be null (but can throw). if (settingsObj != null) { - // NOTHROW UpdateSettings can throw an ArgumentNullException, but that will never happen since settingsObj is tested for non-nullity immediately above. + // NOTHROW UpdateSettings(object) can throw an ArgumentNullException, but that will never happen since settingsObj is tested for non-nullity immediately above. ScriptAnalyzer.Instance.UpdateSettings(settingsObj); // For includeDefaultRules and RecurseCustomRulePath we override the value in the settings file by @@ -336,12 +336,15 @@ protected override void BeginProcessing() ? rulePaths : rulePaths.Concat(settingsCustomRulePath).ToArray(); } - catch + catch (Exception e) { // NOTE Any exception in resolving, getting, parsing, updating, etc. the settings herein results in an contextless WriteWarning(Strings.SettingsNotParsable), regardless of provenance. - this.WriteWarning(String.Format(CultureInfo.CurrentCulture, Strings.SettingsNotParsable)); - stopProcessing = true; - return; + var errorRecord = new ErrorRecord( + e, + "SettingsInvalidOrNotFound,Microsoft.Windows.PowerShell.ScriptAnalyzer.Commands.InvokeScriptAnalyzerCommand", + ErrorCategory.InvalidArgument, + settings); + this.ThrowTerminatingError(errorRecord); } ScriptAnalyzer.Instance.Initialize( @@ -369,7 +372,7 @@ protected override void ProcessRecord() { ProcessPath(); } - + #if !PSV3 // TODO Support dependency resolution for analyzing script definitions if (saveDscDependency) From 66483449be59d4f1524c210cee280f4ebd81ea19 Mon Sep 17 00:00:00 2001 From: "Travis C. LaGrone" <22419287+travis-c-lagrone@users.noreply.github.com> Date: Tue, 18 Jun 2019 06:33:55 -0500 Subject: [PATCH 3/8] Refine id and category of error for when processing value of `Invoke-ScriptAnalyzer -Settings` results in an exception --- .../Commands/InvokeScriptAnalyzerCommand.cs | 47 +++++++++++++++++-- 1 file changed, 42 insertions(+), 5 deletions(-) diff --git a/Engine/Commands/InvokeScriptAnalyzerCommand.cs b/Engine/Commands/InvokeScriptAnalyzerCommand.cs index f34b856b1..6a12d1c13 100644 --- a/Engine/Commands/InvokeScriptAnalyzerCommand.cs +++ b/Engine/Commands/InvokeScriptAnalyzerCommand.cs @@ -8,6 +8,7 @@ using System.Collections.ObjectModel; using System.Diagnostics.CodeAnalysis; using System.Globalization; +using System.IO; using System.Linq; using System.Management.Automation; using System.Management.Automation.Runspaces; @@ -339,11 +340,47 @@ protected override void BeginProcessing() catch (Exception e) { // NOTE Any exception in resolving, getting, parsing, updating, etc. the settings herein results in an contextless WriteWarning(Strings.SettingsNotParsable), regardless of provenance. - var errorRecord = new ErrorRecord( - e, - "SettingsInvalidOrNotFound,Microsoft.Windows.PowerShell.ScriptAnalyzer.Commands.InvokeScriptAnalyzerCommand", - ErrorCategory.InvalidArgument, - settings); + string errorId; + ErrorCategory errorCategory; + switch (e) + { + case ArgumentException _: + errorId = "InvalidSettingsArgument"; + errorCategory = ErrorCategory.InvalidArgument; + break; + case InvalidDataException _: + errorId = "InvalidSettingsData"; + errorCategory = ErrorCategory.InvalidData; + break; + case InvalidOperationException _: + errorId = "InvalidPathForProvider"; // InvalidOperationException can arise from provider-specific limitations interacting with a settings path (e.g. wildcards, home, containers, etc.). + errorCategory = ErrorCategory.InvalidOperation; + break; + case InternalBufferOverflowException _: + case PathTooLongException _: + errorId = "PathOrSettingsExceededLimits"; + errorCategory = ErrorCategory.LimitsExceeded; + break; + case NotSupportedException _: + errorId = "PathOrSettingNotSupported"; + errorCategory = ErrorCategory.NotEnabled; + break; + case DirectoryNotFoundException _: + case System.IO.DriveNotFoundException _: + case System.Management.Automation.DriveNotFoundException _: + case FileNotFoundException _: + case ItemNotFoundException _: + case ProviderNotFoundException _: + errorId = "SettingsNotFound"; + errorCategory = ErrorCategory.ObjectNotFound; + break; + default: + errorId = "SettingsNotLoadable"; + errorCategory = ErrorCategory.NotSpecified; + break; + } + + var errorRecord = new ErrorRecord(e, errorId, errorCategory, this.settings); this.ThrowTerminatingError(errorRecord); } From 0472359734f75ef0c32913b257d5e4f5fdfc88e2 Mon Sep 17 00:00:00 2001 From: "Travis C. LaGrone" <22419287+travis-c-lagrone@users.noreply.github.com> Date: Tue, 18 Jun 2019 06:44:01 -0500 Subject: [PATCH 4/8] Revert "Annotatively research current state of exception handling surrounding settings file load invocation" This reverts commit 3bfd45ba6ccf0de5b3853228d85130f18e342071. --- .../Commands/InvokeScriptAnalyzerCommand.cs | 12 ----------- Engine/Helper.cs | 6 ------ Engine/Settings.cs | 21 +------------------ 3 files changed, 1 insertion(+), 38 deletions(-) diff --git a/Engine/Commands/InvokeScriptAnalyzerCommand.cs b/Engine/Commands/InvokeScriptAnalyzerCommand.cs index 6a12d1c13..9ead4d125 100644 --- a/Engine/Commands/InvokeScriptAnalyzerCommand.cs +++ b/Engine/Commands/InvokeScriptAnalyzerCommand.cs @@ -269,7 +269,6 @@ protected override void BeginProcessing() Helper.Instance = new Helper( SessionState.InvokeCommand, this); - // NOTE Helper.Instance.Initialize() does *not* modify this.settings. Helper.Instance.Initialize(); var psVersionTable = this.SessionState.PSVariable.GetValue("PSVersionTable") as Hashtable; @@ -278,7 +277,6 @@ protected override void BeginProcessing() Helper.Instance.SetPSVersionTable(psVersionTable); } - // NOTE Helper.ProcessCustomRulePaths does *not* modify this.settings. string[] rulePaths = Helper.ProcessCustomRulePaths( customRulePath, this.SessionState, @@ -287,7 +285,6 @@ protected override void BeginProcessing() if (IsFileParameterSet() && Path != null) { // just used to obtain the directory to use to find settings below - // NOTE ProcessPath() does *not* modify this.settings. ProcessPath(); } @@ -296,24 +293,17 @@ protected override void BeginProcessing() var combIncludeDefaultRules = IncludeDefaultRules.IsPresent; try { - // THROW PSSASettings.Create(object, string, IOutputWriter, GetResolvedProviderPathFromPSPath) throws if ... - // NOTE Microsoft.Windows.PowerShell.ScriptAnalyzer.Settings.Create(...) types the input, gets the input (if necessary), and parses it (if any). var settingsObj = PSSASettings.Create( - // NOTHROW A null this.settings results in returning an "empty" (but not null) settingsObj without exception. - // NOTE this.settings is unmodified since start of BeginProcessing(). Thus, it is exactly the raw argument value, if any. settings, processedPaths == null || processedPaths.Count == 0 ? CurrentProviderLocation("FileSystem").ProviderPath : processedPaths[0], this, GetResolvedProviderPathFromPSPath); - // NOTE settingsObj cannot be null here since PSSASettings.Create(...) returns exactly `new Settings(settingsFound)`, which can never be null (but can throw). if (settingsObj != null) { - // NOTHROW UpdateSettings(object) can throw an ArgumentNullException, but that will never happen since settingsObj is tested for non-nullity immediately above. ScriptAnalyzer.Instance.UpdateSettings(settingsObj); // For includeDefaultRules and RecurseCustomRulePath we override the value in the settings file by // command line argument. - // NOTHROW InvokeScriptAnalyzerCommand.OverrideSwitchParam(bool, string) combRecurseCustomRulePath = OverrideSwitchParam( settingsObj.RecurseCustomRulePath, "RecurseCustomRulePath"); @@ -326,7 +316,6 @@ protected override void BeginProcessing() // simultaneously. But since, this was done before with IncludeRules, ExcludeRules and Severity, // we use the same strategy for CustomRulePath. So, we take the union of CustomRulePath provided in // the settings file and if provided on command line. - // THROW Helper.ProcessCustomRulePaths(string[], SessionState, bool) throws one of six different exceptions if a settings' custom rule path is invalid somehow (e.g. drive doesn't exit; no wildcards but item doesn't exist; provider throws a lower-level exception; etc.). See the implementation of Helper.ProcessCustomRulePaths(string[], SessionState, bool) for details. var settingsCustomRulePath = Helper.ProcessCustomRulePaths( settingsObj?.CustomRulePath?.ToArray(), this.SessionState, @@ -339,7 +328,6 @@ protected override void BeginProcessing() } catch (Exception e) { - // NOTE Any exception in resolving, getting, parsing, updating, etc. the settings herein results in an contextless WriteWarning(Strings.SettingsNotParsable), regardless of provenance. string errorId; ErrorCategory errorCategory; switch (e) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index e504868d4..330547d02 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -1506,12 +1506,6 @@ public static string[] ProcessCustomRulePaths(string[] rulePaths, SessionState s Collection pathInfo = new Collection(); foreach (string rulePath in rulePaths) { - // THROW PathIntrinsics.GetResolvedPSPathFromPSPath(rulePath) throws [System.Management.Automation.ProviderNotFoundException] if path is a provider-qualified path and the specified provider does not exist. - // THROW PathIntrinsics.GetResolvedPSPathFromPSPath(rulePath) throws [System.Management.Automation.DriveNotFoundException] if path is a drive-qualified path and the specified drive does not exist. - // THROW PathIntrinsics.GetResolvedPSPathFromPSPath(rulePath) throws [System.Management.Automation.ProviderInvocationException] if the provider throws an exception when its MakePath gets called. - // THROW PathIntrinsics.GetResolvedPSPathFromPSPath(rulePath) throws [System.NotSupportedException] if the provider does not support multiple items. - // THROW PathIntrinsics.GetResolvedPSPathFromPSPath(rulePath) throws [System.InvalidOperationException] the home location for the provider is not set and path starts with a "~". - // THROW PathIntrinsics.GetResolvedPSPathFromPSPath(rulePath) throws [System.Management.Automation.ItemNotFoundException] if path does not contain wildcard characters and could not be found. Collection pathInfosForRulePath = sessionState.Path.GetResolvedPSPathFromPSPath(rulePath); if (null != pathInfosForRulePath) { diff --git a/Engine/Settings.cs b/Engine/Settings.cs index 74a357767..506733cc2 100644 --- a/Engine/Settings.cs +++ b/Engine/Settings.cs @@ -73,12 +73,10 @@ public Settings(object settings, Func presetResolver) if (File.Exists(settingsFilePath)) { filePath = settingsFilePath; - // QUESTION When does parseSettingsFile(string) throw? parseSettingsFile(settingsFilePath); } else { - // THROW ArgumentException(Strings.InvalidPath) if the resolution of the settings argument via GetResolvedProviderPathFromPSPathDelegate is non-null but not an existing file. (e.g. it may be a directory) throw new ArgumentException( String.Format( CultureInfo.CurrentCulture, @@ -91,12 +89,10 @@ public Settings(object settings, Func presetResolver) var settingsHashtable = settings as Hashtable; if (settingsHashtable != null) { - // QUESTION When does parseSettingsHashtable(Hashtable) throw? parseSettingsHashtable(settingsHashtable); } else { - // THROW ArgumentException(Strings.SettingsInvalidType) if settings is not convertible (`as` keyword) to Hashtable. throw new ArgumentException(Strings.SettingsInvalidType); } } @@ -183,12 +179,11 @@ public static string GetSettingPresetFilePath(string settingPreset) /// An output writer. /// The GetResolvedProviderPathFromPSPath method from PSCmdlet to resolve relative path including wildcard support. /// An object of Settings type. - // THROW Settings.Create(object, string, IOutputWriter, GetResolvedProviderPathFromPSPath) throws only because of the contained invocation to the Settings constructor, which does throw. internal static Settings Create(object settingsObj, string cwd, IOutputWriter outputWriter, PathResolver.GetResolvedProviderPathFromPSPath> getResolvedProviderPathFromPSPathDelegate) { object settingsFound; - var settingsMode = FindSettingsMode(settingsObj, cwd, out settingsFound); // NOTHROW + var settingsMode = FindSettingsMode(settingsObj, cwd, out settingsFound); switch (settingsMode) { @@ -210,7 +205,6 @@ internal static Settings Create(object settingsObj, string cwd, IOutputWriter ou var userProvidedSettingsString = settingsFound.ToString(); try { - // THROW ..Single() throws [System.InvalidOperationException] if the settings path does not resolve to exactly one item. (more or less) var resolvedPath = getResolvedProviderPathFromPSPathDelegate(userProvidedSettingsString, out ProviderInfo providerInfo).Single(); settingsFound = resolvedPath; outputWriter?.WriteVerbose( @@ -219,11 +213,8 @@ internal static Settings Create(object settingsObj, string cwd, IOutputWriter ou Strings.SettingsUsingFile, resolvedPath)); } - // NOTE This catch block effectively makes *any* exception resulting from resolving the settings file path reduce to the case of no settings (by way of null settingsFound). Only a WriteVerbose(Strings.SettingsCannotFindFile) identifies what happened. catch { - // TODO Upgrade WriteVerbose(Strings.SettingsCannotFindFile) to WriteWarning(Strings.SettingsCannotFindFile). - // TODO Perform a ShouldContinue (?) confirmation check in the event that an exception occurred while resolving the settings file path. outputWriter?.WriteVerbose( String.Format( CultureInfo.CurrentCulture, @@ -247,7 +238,6 @@ internal static Settings Create(object settingsObj, string cwd, IOutputWriter ou return null; } - // THROW new Settings(object) return new Settings(settingsFound); } @@ -466,19 +456,16 @@ private void parseSettingsHashtable(Hashtable settingsHashtable) } } - // NOTE parseSettingsFile concludes by calling parseSettingsHashtable if it (parseSettingsFile) is successful. private void parseSettingsFile(string settingsFilePath) { Token[] parserTokens = null; ParseError[] parserErrors = null; - // QUESTION Can Parser.ParseFile throw? Ast profileAst = Parser.ParseFile(settingsFilePath, out parserTokens, out parserErrors); IEnumerable hashTableAsts = profileAst.FindAll(item => item is HashtableAst, false); // no hashtable, raise warning if (hashTableAsts.Count() == 0) { - // THROW parseSettingsFile throws ArgumentException(Strings.InvalidProfile) if no hashTableAst is detected in settings file. throw new ArgumentException(string.Format(CultureInfo.CurrentCulture, Strings.InvalidProfile, settingsFilePath)); } @@ -488,18 +475,15 @@ private void parseSettingsFile(string settingsFilePath) { // ideally we should use HashtableAst.SafeGetValue() but since // it is not available on PSv3, we resort to our own narrow implementation. - // QUESTION When does GetSafeValueFromHashtableAst(hashTableAst) throw? hashtable = GetSafeValueFromHashtableAst(hashTableAst); } catch (InvalidOperationException e) { - // THROW parseSettingsFile throws ArgumentException(Strings.InvalidProfile) if GetSafeValueFromHashtableAst(hashTableAst) throws an InvalidOperationException. throw new ArgumentException(Strings.InvalidProfile, e); } if (hashtable == null) { - // THROW parseSettingsFile throws ArgumentException if GetSafeValueFromHashtableAst(hashTableAst) returns null. throw new ArgumentException( String.Format( CultureInfo.CurrentCulture, @@ -507,7 +491,6 @@ private void parseSettingsFile(string settingsFilePath) settingsFilePath)); } - // QUESTION When does parseSettingsHashtable(Hashtable) throw? parseSettingsHashtable(hashtable); } @@ -703,7 +686,6 @@ private static bool IsBuiltinSettingPreset(object settingPreset) return false; } - // NOTHROW FindSettingsMode(object, string, out object) internal static SettingsMode FindSettingsMode(object settings, string path, out object settingsFound) { var settingsMode = SettingsMode.None; @@ -724,7 +706,6 @@ internal static SettingsMode FindSettingsMode(object settings, string path, out { // if settings are not provided explicitly, look for it in the given path // check if pssasettings.psd1 exists - // COMBAK Refactor the magic string literal "PSScriptAnalyzerSettings.psd1" to a public const field on the class. (bare minimum... although will also help open possibility for user-configurable default name) var settingsFilename = "PSScriptAnalyzerSettings.psd1"; var settingsFilePath = Path.Combine(directory, settingsFilename); settingsFound = settingsFilePath; From a012dec9bdbb135e14644fa59b7442a197d5e695 Mon Sep 17 00:00:00 2001 From: "Travis C. LaGrone" <22419287+travis-c-lagrone@users.noreply.github.com> Date: Tue, 18 Jun 2019 12:07:21 -0500 Subject: [PATCH 5/8] Do not attempt id and category inference for the error to throw when processing the argument of `Invoke-ScriptAnalyzer -Settings` results in an exception --- .../Commands/InvokeScriptAnalyzerCommand.cs | 47 ++----------------- 1 file changed, 5 insertions(+), 42 deletions(-) diff --git a/Engine/Commands/InvokeScriptAnalyzerCommand.cs b/Engine/Commands/InvokeScriptAnalyzerCommand.cs index 9ead4d125..a818998f9 100644 --- a/Engine/Commands/InvokeScriptAnalyzerCommand.cs +++ b/Engine/Commands/InvokeScriptAnalyzerCommand.cs @@ -328,48 +328,11 @@ protected override void BeginProcessing() } catch (Exception e) { - string errorId; - ErrorCategory errorCategory; - switch (e) - { - case ArgumentException _: - errorId = "InvalidSettingsArgument"; - errorCategory = ErrorCategory.InvalidArgument; - break; - case InvalidDataException _: - errorId = "InvalidSettingsData"; - errorCategory = ErrorCategory.InvalidData; - break; - case InvalidOperationException _: - errorId = "InvalidPathForProvider"; // InvalidOperationException can arise from provider-specific limitations interacting with a settings path (e.g. wildcards, home, containers, etc.). - errorCategory = ErrorCategory.InvalidOperation; - break; - case InternalBufferOverflowException _: - case PathTooLongException _: - errorId = "PathOrSettingsExceededLimits"; - errorCategory = ErrorCategory.LimitsExceeded; - break; - case NotSupportedException _: - errorId = "PathOrSettingNotSupported"; - errorCategory = ErrorCategory.NotEnabled; - break; - case DirectoryNotFoundException _: - case System.IO.DriveNotFoundException _: - case System.Management.Automation.DriveNotFoundException _: - case FileNotFoundException _: - case ItemNotFoundException _: - case ProviderNotFoundException _: - errorId = "SettingsNotFound"; - errorCategory = ErrorCategory.ObjectNotFound; - break; - default: - errorId = "SettingsNotLoadable"; - errorCategory = ErrorCategory.NotSpecified; - break; - } - - var errorRecord = new ErrorRecord(e, errorId, errorCategory, this.settings); - this.ThrowTerminatingError(errorRecord); + this.ThrowTerminatingError(new ErrorRecord( + e, + "SettingsNotProcessable", + ErrorCategory.NotSpecified, + this.settings)); } ScriptAnalyzer.Instance.Initialize( From 88ab1ec889f69deb0fe2e569ceaf2f0b65595327 Mon Sep 17 00:00:00 2001 From: "Travis C. LaGrone" <22419287+travis-c-lagrone@users.noreply.github.com> Date: Sat, 22 Jun 2019 19:26:17 -0500 Subject: [PATCH 6/8] Remove `using System.IO` --- Engine/Settings.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/Engine/Settings.cs b/Engine/Settings.cs index 506733cc2..ebc8d7391 100644 --- a/Engine/Settings.cs +++ b/Engine/Settings.cs @@ -7,7 +7,6 @@ using System.Collections.Generic; using System.Collections.ObjectModel; using System.Globalization; -using System.IO; using System.Linq; using System.Management.Automation; using System.Management.Automation.Language; From 2faab31fe7bcb38e1ee223dc6b46022f268eed10 Mon Sep 17 00:00:00 2001 From: "Travis C. LaGrone" <22419287+travis-c-lagrone@users.noreply.github.com> Date: Sun, 23 Jun 2019 01:27:29 -0500 Subject: [PATCH 7/8] Revert "Remove `using System.IO`" This reverts commit 88ab1ec889f69deb0fe2e569ceaf2f0b65595327. `using System.IO` is required in Engine\Settings.cs after all. The main use is for pre-existing unqualified references to the type InvalidDataException. --- Engine/Settings.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/Engine/Settings.cs b/Engine/Settings.cs index ebc8d7391..506733cc2 100644 --- a/Engine/Settings.cs +++ b/Engine/Settings.cs @@ -7,6 +7,7 @@ using System.Collections.Generic; using System.Collections.ObjectModel; using System.Globalization; +using System.IO; using System.Linq; using System.Management.Automation; using System.Management.Automation.Language; From 7bb07d5681ca88b5fbb2400c5b22a309fa93d33b Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 29 Jun 2019 10:44:48 +0100 Subject: [PATCH 8/8] Remove unused using after refactoring. Use more descriptive variable name and make error record arguments consistent with usage in other parts of the solution. --- Engine/Commands/InvokeScriptAnalyzerCommand.cs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/Engine/Commands/InvokeScriptAnalyzerCommand.cs b/Engine/Commands/InvokeScriptAnalyzerCommand.cs index a818998f9..e7c800f54 100644 --- a/Engine/Commands/InvokeScriptAnalyzerCommand.cs +++ b/Engine/Commands/InvokeScriptAnalyzerCommand.cs @@ -8,7 +8,6 @@ using System.Collections.ObjectModel; using System.Diagnostics.CodeAnalysis; using System.Globalization; -using System.IO; using System.Linq; using System.Management.Automation; using System.Management.Automation.Runspaces; @@ -326,12 +325,12 @@ protected override void BeginProcessing() ? rulePaths : rulePaths.Concat(settingsCustomRulePath).ToArray(); } - catch (Exception e) + catch (Exception exception) { this.ThrowTerminatingError(new ErrorRecord( - e, - "SettingsNotProcessable", - ErrorCategory.NotSpecified, + exception, + "SETTINGS_ERROR", + ErrorCategory.InvalidData, this.settings)); }