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

Keep metadata for rooted members in ILLink #102850

Merged
merged 4 commits into from
May 30, 2024
Merged

Conversation

sbomer
Copy link
Member

@sbomer sbomer commented May 29, 2024

ILLink wasn't keeping metadata (specifically parameter names) for rooted members, whether rooted as part of a root assembly, or by a descriptor XML.

Fixes #81979

@sbomer sbomer requested a review from marek-safar as a code owner May 29, 2024 22:21
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 29, 2024
@dotnet-policy-service dotnet-policy-service bot added the linkable-framework Issues associated with delivering a linker friendly framework label May 29, 2024
@sbomer sbomer added area-Tools-ILLink .NET linker development as well as trimming analyzers and removed linkable-framework Issues associated with delivering a linker friendly framework needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels May 29, 2024
@dotnet-policy-service dotnet-policy-service bot added the linkable-framework Issues associated with delivering a linker friendly framework label May 29, 2024
preserve="fields" only keeps backing fields if a property is already marked.
This looks like it depends on the marking order which is unfortunate, but
let's keep the existing behavior for now.
Copy link
Member

@jtschuster jtschuster left a comment

Choose a reason for hiding this comment

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

Thank you!

Comment on lines +1922 to +1923
if (propertyDefinition != null && !Annotations.IsMarked (propertyDefinition))
continue;
Copy link
Member

Choose a reason for hiding this comment

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

Do we ever need to mark a backing field as visible to reflection since it's reflection over compiler generated code? Could we remove the IsMarked check?

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the idea - filed #102856 to track it.

@jtschuster
Copy link
Member

Should we also mark members marked by the DebuggerDisplayAttribute as visible to reflection? Not necessarily in this PR.

@sbomer sbomer merged commit 45c608b into dotnet:main May 30, 2024
76 checks passed
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
ILLink wasn't keeping metadata (specifically parameter names) for
rooted members, whether rooted as:
- part of a root assembly
- by a root descriptor XML.
- by DebuggerDisplayAttribute/DebuggerTypeProxyAttribute

This ensures all of those mark members visible to reflection so
that method parameter names are kept.
@github-actions github-actions bot locked and limited conversation to collaborators Jun 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Tools-ILLink .NET linker development as well as trimming analyzers linkable-framework Issues associated with delivering a linker friendly framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ILLink shoud keep all metadata for rooted members
2 participants