dotnet
Enable CA1052 (static holder types should be static)
@mavasani There are flagged public APIs here. I'll add suppressions.
Meanwhile, would you like me to open an issue for any of these suggesting a breaking change to discuss in API review?
Meanwhile, would you like me to open an issue for any of these suggesting a breaking change to discuss in API review?
IMO, it will be difficult to justify such a breaking change for a public API. @333fred for his views.
@mavasani @333fred nothing good can come of people instantiating these types. I don't think we have any obligation to keep that working.
@mavasani @333fred nothing good can come of people instantiating these types. I don't think we have any obligation to keep that working.
Not against it, but definitely needs to go through a public API review as it is a technically a breaking change.
I guess I'm not convinced of the need to break those users.
Instantiating these types would indicate something completely broken in their code. First, i'm skeptical this would even exist anywhere. Second, it doesn't seem like this code could even do anything (you can't access statics through instance values for example). So it would have ot exist, and be unused for any purpose. Compat around that seems unnecessary. But we can certainly bring to discussion.
I guess I'm ok having that discussion. @Youssef1313, can you please open an issue?
roslyn-CI (Correctness Correctness_Analyzers) is green. This doesn't seem good. SymbolAnnotation should have failed since #65829 isn't yet merged.
@mavasani I did a local build of Workspaces via dotnet build src/Workspaces/Core/Portable/Microsoft.CodeAnalysis.Workspaces.csproj --configuration Debug --no-incremental -bl -p:RunAnalyzersDuringBuild=true -p:TreatWarningsAsErrors=true, and no diagnostic is reported for CA1052 for the violation of SymbolAnnotation. What I looked at in the binlog:
- Confirm that
C:\Users\PC\Desktop\roslyn\src\Workspaces\SharedUtilitiesAndExtensions\Compiler\Core\Simplification\SymbolAnnotation.csis passed to csc. - Confirm
/analyzerconfig:C:\Users\PC\Desktop\roslyn\eng\config\globalconfigs\Common.globalconfig - Confirm
/analyzer:C:\Users\PC\.nuget\packages\microsoft.codeanalysis.netanalyzers\7.0.0-preview1.22218.1\analyzers\dotnet\cs\Microsoft.CodeAnalysis.CSharp.NetAnalyzers.dlland/analyzer:C:\Users\PC\.nuget\packages\microsoft.codeanalysis.netanalyzers\7.0.0-preview1.22218.1\analyzers\dotnet\cs\Microsoft.CodeAnalysis.NetAnalyzers.dll - The class
SymbolAnnotationdoesn't seem to have anything "special" other than a constant field. I tried a unit test in Roslyn.sln locally with a constant field and it is working properly.
I guess I'm ok having that discussion. @Youssef1313, can you please open an issue?
https://github.com/dotnet/roslyn/issues/65909
@mavasani This is ready for review, pending a question on why CI didn't fail for SymbolAnnotation despite a similar analyzer test in roslyn-analyzers is passing.
Enable CA1052 (static holder types should be static)
Why would we want to do this? I see nothing wrong in a non-static type with only static explicitly declared members.
Enable CA1052 (static holder types should be static)
Why would we want to do this? I see nothing wrong in a non-static type with only static explicitly declared members.
@AlekseyTs The analyzer caught cases where the class should really have been static. https://github.com/dotnet/roslyn/issues/65909. The rate of false positives should be quite low and not that noisy, and source suppressions are suitable for such cases.
The analyzer caught cases where the class should really have been static. #65909
I do not think there was a real harm there, even if we decided to approve the suggested API change. I find this rule useless in general. It might provide some very small guidance around public API shape, but I prefer to not deal with this noise under Compilers. Could we exclude code under Compilers?
The analyzer caught cases where the class should really have been static. #65909
I do not think there was a real harm there, even if we decided to approve the suggested API change. I find this rule useless in general. It might provide some very small guidance around public API shape, but I prefer to not deal with this noise under Compilers. Could we exclude code under Compilers?
Excluded in last commit. @AlekseyTs Do you want me to revert changes under Compilers too or they are good to go?
Note: Since most violations found under Compilers are tests, let me know if you find it noisy for tests specifically and we can disable it for tests only (ie, enabled in compilers product code but not tests).
@Youssef1313
Do you want me to revert changes under Compilers too or they are good to go?
In my opinion, all changes that are not fixing "real" problems should be reverted.