Fix (and test) regression with PSScriptAnalyzer default rules (#1873)

When we swapped a ternary for a null-coalescing operator we ran into a
problem because the two constructor overloads took `string[]` and
`object`, and we were now calling the constructor first, which meant the
`object` version was always being called. This erroneously sent the rule
list to the settings object parameter.

Author: andyleejordan

src/PowerShellEditorServices/Services/Analysis/AnalysisService.cs

@@ -39,19 +39,19 @@ internal static string GetUniqueIdFromDiagnostic(Diagnostic diagnostic)
             Position end = diagnostic.Range.End;
 
             StringBuilder sb = new StringBuilder(256)
-            .Append(diagnostic.Source ?? "?")
-            .Append('_')
-            .Append(diagnostic.Code?.IsString ?? true ? diagnostic.Code?.String : diagnostic.Code?.Long.ToString())
-            .Append('_')
-            .Append(diagnostic.Severity?.ToString() ?? "?")
-            .Append('_')
-            .Append(start.Line)
-            .Append(':')
-            .Append(start.Character)
-            .Append('-')
-            .Append(end.Line)
-            .Append(':')
-            .Append(end.Character);
+                .Append(diagnostic.Source ?? "?")
+                .Append('_')
+                .Append(diagnostic.Code?.IsString ?? true ? diagnostic.Code?.String : diagnostic.Code?.Long.ToString())
+                .Append('_')
+                .Append(diagnostic.Severity?.ToString() ?? "?")
+                .Append('_')
+                .Append(start.Line)
+                .Append(':')
+                .Append(start.Character)
+                .Append('-')
+                .Append(end.Line)
+                .Append(':')
+                .Append(end.Character);
 
             return sb.ToString();
         }
@@ -60,7 +60,7 @@ internal static string GetUniqueIdFromDiagnostic(Diagnostic diagnostic)
         /// Defines the list of Script Analyzer rules to include by default if
         /// no settings file is specified.
         /// </summary>
-        private static readonly string[] s_defaultRules = {
+        internal static readonly string[] s_defaultRules = {
             "PSAvoidAssignmentToAutomaticVariable",
             "PSUseToExportFieldsInManifest",
             "PSMisleadingBacktick",
@@ -141,7 +141,7 @@ public void StartScriptDiagnostics(ScriptFile[] filesToAnalyze)
             // If there's an existing task, we want to cancel it here;
             CancellationTokenSource cancellationSource = new();
             CancellationTokenSource oldTaskCancellation = Interlocked.Exchange(ref _diagnosticsCancellationTokenSource, cancellationSource);
-            if (oldTaskCancellation != null)
+            if (oldTaskCancellation is not null)
             {
                 try
                 {
@@ -194,7 +194,7 @@ public Task<string> FormatAsync(string scriptFileContents, Hashtable formatSetti
         /// <returns></returns>
         public async Task<string> GetCommentHelpText(string functionText, string helpLocation, bool forBlockComment)
         {
-            if (AnalysisEngine == null)
+            if (AnalysisEngine is null)
             {
                 return null;
             }
@@ -215,7 +215,7 @@ public async Task<string> GetCommentHelpText(string functionText, string helpLoc
         /// Get the most recent corrections computed for a given script file.
         /// </summary>
         /// <param name="uri">The URI string of the file to get code actions for.</param>
-        /// <returns>A threadsafe readonly dictionary of the code actions of the particular file.</returns>
+        /// <returns>A thread-safe readonly dictionary of the code actions of the particular file.</returns>
         public async Task<IReadOnlyDictionary<string, IEnumerable<MarkerCorrection>>> GetMostRecentCodeActionsForFileAsync(DocumentUri uri)
         {
             if (!_workspaceService.TryGetFile(uri, out ScriptFile file)
@@ -252,8 +252,8 @@ public void OnConfigurationUpdated(object _, LanguageServerSettings settings)
 
         private void EnsureEngineSettingsCurrent()
         {
-            if (_analysisEngineLazy == null
-                    || (_pssaSettingsFilePath != null
+            if (_analysisEngineLazy is null
+                    || (_pssaSettingsFilePath is not null
                         && !File.Exists(_pssaSettingsFilePath)))
             {
                 InitializeAnalysisEngineToCurrentSettings();
@@ -264,7 +264,7 @@ private void InitializeAnalysisEngineToCurrentSettings()
         {
             // We may be triggered after the lazy factory is set,
             // but before it's been able to instantiate
-            if (_analysisEngineLazy == null)
+            if (_analysisEngineLazy is null)
             {
                 _analysisEngineLazy = new Lazy<PssaCmdletAnalysisEngine>(InstantiateAnalysisEngine);
                 return;
@@ -327,7 +327,7 @@ private bool TryFindSettingsFile(out string settingsFilePath)
 
             settingsFilePath = _workspaceService?.ResolveWorkspacePath(configuredPath);
 
-            if (settingsFilePath == null
+            if (settingsFilePath is null
                 || !File.Exists(settingsFilePath))
             {
                 _logger.LogInformation($"Unable to find PSSA settings file at '{configuredPath}'. Loading default rules.");

src/PowerShellEditorServices/Services/Analysis/PssaCmdletAnalysisEngine.cs

@@ -75,7 +75,7 @@ public PssaCmdletAnalysisEngine Build(string pssaModulePath)
                 {
                     logger.LogDebug("Creating PSScriptAnalyzer runspace with module at: '{Path}'", pssaModulePath);
                     RunspacePool pssaRunspacePool = CreatePssaRunspacePool(pssaModulePath);
-                    PssaCmdletAnalysisEngine cmdletAnalysisEngine = new(logger, pssaRunspacePool, _settingsParameter ?? _rules);
+                    PssaCmdletAnalysisEngine cmdletAnalysisEngine = new(logger, pssaRunspacePool, _rules, _settingsParameter);
                     cmdletAnalysisEngine.LogAvailablePssaFeatures();
                     return cmdletAnalysisEngine;
                 }
@@ -105,28 +105,20 @@ public PssaCmdletAnalysisEngine Build(string pssaModulePath)
 
         private readonly RunspacePool _analysisRunspacePool;
 
-        private readonly object _settingsParameter;
+        internal readonly object _settingsParameter;
 
-        private readonly string[] _rulesToInclude;
+        internal readonly string[] _rulesToInclude;
 
         private PssaCmdletAnalysisEngine(
             ILogger logger,
             RunspacePool analysisRunspacePool,
-            string[] rulesToInclude)
-            : this(logger, analysisRunspacePool) => _rulesToInclude = rulesToInclude;
-
-        private PssaCmdletAnalysisEngine(
-            ILogger logger,
-            RunspacePool analysisRunspacePool,
-            object analysisSettingsParameter)
-            : this(logger, analysisRunspacePool) => _settingsParameter = analysisSettingsParameter;
-
-        private PssaCmdletAnalysisEngine(
-            ILogger logger,
-            RunspacePool analysisRunspacePool)
+            string[] rulesToInclude = default,
+            object analysisSettingsParameter = default)
         {
             _logger = logger;
             _analysisRunspacePool = analysisRunspacePool;
+            _rulesToInclude = rulesToInclude;
+            _settingsParameter = analysisSettingsParameter;
         }
 
         /// <summary>
@@ -228,9 +220,17 @@ public Task<ScriptFileMarker[]> AnalyzeScriptAsync(string scriptContent, Hashtab
             return GetSemanticMarkersFromCommandAsync(command);
         }
 
-        public PssaCmdletAnalysisEngine RecreateWithNewSettings(string settingsPath) => new(_logger, _analysisRunspacePool, settingsPath);
-
-        public PssaCmdletAnalysisEngine RecreateWithRules(string[] rules) => new(_logger, _analysisRunspacePool, rules);
+        public PssaCmdletAnalysisEngine RecreateWithNewSettings(string settingsPath) => new(
+            _logger,
+            _analysisRunspacePool,
+            rulesToInclude: null,
+            analysisSettingsParameter: settingsPath);
+
+        public PssaCmdletAnalysisEngine RecreateWithRules(string[] rules) => new(
+            _logger,
+            _analysisRunspacePool,
+            rulesToInclude: rules,
+            analysisSettingsParameter: null);
 
         #region IDisposable Support
         private bool disposedValue; // To detect redundant calls

test/PowerShellEditorServices.Test/Services/Symbols/PSScriptAnalyzerTests.cs

@@ -1,7 +1,6 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 
-using System.Linq;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.Extensions.Logging.Abstractions;
@@ -18,7 +17,7 @@ public class PSScriptAnalyzerTests
     {
         private readonly WorkspaceService workspaceService = new(NullLoggerFactory.Instance);
         private readonly AnalysisService analysisService;
-        private const string script = "function Get-Widgets {}";
+        private const string script = "function Do-Work {}";
 
         public PSScriptAnalyzerTests() => analysisService = new(
             NullLoggerFactory.Instance,
@@ -40,6 +39,13 @@ public class PSScriptAnalyzerTests
                 usesLegacyReadLine: false,
                 bundledModulePath: PsesHostFactory.BundledModulePath));
 
+        [Fact]
+        public void IncludesDefaultRules()
+        {
+            Assert.Null(analysisService.AnalysisEngine._settingsParameter);
+            Assert.Equal(AnalysisService.s_defaultRules, analysisService.AnalysisEngine._rulesToInclude);
+        }
+
         [Fact]
         public async Task CanLoadPSScriptAnalyzerAsync()
         {
@@ -51,10 +57,10 @@ public async Task CanLoadPSScriptAnalyzerAsync()
             Assert.Collection(violations,
             (actual) =>
             {
-                Assert.Single(actual.Corrections);
-                Assert.Equal("Singularized correction of 'Get-Widgets'", actual.Corrections.First().Name);
+                Assert.Empty(actual.Corrections);
                 Assert.Equal(ScriptFileMarkerLevel.Warning, actual.Level);
-                Assert.Equal("PSUseSingularNouns", actual.RuleName);
+                Assert.Equal("The cmdlet 'Do-Work' uses an unapproved verb.", actual.Message);
+                Assert.Equal("PSUseApprovedVerbs", actual.RuleName);
                 Assert.Equal("PSScriptAnalyzer", actual.Source);
             });
         }
@@ -96,7 +102,7 @@ await analysisService
                 },
                 (actual) =>
                 {
-                    Assert.Equal("PSUseSingularNouns", actual.RuleName);
+                    Assert.Equal("PSUseApprovedVerbs", actual.RuleName);
                     Assert.Equal("PSScriptAnalyzer", actual.Source);
                 });
         }