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.

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

Address unused suppressions #89216

Merged
merged 3 commits into from
Jul 25, 2023
Merged

Address unused suppressions #89216

merged 3 commits into from
Jul 25, 2023

Conversation

ericstj
Copy link
Member

@ericstj ericstj commented Jul 19, 2023

Fix #89206

<Suppressions xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema">
<Suppression>
<DiagnosticId>CP0001</DiagnosticId>
<Target>T:System.Runtime.InteropServices.JavaScript.WebWorker</Target>
Copy link
Member Author

Choose a reason for hiding this comment

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

So it looks like this is implementation-only API that's only present in one implementation assembly. Since it's excluded from the ref that means it's not a problem for users. Make the suppression conditional as well so that we don't fail the new unused suppressions check.

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

Thanks!

@MichalStrehovsky
Copy link
Member

Thanks!

Although it looks like it's failing ApiCompat. EagerStaticClassConstructionAttribute can be made private to corelib, we don't use it outside corelib and the only reason why it's public is probably some .NET Native history.

DebuggerGuidedStepThroughAttribute is used outside corelib. This attribute is used in pair with System.Diagnostics.DebugAnnotations that is also in the suppression. We could either move this to the plan where we make it private and source include the file from multiple assemblies (these are matched by name, they don't need to be in corelib), or keep the suppression.

@mthalman
Copy link
Member

We've got some new API compat errors that popped up on Friday. Can those be addressed here as well?

@ericstj
Copy link
Member Author

ericstj commented Jul 25, 2023

We've got some #89206 (comment) that popped up on Friday. Can those be addressed here as well?

I'll put a separate PR for those. I think this one is just about ready to merge.

Ideally we could also get the new APICompat in build so that we can hold the line on these issues. @ViktorHofer

@ViktorHofer
Copy link
Member

ViktorHofer commented Jul 25, 2023

Ideally we could also get the new APICompat in build so that we can hold the line on these issues. @ViktorHofer

Tracking issue: #88675

@ericstj ericstj merged commit 2300d0e into dotnet:main Jul 25, 2023
147 of 150 checks passed
@ericstj
Copy link
Member Author

ericstj commented Jul 25, 2023

The one failure was a test issue which I filed.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unnecessary suppression errors showing up in build using .NET 8 Preview 7 SDK
5 participants