-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add linker annotations for System.Text.Json #38595
Merged
Merged
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
e317b96
Add linker annotations for System.Text.Json
layomia b550bf6
Review feedback - don't use property accessors in tests and use norma…
layomia 94ce9ec
Add annotations to ref & fix immutable collections
layomia 2f14a0b
Address review feedback
layomia e202f53
Clean up linker tests
layomia 2ec1df2
Fix ref file #if directives
layomia 9e34bd0
Clean up
layomia 8630e43
Re-add Hastable.cs
layomia bf7a89b
Remove serialization code from Deserialize overload tests
layomia 1b67842
Verify collection deserialization is correct
layomia a0c5de0
Merge branch 'master' into linker_json
layomia 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 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 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 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 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 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.
I don't think I understand why you need this?
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 understand why you need it now. I'm not sure I agree with what we are doing here to be honest, but I'm fine if that is the direction we are going to take.
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.
Yes, that is the direction we've decided to take. See #36656 (comment)
The other attributes in this folder already have it:
runtime/src/libraries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/DynamicDependencyAttribute.cs
Lines 17 to 22 in 8126962
runtime/src/libraries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/NullableAttributes.cs
Lines 9 to 14 in 8126962
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, honestly I don't fully agree with this approach (as opposed to having them available down level) since we probably want to allow library developers that target netstandard or < .NET5.0 to be able to annotate their libraries as well, specially if these will be libraries used for blazor wasm. Given the attribute implementation is fully netstandard2.0 compatible, I would opt instead for having compatibility packs (similar to what we did with AsyncInterfaces) for shipping the attributes down level so that we a) don't have to compile these attributes internally on a bunch of libraries and b) allow third party library writters to consume them. I don't want to push to much on doing this but just wanted to share my opinion 😄 cc: @stephentoub