-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Remove redundant trimmer warning suppressions #73238
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
Merged
Merged
Changes from all commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
761aefb
Removed redundant suppressions from System.Private.CoreLib
jkurdek bbbc1e7
Removed redundant suppressions from System.Text.Json
jkurdek 75abc6c
Removed redundant suppressions from System.Net.Http.Json
jkurdek 060a478
Removed redundant suppressions from System.Private.DataContractSerial…
jkurdek 0e6e96a
Removed redundant suppressions from System.Linq.Queryable
jkurdek 8438461
Removed redundant suppressions from System.Reflection.DispatchProxy
jkurdek 3d15fcc
Removed redundant suppressions from System.ComponentModel.TypeConverter
jkurdek e25790d
Removed redundant suppressions from System.Data.Common
jkurdek d6b594d
Removed redundant suppressions from Microsoft.CSharp
jkurdek 32162d2
Removed redundant suppressions from Microsoft.VisualBasic.Core
jkurdek fc0e9d5
Removed redundant suppressions from System.Diagnostics.DiagnosticSource
jkurdek f3670c2
Apply suggestions from code review
jkurdek 38b6f45
Suggestions from PR review
jkurdek 5a6bb2e
removed omitted local function call
jkurdek e941d97
Enabled IL2121 in sfx
jkurdek bedf079
fix suppressions mono + X86
jkurdek 393a6f2
Removed unnecessery suppressions around PInokes
jkurdek f8209e1
Fixed IL2121 warnings on Linux
jkurdek 9cc221b
Apply suggestions from PR review
jkurdek d58df4f
Fix redundant suppression after dynamic code annotation
jkurdek 9a0df91
Removed redundant suppressions NativeAot
jkurdek 0c7165d
Reverted change in EventSource, put suppression in ifdef
jkurdek 04dc831
Removed redundant suppressions found on mac/iOS
jkurdek File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have references to this bug under src/coreclr/nativeaot as well - how was the redundant suppression analysis for NativeAOT done? I wonder why it didn't find those (they should be redundant because they were just a bug workaround).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The redundant warning suppression detection has not been ported to nativeaot yet (#75201). The suppression you mentioned should be identified after the functionality is ported and run witihin nativeaot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR has a change in src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/Assignability.cs - what mechanism was used to detect it?
(NativeAOT doesn't run on code that is unreferenced and doesn't have a "library mode" like illink does - so the ability to detect redundant suppressions is somewhat limited even if we were to port the feature - we would have to come up with an app that includes all the code in NativeAOT's libraries. It will only flag things that are used, making it easy for us to ship with an actual warning that only some customers will see - customers who happen to use that part of the library.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was detected because linker was used to "trim libraries for warnings" (The
sfxproject run of linker) and it was given the AOT corelib as input. Jeremi found out that this happens when you runbuild Clr.Aot+Libs -rc Debug -lc Release, in which case linker getsartifacts\bin\coreclr\windows.x64.Debug\aotsdk\System.Private.CoreLib.dllas one of the input assemblies.As for the comment about NativeAOT not having libraries mode - that should not matter. The detection works in the "trim app" mode as well - it only reports redundant warnings on methods which were "marked".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, yes, there can only be one corelib project for the libs partition but we have 3 in this repo.
The corelib is selected here:
runtime/Directory.Build.props
Lines 261 to 263 in e8b1491