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

Add linker annotations for System.Text.Json #38595

Merged
merged 11 commits into from
Jul 7, 2020
Merged

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Jun 30, 2020

Deserializing immutable collections is not linker safe - will address with #38593.

@layomia layomia added this to the 5.0.0 milestone Jun 30, 2020
@layomia layomia requested a review from jozkee as a code owner June 30, 2020 01:55
@layomia layomia self-assigned this Jun 30, 2020
@layomia layomia force-pushed the linker_json branch 2 times, most recently from 0b34849 to 2f61fcc Compare July 2, 2020 16:07
@@ -26,7 +26,12 @@ namespace System.Diagnostics.CodeAnalysis
AttributeTargets.Field | AttributeTargets.ReturnValue | AttributeTargets.GenericParameter |
AttributeTargets.Parameter | AttributeTargets.Property | AttributeTargets.Method,
Inherited = false)]
public sealed class DynamicallyAccessedMembersAttribute : Attribute
#if SYSTEM_PRIVATE_CORELIB
Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

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:

#if SYSTEM_PRIVATE_CORELIB
public
#else
internal
#endif
sealed class DynamicDependencyAttribute : Attribute

#if INTERNAL_NULLABLE_ATTRIBUTES
internal
#else
public
#endif
sealed class AllowNullAttribute : Attribute { }

Copy link
Member

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

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM

@layomia layomia closed this Jul 6, 2020
@layomia layomia reopened this Jul 6, 2020
@safern
Copy link
Member

safern commented Jul 7, 2020

Merging per @layomia's request.

@safern safern merged commit d43299d into dotnet:master Jul 7, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
@layomia layomia deleted the linker_json branch May 18, 2021 07:03
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.

System.Text.Json.Serialization.JsonStringEnumConverter isn't linker-safe
6 participants