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

Improve JsonSchemaExporter trimmer safety. #5591

Merged
merged 7 commits into from
Nov 1, 2024

Conversation

eiriktsarpalis
Copy link
Member

@eiriktsarpalis eiriktsarpalis commented Oct 31, 2024

Improves the private reflection code used by the JsonSchemaExporter polyfill so that trimmer safety is improved.

Microsoft Reviewers: Open in CodeFlow

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.

I think the current changes look better than what we had before. I think there is something that can be done in the future about that DynamicallyAccessedMemberTypes.All reference. But that shouldn't block this PR.

I see there are still 3 UnconditionalSuppressMessage attributes in this file.

The 2 regarding NullableAttribute and NullableContextAttribute don't look correct to me. The AOT compiler could trim the field metadata for these types, in theory. Maybe it would be better to use GetCustomAttributesData instead.

For the one regarding GetGenericMemberDefinition, I think we could use GetMemberWithSameMetadataDefinitionAs to remove the suppression.

@MichalStrehovsky
Copy link
Member

The 2 regarding NullableAttribute and NullableContextAttribute don't look correct to me. The AOT compiler could trim the field metadata for these types, in theory. Maybe it would be better to use GetCustomAttributesData instead.

It would be better to use GetCustomAttributesData for perf in general. This activates all custom attributes on the type, only to maybe find one that it interesting and look at the field. It's unnecessary work. I also agree native AOT might be able to optimize things away here and turn the suppression into a bug.

eiriktsarpalis added a commit to eiriktsarpalis/runtime that referenced this pull request Nov 1, 2024
@eiriktsarpalis eiriktsarpalis enabled auto-merge (squash) November 1, 2024 10:49
@eiriktsarpalis eiriktsarpalis merged commit 0672220 into dotnet:main Nov 1, 2024
6 checks passed
@eiriktsarpalis eiriktsarpalis deleted the improve-trimmer-safety branch November 1, 2024 11:20
eiriktsarpalis added a commit to dotnet/runtime that referenced this pull request Nov 1, 2024
… schema generation. (#109424)

* Add annotations and regression testing for members accessed by legacy schema generation.

* Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs

* Incorporate feedback from dotnet/extensions#5591
joperezr pushed a commit to joperezr/extensions that referenced this pull request Nov 5, 2024
* Improve JsonSchemaExporter trimmer safety.

* Remove var

* Address feedback.

* Remove DynamicallyAccessedMemberTypes.All

* Extract reflection helpers into separate file and remove a number of warning suppressions.

* Re-enable failing tests that were patched in .NET 9
@github-actions github-actions bot locked and limited conversation to collaborators Dec 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants