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 (and test) regression with PSScriptAnalyzer default rules #1873

Merged
merged 1 commit into from
Aug 5, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
Fix (and test) regression with PSScriptAnalyzer default rules 
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.
  • Loading branch information
@andyleejordan
andyleejordan committed Aug 5, 2022
commit 1e009dfce80a1f455fb8ddaf5154a11b35a2aab0
42 changes: 21 additions & 21 deletions src/PowerShellEditorServices/Services/Analysis/AnalysisService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand All @@ -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",
Expand Down Expand Up @@ -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
{
Expand Down Expand Up @@ -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;
}
Expand All @@ -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)
Expand Down Expand Up @@ -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();
Expand All @@ -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;
Expand Down Expand Up @@ -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.");
Expand Down
36 changes: 18 additions & 18 deletions src/PowerShellEditorServices/Services/Analysis/PssaCmdletAnalysisEngine.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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>
Expand Down Expand Up @@ -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
Expand Down
18 changes: 12 additions & 6 deletions test/PowerShellEditorServices.Test/Services/Symbols/PSScriptAnalyzerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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,
Expand All @@ -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()
{
Expand All @@ -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);
});
}
Expand Down Expand Up @@ -96,7 +102,7 @@ await analysisService
},
(actual) =>
{
Assert.Equal("PSUseSingularNouns", actual.RuleName);
Assert.Equal("PSUseApprovedVerbs", actual.RuleName);
Assert.Equal("PSScriptAnalyzer", actual.Source);
});
}
Expand Down