Skip to content

Enable and fix many .NET Code Analysis warnings #1533

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.

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

Merged
merged 10 commits into from
Aug 3, 2021
Merged
27 changes: 27 additions & 0 deletions .editorconfig
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,33 @@ trim_trailing_whitespace = true
csharp_space_before_open_square_brackets = true
csharp_space_after_keywords_in_control_flow_statements = true

# CS0168: The variable 'var' is declared but never used
dotnet_diagnostic.CS0168.severity = error
# CS0169: The private field 'class member' is never used
dotnet_diagnostic.CS0169.severity = error
# CS0219: The variable 'variable' is assigned but its value is never used
dotnet_diagnostic.CS0219.severity = error
# CS0414: The private field 'field' is assigned but its value is never used
dotnet_diagnostic.CS0414.severity = error
# CA1068: CancellationToken parameters must come last
dotnet_diagnostic.CA1068.severity = error
# CA1822: Mark members as static
dotnet_diagnostic.CA1822.severity = error
# CA1823: Avoid unused private fields
dotnet_diagnostic.CA1823.severity = error
# CA2007: Do not directly await a Task
dotnet_diagnostic.CA2007.severity = error
# CA2016: Forward the CancellationToken parameter to methods that take one
dotnet_diagnostic.CA2016.severity = error
# All maintainability issues (dead code etc.)
# See: https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/maintainability-warnings
dotnet_analyzer_diagnostic.category-Maintainability.severity = error
# VSTHRD002: Synchronously waiting on tasks or awaiters may cause deadlocks
# TODO: Fix all of these issues and explicitly ignore the intentional ones.
dotnet_diagnostic.VSTHRD002.severity = silent
# VSTHRD200: Use "Async" suffix for awaitable methods
dotnet_diagnostic.VSTHRD200.severity = silent

[*.{json}]
indent_size = 2
trim_trailing_whitespace = true
Expand Down
3 changes: 3 additions & 0 deletions PowerShellEditorServices.Common.props
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,8 @@
<RepositoryType>git</RepositoryType>
<RepositoryUrl>https://github.com/PowerShell/PowerShellEditorServices</RepositoryUrl>
<DebugType>portable</DebugType>
<!-- See: https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/overview -->
<EnableNETAnalyzers>true</EnableNETAnalyzers>
<!-- TODO: Enable `EnforceCodeStyleInBuild` -->
</PropertyGroup>
</Project>
6 changes: 3 additions & 3 deletions src/PowerShellEditorServices.Hosting/EditorServicesLoader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ public void Dispose()
// This is not high priority, since the PSES process shouldn't be reused
}

private void LoadEditorServices()
private static void LoadEditorServices()
{
// This must be in its own method, since the actual load happens when the calling method is called
// The call within this method is therefore a total no-op
Expand Down Expand Up @@ -317,7 +317,7 @@ private void LogHostInformation()
LogOperatingSystemDetails();
}

private string GetPSOutputEncoding()
private static string GetPSOutputEncoding()
{
using (var pwsh = SMA.PowerShell.Create())
{
Expand Down Expand Up @@ -346,7 +346,7 @@ private void LogOperatingSystemDetails()
");
}

private string GetOSArchitecture()
private static string GetOSArchitecture()
{
#if CoreCLR
if (Environment.OSVersion.Platform != PlatformID.Win32NT)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,6 @@

<ItemGroup>
<ProjectReference Include="..\PowerShellEditorServices\PowerShellEditorServices.csproj" PrivateAssets="all" />
<PackageReference Include="Microsoft.CodeAnalysis.FxCopAnalyzers" Version="3.3.2">
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>
</ItemGroup>

<ItemGroup Condition="'$(TargetFramework)'=='net461'">
Expand Down
27 changes: 9 additions & 18 deletions src/PowerShellEditorServices.VSCode/CustomViews/CustomViewBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -28,40 +28,31 @@ public CustomViewBase(
this.languageServer = languageServer;
}

internal async Task CreateAsync()
{
await languageServer.SendRequestAsync(
internal Task CreateAsync() =>
languageServer.SendRequestAsync(
NewCustomViewRequest.Method,
new NewCustomViewRequest
{
Id = this.Id,
Title = this.Title,
ViewType = this.ViewType,
}
);
}
});

public async Task Show(ViewColumn viewColumn)
{
await languageServer.SendRequestAsync(
public Task Show(ViewColumn viewColumn) =>
languageServer.SendRequestAsync(
ShowCustomViewRequest.Method,
new ShowCustomViewRequest
{
Id = this.Id,
ViewColumn = viewColumn
}
);
}
});

public async Task Close()
{
await languageServer.SendRequestAsync(
public Task Close() =>
languageServer.SendRequestAsync(
CloseCustomViewRequest.Method,
new CloseCustomViewRequest
{
Id = this.Id,
}
);
}
});
}
}
35 changes: 13 additions & 22 deletions src/PowerShellEditorServices.VSCode/CustomViews/HtmlContentView.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,51 +21,42 @@ public HtmlContentView(
{
}

public async Task SetContentAsync(string htmlBodyContent)
{
await languageServer.SendRequestAsync(
public Task SetContentAsync(string htmlBodyContent) =>
languageServer.SendRequestAsync(
SetHtmlContentViewRequest.Method,
new SetHtmlContentViewRequest
{
Id = this.Id,
HtmlContent = new HtmlContent { BodyContent = htmlBodyContent }
}
);
}

public async Task SetContentAsync(HtmlContent htmlContent)
{
HtmlContent validatedContent =
new HtmlContent()
{
BodyContent = htmlContent.BodyContent,
JavaScriptPaths = this.GetUriPaths(htmlContent.JavaScriptPaths),
StyleSheetPaths = this.GetUriPaths(htmlContent.StyleSheetPaths)
};

await languageServer.SendRequestAsync(
public Task SetContentAsync(HtmlContent htmlContent) =>
languageServer.SendRequestAsync(
SetHtmlContentViewRequest.Method,
new SetHtmlContentViewRequest
{
Id = this.Id,
HtmlContent = validatedContent
HtmlContent = new HtmlContent()
{
BodyContent = htmlContent.BodyContent,
JavaScriptPaths = HtmlContentView.GetUriPaths(htmlContent.JavaScriptPaths),
StyleSheetPaths = HtmlContentView.GetUriPaths(htmlContent.StyleSheetPaths)
}
}
);
}

public async Task AppendContentAsync(string appendedHtmlBodyContent)
{
await languageServer.SendRequestAsync(
public Task AppendContentAsync(string appendedHtmlBodyContent) =>
languageServer.SendRequestAsync(
AppendHtmlContentViewRequest.Method,
new AppendHtmlContentViewRequest
{
Id = this.Id,
AppendedHtmlBodyContent = appendedHtmlBodyContent
}
);
}

private string[] GetUriPaths(string[] filePaths)
private static string[] GetUriPaths(string[] filePaths)
{
return
filePaths?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ public async Task<IHtmlContentView> CreateHtmlContentViewAsync(string viewTitle)
viewTitle,
languageServer);

await htmlView.CreateAsync();
await htmlView.CreateAsync().ConfigureAwait(false);
this.AddView(htmlView);

return htmlView;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ public object GetService(Type serviceType)
/// In .NET Framework, this returns null.
/// </summary>
/// <returns>The assembly load context used for loading PSES, or null in .NET Framework.</returns>
public object GetPsesAssemblyLoadContext()
public static object GetPsesAssemblyLoadContext()
{
if (!VersionUtils.IsNetCore)
{
Expand All @@ -172,7 +172,7 @@ public object GetPsesAssemblyLoadContext()
/// </summary>
/// <param name="assemblyPath">The absolute path of the assembly to load.</param>
/// <returns>The loaded assembly object.</returns>
public Assembly LoadAssemblyInPsesLoadContext(string assemblyPath)
public static Assembly LoadAssemblyInPsesLoadContext(string assemblyPath)
{
if (!VersionUtils.IsNetCore)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ public async Task<string> PromptInputAsync(string message)
new ShowInputPromptRequest
{
Name = message,
}).Returning<ShowInputPromptResponse>(CancellationToken.None);
}).Returning<ShowInputPromptResponse>(CancellationToken.None).ConfigureAwait(false);

if (response.PromptCancelled)
{
Expand All @@ -142,7 +142,7 @@ public async Task<IReadOnlyList<string>> PromptMultipleSelectionAsync(string mes
Message = message,
Choices = choiceDetails,
DefaultChoices = defaultChoiceIndexes?.ToArray(),
}).Returning<ShowChoicePromptResponse>(CancellationToken.None);
}).Returning<ShowChoicePromptResponse>(CancellationToken.None).ConfigureAwait(false);

if (response.PromptCancelled)
{
Expand All @@ -168,7 +168,7 @@ public async Task<string> PromptSelectionAsync(string message, IReadOnlyList<Pro
Message = message,
Choices = choiceDetails,
DefaultChoices = defaultChoiceIndex > -1 ? new[] { defaultChoiceIndex } : null,
}).Returning<ShowChoicePromptResponse>(CancellationToken.None);
}).Returning<ShowChoicePromptResponse>(CancellationToken.None).ConfigureAwait(false);

if (response.PromptCancelled)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ public IReadOnlyList<IEditorScriptFile> GetOpenedFiles()
return files.AsReadOnly();
}

private IEditorScriptFile GetEditorFileFromScriptFile(ScriptFile file)
private static IEditorScriptFile GetEditorFileFromScriptFile(ScriptFile file)
{
return new EditorScriptFile(file);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">

<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), PowerShellEditorServices.Common.props))\PowerShellEditorServices.Common.props" />

<PropertyGroup>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@ private PowerShellResult InvokePowerShell(PSCommand command)
/// </summary>
/// <param name="powershell">The PowerShell instance to execute.</param>
/// <returns>The output of PowerShell execution.</returns>
private Collection<PSObject> InvokePowerShellWithModulePathPreservation(System.Management.Automation.PowerShell powershell)
private static Collection<PSObject> InvokePowerShellWithModulePathPreservation(System.Management.Automation.PowerShell powershell)
{
using (PSModulePathPreserver.Take())
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public PesterCodeLensProvider(ConfigurationService configurationService)
/// <param name="pesterSymbol">The Pester symbol to get CodeLenses for.</param>
/// <param name="scriptFile">The script file the Pester symbol comes from.</param>
/// <returns>All CodeLenses for the given Pester symbol.</returns>
private CodeLens[] GetPesterLens(PesterSymbolReference pesterSymbol, ScriptFile scriptFile)
private static CodeLens[] GetPesterLens(PesterSymbolReference pesterSymbol, ScriptFile scriptFile)
{
string word = pesterSymbol.Command == PesterCommandType.It ? "test" : "tests";
var codeLensResults = new CodeLens[]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public CodeLens ResolveCodeLens(CodeLens codeLens, ScriptFile scriptFile)
ScriptFile[] references = _workspaceService.ExpandScriptReferences(
scriptFile);

SymbolReference foundSymbol = _symbolsService.FindFunctionDefinitionAtLocation(
SymbolReference foundSymbol = SymbolsService.FindFunctionDefinitionAtLocation(
scriptFile,
codeLens.Range.Start.Line + 1,
codeLens.Range.Start.Character + 1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public async Task<List<Breakpoint>> GetBreakpointsAsync()
// Legacy behavior
PSCommand psCommand = new PSCommand();
psCommand.AddCommand(@"Microsoft.PowerShell.Utility\Get-PSBreakpoint");
IEnumerable<Breakpoint> breakpoints = await _powerShellContextService.ExecuteCommandAsync<Breakpoint>(psCommand);
IEnumerable<Breakpoint> breakpoints = await _powerShellContextService.ExecuteCommandAsync<Breakpoint>(psCommand).ConfigureAwait(false);
return breakpoints.ToList();
}

Expand Down Expand Up @@ -133,7 +133,7 @@ public async Task<IEnumerable<BreakpointDetails>> SetBreakpointsAsync(string esc
if (psCommand != null)
{
IEnumerable<Breakpoint> setBreakpoints =
await _powerShellContextService.ExecuteCommandAsync<Breakpoint>(psCommand);
await _powerShellContextService.ExecuteCommandAsync<Breakpoint>(psCommand).ConfigureAwait(false);
configuredBreakpoints.AddRange(
setBreakpoints.Select(BreakpointDetails.Create));
}
Expand Down Expand Up @@ -210,7 +210,7 @@ public async Task<IEnumerable<CommandBreakpointDetails>> SetCommandBreakpoints(I
if (psCommand != null)
{
IEnumerable<Breakpoint> setBreakpoints =
await _powerShellContextService.ExecuteCommandAsync<Breakpoint>(psCommand);
await _powerShellContextService.ExecuteCommandAsync<Breakpoint>(psCommand).ConfigureAwait(false);
configuredBreakpoints.AddRange(
setBreakpoints.Select(CommandBreakpointDetails.Create));
}
Expand Down Expand Up @@ -301,7 +301,7 @@ public async Task RemoveBreakpointsAsync(IEnumerable<Breakpoint> breakpoints)
psCommand.AddCommand(@"Microsoft.PowerShell.Utility\Remove-PSBreakpoint");
psCommand.AddParameter("Id", breakpoints.Select(b => b.Id).ToArray());

await _powerShellContextService.ExecuteCommandAsync<object>(psCommand);
await _powerShellContextService.ExecuteCommandAsync<object>(psCommand).ConfigureAwait(false);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,6 @@ internal class DebugService
private StackFrameDetails[] stackFrameDetails;
private readonly PropertyInfo invocationTypeScriptPositionProperty;

private static int breakpointHitCounter;

private readonly SemaphoreSlim debugInfoHandle = AsyncUtils.CreateSimpleLockingSemaphore();
#endregion

Expand Down Expand Up @@ -190,7 +188,7 @@ public async Task<BreakpointDetails[]> SetLineBreakpointsAsync(
return await dscBreakpoints.SetLineBreakpointsAsync(
this.powerShellContext,
escapedScriptPath,
breakpoints);
breakpoints).ConfigureAwait(false);
}

/// <summary>
Expand All @@ -208,7 +206,9 @@ public async Task<CommandBreakpointDetails[]> SetCommandBreakpointsAsync(
if (clearExisting)
{
// Flatten dictionary values into one list and remove them all.
await _breakpointService.RemoveBreakpointsAsync((await _breakpointService.GetBreakpointsAsync()).Where( i => i is CommandBreakpoint)).ConfigureAwait(false);
await _breakpointService.RemoveBreakpointsAsync(
(await _breakpointService.GetBreakpointsAsync().ConfigureAwait(false))
.Where( i => i is CommandBreakpoint)).ConfigureAwait(false);
}

if (breakpoints.Length > 0)
Expand Down Expand Up @@ -854,7 +854,7 @@ private async Task FetchStackFramesAsync(string scriptNameOverride)
}
}

private string TrimScriptListingLine(PSObject scriptLineObj, ref int prefixLength)
private static string TrimScriptListingLine(PSObject scriptLineObj, ref int prefixLength)
{
string scriptLine = scriptLineObj.ToString();

Expand Down Expand Up @@ -906,7 +906,7 @@ await this.powerShellContext.ExecuteCommandAsync<PSObject>(
string.Join(
Environment.NewLine,
scriptListingLines
.Select(o => this.TrimScriptListingLine(o, ref linePrefixLength))
.Select(o => DebugService.TrimScriptListingLine(o, ref linePrefixLength))
.Where(s => s != null));

this.temporaryScriptListingPath =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ public async Task<DisconnectResponse> Handle(DisconnectArguments request, Cancel

#pragma warning disable CS4014
// Trigger the clean up of the debugger. No need to wait for it.
Task.Run(_psesDebugServer.OnSessionEnded);
Task.Run(_psesDebugServer.OnSessionEnded, cancellationToken);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rjmholt Should this one be explicitly CancellationToken.None? Presumably we probably don't want to the debug server's clean up to be cancelled.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that said if it's cancelled here then we don't return the DisconnectResponse so I suppose it would get cancelled again?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think None seems reasonable

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops forgot about this 🤷

#pragma warning restore CS4014

return new DisconnectResponse();
Expand Down
Loading