Enable and fix many .NET Code Analysis warnings #1533
Conversation
Since all our projects target the `Microsoft.NET.Sdk` we can use roslyn's built-in analyzers instead of the out-of-date package (we could update that instead, but this is the modern method).
andyleejordan
added
Area-Debugging
Issue-Enhancement
andyleejordan
force-pushed
the
andschwa/dotnet-analysis
branch
3 times, most recently
from
4f02fb6 to
f1d920c
Compare
andyleejordan
changed the title
andyleejordan
changed the title
src/PowerShellEditorServices.VSCode/CustomViews/CustomViewBase.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices.VSCode/CustomViews/CustomViewBase.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices.VSCode/CustomViews/CustomViewBase.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices.VSCode/CustomViews/HtmlContentView.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices.VSCode/CustomViews/HtmlContentView.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/PowerShellContext/Session/PSReadLinePromptContext.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/PowerShellContext/Session/PSReadLinePromptContext.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/PowerShellContext/Session/PromptNest.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/PowerShellContext/Session/PromptNest.cs
Outdated
Show resolved
Hide resolved
| @@ -62,7 +62,7 @@ internal static class AstOperations | |||
| ILogger logger, | |||
| CancellationToken cancellationToken) | |||
| { | |||
| if (!s_completionHandle.Wait(0)) | |||
| if (!s_completionHandle.Wait(0, cancellationToken)) | |||
There was a problem hiding this comment.
This shouldn't need the cancellation passed through, since it's a Wait(0) -- the token should never get a chance to cancel
There was a problem hiding this comment.
Tell that to the compiler that complained.
There was a problem hiding this comment.
I suppose it can't hurt here, but I don't like the idea of misleading code -- the cancellation token definitely has no utility here
There was a problem hiding this comment.
We could pass CancellationToken.None instead (which says explicitly this won't be cancelled).
There was a problem hiding this comment.
Nah I think what you've got works fine
andyleejordan
force-pushed
the
andschwa/dotnet-analysis
branch
from
5872caf to
d07af0f
Compare
| @@ -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); | |||
There was a problem hiding this comment.
@rjmholt Should this one be explicitly CancellationToken.None? Presumably we probably don't want to the debug server's clean up to be cancelled.
There was a problem hiding this comment.
But that said if it's cancelled here then we don't return the DisconnectResponse so I suppose it would get cancelled again?
There was a problem hiding this comment.
Yeah I think None seems reasonable
There was a problem hiding this comment.
Oops forgot about this 🤷
| @@ -14,9 +14,6 @@ namespace Microsoft.PowerShell.EditorServices.Services.TextDocument | |||
| /// </summary> | |||
| internal static class TokenOperations | |||
| { | |||
| // Region kinds to align with VSCode's region kinds | |||
| private const string RegionKindComment = "comment"; | |||
| private const string RegionKindRegion = "region"; | |||
There was a problem hiding this comment.
These unused fields smell like olllld code to me.
andyleejordan
force-pushed
the
andschwa/dotnet-analysis
branch
from
d07af0f to
52501c8
Compare
While easy to fix, it’s a noisy change that only affects names.
These are all compiler level 3 warnings.
Because we do this all over the place, generating a lot of warnings, and it's often an intentional behavior.
andyleejordan
force-pushed
the
andschwa/dotnet-analysis
branch
from
52501c8 to
707cad6
Compare
Pulling this out of #1532, enabling and fixing the code analysis warnings that matter to us one-by-one, and treating them as build errors. I should note that there are many other warnings we're ignoring for now and should fix eventually.