Skip to content

Setting RequiredMemberAttribute usage as inherited. #89418

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.

Sign up for GitHub

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

Jump to bottom
Merged
eiriktsarpalis merged 8 commits into from
Jul 27, 2023
Merged

Setting RequiredMemberAttribute usage as inherited. #89418

eiriktsarpalis merged 8 commits into from
Jul 27, 2023

Conversation

@Maximys
Copy link
Contributor

@Maximys Maximys commented Jul 25, 2023

Trying to fix #86031

@ghost
Copy link

ghost commented Jul 25, 2023

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 25, 2023
@ghost
Copy link

ghost commented Jul 25, 2023

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

Trying to fix #86031

Author: Maximys
Assignees: -
Labels:

area-System.Text.Json, new-api-needs-documentation
Milestone: -

@Maximys Maximys marked this pull request as ready for review July 25, 2023 09:56
eiriktsarpalis
eiriktsarpalis requested changes Jul 25, 2023
View reviewed changes
Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

Please see #86031 (comment). This is not an issue with how the attribute itself is defined (it works as expected in the case of source gen) but rather it is a bug in the reflection resolver.

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 25, 2023
@build-analysis build-analysis bot mentioned this pull request Jul 25, 2023
Closed
@Maximys Maximys force-pushed the bugfix/86031-invalid-required-keyword-processing-for-inheritance branch from b40f59c to d1c4cfa Compare July 26, 2023 04:58
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 26, 2023
@Maximys
Copy link
Contributor Author

Maximys commented Jul 26, 2023 •
edited
Loading

Please see #86031 (comment). This is not an issue with how the attribute itself is defined (it works as expected in the case of source gen) but rather it is a bug in the reflection resolver.

@eiriktsarpalis , thanks. Fixed by the proposed way

@Maximys Maximys force-pushed the bugfix/86031-invalid-required-keyword-processing-for-inheritance branch from d1c4cfa to 276c4fb Compare July 27, 2023 09:08
eiriktsarpalis
eiriktsarpalis reviewed Jul 27, 2023
View reviewed changes
...Text.Json/src/System/Text/Json/Serialization/Metadata/DefaultJsonTypeInfoResolver.Helpers.cs Outdated
Comment on lines 93 to 94
bool shouldCheckMembersForRequiredMemberAttribute
= !constructorHasSetsRequiredMembersAttribute && currentType.HasRequiredMemberAttribute();
Copy link
Member

@eiriktsarpalis eiriktsarpalis Jul 27, 2023

Choose a reason for hiding this comment

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

Suggested change
bool shouldCheckMembersForRequiredMemberAttribute
= !constructorHasSetsRequiredMembersAttribute && currentType.HasRequiredMemberAttribute();
bool shouldCheckMembersForRequiredMemberAttribute = !constructorHasSetsRequiredMembersAttribute && currentType.HasRequiredMemberAttribute();

@eiriktsarpalis
Update src/libraries/System.Text.Json/tests/System.Text.Json.SourceGe…
76140b1 
…neration.Tests/System.Text.Json.SourceGeneration.Tests.targets
eiriktsarpalis
eiriktsarpalis approved these changes Jul 27, 2023
View reviewed changes
Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

This looks great, thank you for the contribution!

@eiriktsarpalis
Update src/libraries/System.Text.Json/src/System/Text/Json/Serializat…
d9c83e3 
…ion/Metadata/DefaultJsonTypeInfoResolver.Helpers.cs
@eiriktsarpalis eiriktsarpalis merged commit 8ba85b4 into dotnet:main Jul 27, 2023
This was referenced Aug 1, 2023
Closed
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Aug 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@eiriktsarpalis eiriktsarpalis eiriktsarpalis approved these changes

Assignees

No one assigned

Labels

area-System.Text.Json community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

System.Text.Json deserialisation does not respect "required" keyword when used in base class

2 participants

@Maximys @eiriktsarpalis