-
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
[release/6.0] Treat 'abstract' properties the same as 'virtual' for src-gen #59771
Conversation
Tagging subscribers to this area: @dotnet/area-system-text-json Issue DetailsBackport of #59707 to release/6.0 /cc @steveharter Customer ImpactTestingRisk
|
Very low risk change scoped to SG and fixing an issue reported by customer. Please send mail if you haven't already. |
@@ -37,7 +37,7 @@ public static MethodAttributes GetMethodAttributes(this IMethodSymbol methodSymb | |||
|
|||
if (methodSymbol.IsAbstract) | |||
{ | |||
attributes |= MethodAttributes.Abstract; | |||
attributes |= MethodAttributes.Abstract | MethodAttributes.Virtual; |
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.
LGTM, and is also more consistent with reflection:
https://sharplab.io/#v2:EYLgtghglgdgNAFxAJwK7wCYgNQB8ACATAIwCwAUPgAwAE+xAdAJIDyA3BRfQJwAUCATwAOAUwD2AM14BlBMhEQwASgYBxEQgCyGgBZiMvAEQAxADaoAzjsMqAggjlRgqBCItK2QA===
Servicing approved via mail. |
Backport of #59707 to release/6.0
/cc @steveharter
Customer Impact
Fixes an issue where an
abstract
property with a[JsonIgnore]
attribute can cause the source generator to fail and not generate the code. If the base class is not owned by the customer, there is no work-around without this PR.Testing
A new test was added that covers the failure case as well as other permuations.
Risk
Low; the change is specific to source-gen (does not affect the normal JSON serializer) and is a one-line change specific to initializing the metadata for a method.