Skip to content
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

Jump to bottom

Enable CA1052 (static holder types should be static) #65830

Merged
merged 3 commits into from
Dec 3, 2024
Merged

Enable CA1052 (static holder types should be static) #65830

merged 3 commits into from
Dec 3, 2024

Conversation

Youssef1313
Copy link
Member

No description provided.

@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Dec 7, 2022
@Youssef1313 Youssef1313 mentioned this pull request Dec 7, 2022
Merged
@Youssef1313
Copy link
Member Author

@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?

@mavasani
Copy link
Contributor

mavasani commented Dec 7, 2022

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.

CyrusNajmabadi
CyrusNajmabadi reviewed Dec 7, 2022
View reviewed changes
src/Workspaces/Core/Portable/CodeStyle/CodeStyleOptions.cs Outdated
public class CodeStyleOptions
#pragma warning restore CA1052 // Static holder types should be Static or NotInheritable
Copy link
Member

Choose a reason for hiding this comment

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

Same. We'd want to break things if anyone is instantiating these.

@CyrusNajmabadi
Copy link
Member

@mavasani @333fred nothing good can come of people instantiating these types. I don't think we have any obligation to keep that working.

@mavasani
Copy link
Contributor

mavasani commented Dec 7, 2022

@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.

@333fred
Copy link
Member

333fred commented Dec 7, 2022

I guess I'm not convinced of the need to break those users.

@CyrusNajmabadi
Copy link
Member

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.

@333fred
Copy link
Member

333fred commented Dec 7, 2022

I guess I'm ok having that discussion. @Youssef1313, can you please open an issue?

@Youssef1313
Copy link
Member Author

roslyn-CI (Correctness Correctness_Analyzers) is green. This doesn't seem good. SymbolAnnotation should have failed since #65829 isn't yet merged.

@Youssef1313
Copy link
Member Author

@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.cs is 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.dll and /analyzer:C:\Users\PC\.nuget\packages\microsoft.codeanalysis.netanalyzers\7.0.0-preview1.22218.1\analyzers\dotnet\cs\Microsoft.CodeAnalysis.NetAnalyzers.dll
  • The class SymbolAnnotation doesn'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.

@Youssef1313 Youssef1313 mentioned this pull request Dec 9, 2022
Closed
@Youssef1313
Copy link
Member Author

I guess I'm ok having that discussion. @Youssef1313, can you please open an issue?

#65909

@Youssef1313 Youssef1313 marked this pull request as ready for review December 9, 2022 21:18
@Youssef1313 Youssef1313 requested review from a team as code owners December 9, 2022 21:18
@Youssef1313
Copy link
Member Author

@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.

@AlekseyTs
Copy link
Contributor

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.

@Youssef1313 Youssef1313 force-pushed the patch-43 branch 2 times, most recently from 08d8512 to 0b8f7af Compare December 1, 2024 12:58
@Youssef1313
Copy link
Member Author

@CyrusNajmabadi Can you restart the failing jobs please? Thank you!

@build-analysis build-analysis bot mentioned this pull request Dec 1, 2024
Open
3 tasks
@Youssef1313
Copy link
Member Author

@CyrusNajmabadi Sorry for pinging again. I think you already restarted but one job is still failing. If you can restart once again please and review the PR as well. Thanks!

333fred
333fred reviewed Dec 2, 2024
View reviewed changes
.editorconfig Outdated Show resolved Hide resolved
333fred
333fred approved these changes Dec 2, 2024
View reviewed changes
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Compiler change LGTM

CyrusNajmabadi
CyrusNajmabadi approved these changes Dec 2, 2024
View reviewed changes
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

IDE side lgtm.

@Youssef1313
Copy link
Member Author

Youssef1313 commented Dec 2, 2024 •
edited
Loading

Thanks @CyrusNajmabadi @333fred. There is a failing job if you can restart please. Also do we need a second approval on compilers side?

@Youssef1313
Copy link
Member Author

This is green now. Can we get this merged please? @CyrusNajmabadi @jjonescz @333fred

@CyrusNajmabadi CyrusNajmabadi merged commit ef0d493 into dotnet:main Dec 3, 2024
28 checks passed
@CyrusNajmabadi
Copy link
Member

Thanks @Youssef1313 !

@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Dec 3, 2024
@Youssef1313 Youssef1313 deleted the patch-43 branch December 3, 2024 18:40
This was referenced Dec 12, 2024
Closed
Closed
Closed
@dibarbet dibarbet modified the milestones: Next, 17.13 P3 Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlekseyTs AlekseyTs AlekseyTs left review comments

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

@jjonescz jjonescz jjonescz approved these changes

@333fred 333fred 333fred approved these changes

@sharwell sharwell Awaiting requested review from sharwell

Assignees
No one assigned
Labels
Area-Infrastructure Community The pull request was submitted by a contributor who is not a Microsoft employee. VSCode
Projects
None yet
Milestone
17.13 P3
Development

Successfully merging this pull request may close these issues.
8 participants
@Youssef1313 @mavasani @CyrusNajmabadi @333fred @AlekseyTs @sharwell @jjonescz @dibarbet