Enable CA1069 for ErrorCode and MessageID #54695
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jmarolf
approved these changes
Jul 8, 2021
333fred
previously approved these changes
Jul 8, 2021
|
fwiw, the analyzer seems to have known flakiness due to a concurrency issue. dotnet/roslyn-analyzers#5170 dotnet/roslyn-analyzers#3871 |
333fred
dismissed
their stale review
Given the concurrency issue, I think we should wait until that is resolved to merge this. I have no idea how flaky the analyzer is, but any amount is more than I'm willing to accept for something we already have enforcement on. It's not as nicely formatted, true, but our CI doesn't need any more sources of flakiness.
RikkiGibson
force-pushed
the
enable-ca1069
branch
from
c5fbe0f
to
fe010a5
Compare
|
I updated the roslyn-analyzers version to one which includes the concurrency fix in dotnet/roslyn-analyzers#5243. I think our roslyn-analyzers dependency is non-shipping so it should be fine to update to an "rc1" labeled version now instead of waiting for us to start publishing to an rc1 DARC feed for example. There were a few new warnings to fix in fe010a5. @sharwell @CyrusNajmabadi if you could please take a glance at those. |
this is correct. |
333fred
approved these changes
Jul 19, 2021
|
@dotnet/roslyn-compiler for a second review. |
jaredpar
approved these changes
Jul 21, 2021
sharwell
reviewed
Jul 21, 2021
| @@ -0,0 +1,4 @@ | |||
| # CSharp code style settings: | |||
There was a problem hiding this comment.
😕 Why are we adding a new file here?
There was a problem hiding this comment.
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.
| # CSharp code style settings: | ||
| [*.cs] | ||
| # CA1069: Enums should not have duplicate values | ||
| dotnet_diagnostic.CA1069.severity = warning |
There was a problem hiding this comment.
💡 These lines should appear in .globalconfig instead of .editorconfig for performance and consistent application
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
For intentional conditional application, .editorconfig is fine.
| @@ -14,7 +13,6 @@ namespace Microsoft.CodeAnalysis.Interactive | |||
| { | |||
| internal static class InteractiveHostEntryPoint | |||
| { | |||
| [SupportedOSPlatform("windows")] | |||
There was a problem hiding this comment.
😕 I'm not sure what this has to do with "Enums should not have duplicate values"
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
That seems reasonable to me. Thanks
Comment on lines
+67
to
+68
| var disposable = await _progressTracker.AddSingleItemAsync(cancellationToken).ConfigureAwait(false); | ||
| await using var _ = disposable.ConfigureAwait(false); |
There was a problem hiding this comment.
💡 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).
There was a problem hiding this comment.
I chatted with @CyrusNajmabadi offline about this and he expressed a preference to factor it this way rather than as a single line.
|
I did find the output from running this analyzer on 94 new warnings |
jcouv
reviewed
Jul 22, 2021
Thanks for providing that output. I was wondering about that. The only example I could think of was |
ghost
added this to the
333fred
added a commit
to 333fred/roslyn
that referenced
this pull request
Jul 23, 2021
* upstream/main: (249 commits) Switch back queue name to default (dotnet#55064) Fix 'code model' with file scoped namespaces Map documents to be reanalyzed back from compile-time to design-time documents (dotnet#55054) Update MSBuild Workspace test projects target framework Enable CA1069 for ErrorCode and MessageID (dotnet#54695) Dev16->17 updates Update global.json Record completion of "parameterless struct constructor" feature (dotnet#55049) Generalize rude edit messages to be applicable to both Hot Reload and EnC (dotnet#55012) Update azure-pipelines-official.yml Update azure-pipelines-integration.yml Merge pull request dotnet#54992 from jaredpar/so-big Parameterless struct constructors: Remaining work items (dotnet#54811) Update docs/wiki/Diagnosing-Project-System-Build-Errors.md update queue name Dev16->17 changes Fix test Fix 'move type' with file scoped namespaces Fix 'match folder and namespace' with file scoped namespaces Log NFW ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CA1069: Enums should not have duplicate values
Including an .editorconfig under the Errors path lets us add this check specifically for ErrorCode, MessageId and XmlParseErrorCode, all of which we expect to not use any duplicate IDs. Thanks @jmarolf for the suggestion.
We already have this enforcement for ErrorCode and possibly for MessageID, but the quality of the error message from this analyzer is a lot better than what we get in
DiagnosticTest.NoDuplicates.Opened #54696 to demonstrate the error experience in CI.
Compare the NoDuplicates error message:
with the analyzer warning:
> dotnet build .\src\Compilers\CSharp\Portable /p:RunAnalyzersDuringBuild=true...src\Compilers\CSharp\Portable\Errors\ErrorCode.cs(1959,9): warning CA1069: The enum member 'ERR_CantConvAnonMethReturnType' has the same constant value '8933' as member 'HDN_DuplicateWithGlobalUsing' [src\Compilers\CSharp\Portable\Microsoft.CodeAnalysis.CSharp.csproj]