Skip to content

Enable CA1069 for ErrorCode and MessageID #54695

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

Merged
RikkiGibson merged 7 commits into from
Jul 22, 2021
Merged

Enable CA1069 for ErrorCode and MessageID #54695

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0e93230
Enable CA1069 for ErrorCode and MessageID
RikkiGibson Jul 8, 2021
eee1dac
Update Versions.props
RikkiGibson Jul 16, 2021
3e3e5d6
Merge branch 'main' of github.com:dotnet/roslyn into enable-ca1069
RikkiGibson Jul 17, 2021
fe010a5
Fix new analyzer warnings
RikkiGibson Jul 17, 2021
e9c83f4
Split complex expressions to multiple statements
RikkiGibson Jul 19, 2021
1793d60
Remove PlatformAttributes.cs
RikkiGibson Jul 22, 2021
805d1ac
Remove usages of PlatformAttributes.cs
RikkiGibson Jul 22, 2021
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
2 changes: 1 addition & 1 deletion eng/Versions.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
<PropertyGroup>
<!-- Versions used by several individual references below -->
<RoslynDiagnosticsNugetPackageVersion>3.3.3-beta1.21105.3</RoslynDiagnosticsNugetPackageVersion>
<MicrosoftCodeAnalysisNetAnalyzersVersion>6.0.0-preview1.21054.10</MicrosoftCodeAnalysisNetAnalyzersVersion>
<MicrosoftCodeAnalysisNetAnalyzersVersion>6.0.0-rc1.21366.2</MicrosoftCodeAnalysisNetAnalyzersVersion>
<MicrosoftCodeAnalysisTestingVersion>1.1.0-beta1.21322.2</MicrosoftCodeAnalysisTestingVersion>
<CodeStyleAnalyzerVersion>3.10.0</CodeStyleAnalyzerVersion>
<VisualStudioEditorPackagesVersion>16.10.230</VisualStudioEditorPackagesVersion>
Expand Down
4 changes: 4 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/.editorconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# CSharp code style settings:
Copy link
Contributor

@sharwell sharwell Jul 21, 2021

Choose a reason for hiding this comment

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

😕 Why are we adding a new file here?

Copy link
Member Author

@RikkiGibson RikkiGibson Jul 21, 2021

Choose a reason for hiding this comment

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

We want to run the analyzer specifically on the enums in this file. We're not interested in having the warning on the rest of the enums in the compiler at this time.

[*.cs]
# CA1069: Enums should not have duplicate values
dotnet_diagnostic.CA1069.severity = warning
Copy link
Contributor

@sharwell sharwell Jul 21, 2021

Choose a reason for hiding this comment

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

💡 These lines should appear in .globalconfig instead of .editorconfig for performance and consistent application

Copy link
Member Author

@RikkiGibson RikkiGibson Jul 21, 2021

Choose a reason for hiding this comment

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

The compiler team doesn't currently desire consistent application of this warning. Is there a change that we should make here to improve performance which doesn't affect which files are subject to the new warning?

Copy link
Contributor

@sharwell sharwell Jul 21, 2021

Choose a reason for hiding this comment

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

For intentional conditional application, .editorconfig is fine.

94 changes: 0 additions & 94 deletions src/Compilers/Core/Portable/InternalUtilities/PlatformAttributes.cs
View file
Open in desktop

This file was deleted.

4 changes: 0 additions & 4 deletions src/Interactive/HostProcess/InteractiveHost32.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,6 @@
<ProjectReference Include="..\Host\Microsoft.CodeAnalysis.InteractiveHost.csproj" />
</ItemGroup>

<ItemGroup>
<Compile Include="..\..\Compilers\Core\Portable\InternalUtilities\PlatformAttributes.cs" Link="Utilities\PlatformAttributes.cs" />
</ItemGroup>

<!--
InteractiveHost32 is deployed to the same directory as InteractiveHost64 as it shares the same dependencies.

Expand Down
4 changes: 0 additions & 4 deletions src/Interactive/HostProcess/InteractiveHost64.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,6 @@
<ProjectReference Include="..\Host\Microsoft.CodeAnalysis.InteractiveHost.csproj" />
</ItemGroup>

<ItemGroup>
<Compile Include="..\..\Compilers\Core\Portable\InternalUtilities\PlatformAttributes.cs" Link="Utilities\PlatformAttributes.cs" />
</ItemGroup>

<Target Name="PublishProjectOutputGroup" DependsOnTargets="Publish" Returns="@(_PublishedFiles)">
<ItemGroup>
<!-- Need to include and then update items (https://github.com/microsoft/msbuild/issues/1053) -->
Expand Down
4 changes: 0 additions & 4 deletions src/Interactive/HostProcess/InteractiveHostEntryPoint.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

using System;
using System.Runtime.InteropServices;
using System.Runtime.Versioning;
using System.Threading;
using System.Threading.Tasks;
using System.Windows.Forms;
Expand All @@ -14,7 +13,6 @@ namespace Microsoft.CodeAnalysis.Interactive
{
internal static class InteractiveHostEntryPoint
{
[SupportedOSPlatform("windows")]
Copy link
Contributor

@sharwell sharwell Jul 21, 2021

Choose a reason for hiding this comment

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

😕 I'm not sure what this has to do with "Enums should not have duplicate values"

Copy link
Member Author

@RikkiGibson RikkiGibson Jul 21, 2021

Choose a reason for hiding this comment

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

I had to update the roslyn-analyzers version in this PR which introduced new warnings. This looked to me like the most straightforward way to resolve the warning in this file. I speculate that the new analyzer infers a "default" SupportedOSPlatform based on the target framework of the project.

Copy link
Member

@jcouv jcouv Jul 22, 2021

Choose a reason for hiding this comment

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

Could you clarify why removing SupportedOSPlatform is the solution?

Just to recap, the diagnostic was "src\Interactive\HostProcess\InteractiveHostEntryPoint.cs(23,113): error CA1416: (NETCORE_ENGINEERING_TELEMETRY=Build) This call site is reachable on: 'windows' all versions. 'InteractiveHostEntryPoint.ErrorMode.SEM_NOGPFAULTERRORBOX' is only supported on: 'Windows' 7.0 and later."

Looking at the doc on CA1416, it's not clear why removing the SupportedOSPlatform would even fix this diagnostic.
I would have expected that we needed to specify a minimum version of windows instead. Something like [SupportedOSPlatform("windows10.0.1903")] (with some proper version where the SEM_NOGPFAULTERRORBOX was added).

Copy link
Member Author

@RikkiGibson RikkiGibson Jul 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Basically an analyzer thinks it's not OK to use an enum in one of these methods because the enum (which was also declared in this project) has a "supported os platform" value that is more restrictive than the caller's value. The enum's value is being inferred as "windows7" from the project while the caller's was explicitly set to "windows". So deleting these attributes causes a consistently restrictive "supported os platform" value to be associated with all the symbols in the project.

Copy link
Member

@jcouv jcouv Jul 22, 2021

Choose a reason for hiding this comment

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

Got it. So removing the attribute is functionally equivalent to changing it [SupportedOSPlatform("windows7")] then (inferred from APIs used in that method)?

Should we revert other parts of commit c756744 too then?

Copy link
Member Author

@RikkiGibson RikkiGibson Jul 22, 2021

Choose a reason for hiding this comment

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

That seems reasonable to me. Thanks

private static async Task<int> Main(string[] args)
{
FatalError.Handler = FailFast.OnFatalException;
Expand Down Expand Up @@ -53,11 +51,9 @@ private static async Task<int> Main(string[] args)
}
}

[SupportedOSPlatform("windows")]
[DllImport("kernel32", PreserveSig = true)]
internal static extern ErrorMode SetErrorMode(ErrorMode mode);

[SupportedOSPlatform("windows")]
[DllImport("kernel32", PreserveSig = true)]
internal static extern ErrorMode GetErrorMode();

Expand Down
3 changes: 2 additions & 1 deletion src/Workspaces/Core/Portable/FindSymbols/FindLiterals/FindLiteralsSearchEngine.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ public FindLiteralsSearchEngine(

public async Task FindReferencesAsync(CancellationToken cancellationToken)
{
await using var _ = await _progressTracker.AddSingleItemAsync(cancellationToken).ConfigureAwait(false);
var disposable = await _progressTracker.AddSingleItemAsync(cancellationToken).ConfigureAwait(false);
await using var _ = disposable.ConfigureAwait(false);

if (_searchKind != SearchKind.None)
{
Expand Down
3 changes: 2 additions & 1 deletion src/Workspaces/Core/Portable/FindSymbols/FindReferences/FindReferencesSearchEngine.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ public async Task FindReferencesAsync(ISymbol symbol, CancellationToken cancella
await _progress.OnStartedAsync(cancellationToken).ConfigureAwait(false);
try
{
await using var _ = await _progressTracker.AddSingleItemAsync(cancellationToken).ConfigureAwait(false);
var disposable = await _progressTracker.AddSingleItemAsync(cancellationToken).ConfigureAwait(false);
await using var _ = disposable.ConfigureAwait(false);
Comment on lines +67 to +68
Copy link
Contributor

@sharwell sharwell Jul 21, 2021

Choose a reason for hiding this comment

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

💡 The separate form is typically for cases where disposable needs to be used. For this code, I would typically combine these into a single line (it's a long line, but that's acceptable).

Copy link
Member Author

@RikkiGibson RikkiGibson Jul 21, 2021

Choose a reason for hiding this comment

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

I chatted with @CyrusNajmabadi offline about this and he expressed a preference to factor it this way rather than as a single line.


// For the starting symbol, always cascade up and down the inheritance hierarchy.
var symbols = await DetermineAllSymbolsAsync(
Expand Down