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

Implements more advanced ILLink/ILCompiler analysis for Type.GetMember #94879

Closed
wants to merge 20 commits into from

Conversation

hamarb123
Copy link
Contributor

@hamarb123 hamarb123 commented Nov 16, 2023

Fixes #94745

Allows narrowing members down by name & by member type.

- Implements masking the required member types based on the MemberTypes parameter
- Matching by name is implemented here, including logic to handle the prefix name lookup scheme of GetMember & special handling for constructors
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 16, 2023
@ghost
Copy link

ghost commented Nov 16, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #94745

Author: hamarb123
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@hamarb123
Copy link
Contributor Author

hamarb123 commented Dec 7, 2023

Comment so this doesn't get closed - can someone please review this? Thanks!

Edit: sorry for so many commits btw, I'm using VS Code so I only get basic syntax highlighting 😆.

@MichalStrehovsky MichalStrehovsky added area-Tools-ILLink .NET linker development as well as trimming analyzers and removed area-NativeAOT-coreclr labels Dec 8, 2023
@ghost
Copy link

ghost commented Dec 8, 2023

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #94745

Allows narrowing members down by name & by member type.

Author: hamarb123
Assignees: -
Labels:

community-contribution, area-Tools-ILLink

Milestone: -

- Update the `[Kept]` attributes on the unit tests to reflect what we should now expect to be kept
- Add some more unit tests
- Fix missing `| BindingFlags.Static` for the default binding flags
- And add another .IsEmpty check like similar APIs have
Copy link
Member

@sbomer sbomer left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

// Assume a default value for MemberTypes for methods that don't use MemberTypes as a parameter
memberTypes = MemberTypes.All;
// Assume a default value for BindingFlags for methods that don't use BindingFlags as a parameter (Type.DefaultLookup)
bindingFlags = BindingFlags.Public | BindingFlags.Static | BindingFlags.Instance;
Copy link
Member

Choose a reason for hiding this comment

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

This changes the default behavior (looks correct to me). Could we make sure there are tests for this part of the change? So add a test that checks that static methods are kept when not passing binding flags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will take a look through this next week some time (away currently).

MemberTypes.Method |
MemberTypes.Property |
MemberTypes.TypeInfo |
MemberTypes.NestedType;
Copy link
Member

Choose a reason for hiding this comment

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

Could you also please add test coverage for the various member types (ensure that when MemberTypes are passed, we keep exactly those member types on a reflected type)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't implemented the changes to the tests yet, will close these comments when I do. I want to get the current tests to not fail first, then I'll change them to be what the new code should expect.

- Remove prefix versions of APIs
- Remove ignoring of .Custom
- Add missing space
@ghost ghost added the linkable-framework Issues associated with delivering a linker friendly framework label Jan 17, 2024
@ghost
Copy link

ghost commented Jan 17, 2024

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr, @marek-safar
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #94745

Allows narrowing members down by name & by member type.

Author: hamarb123
Assignees: -
Labels:

linkable-framework, community-contribution, area-Tools-ILLink

Milestone: -

- Remove remaining prefix code
- Don't call AddReturnValue
- Implement feedback for .ctor
- Remove calls to `AddReturnValue (MultiValueLattice.Top);`, since this returns an array
@jeffschwMSFT jeffschwMSFT added the needs-author-action An issue or pull request that requires more info or actions from the author. label Mar 18, 2024
@hamarb123
Copy link
Contributor Author

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

I'm unable to look at this at the moment, or for another few weeks probably. Will re-open or open a new PR when I'm able to if/when I come back to it.

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 community-contribution Indicates that the PR has been added by a community member linkable-framework Issues associated with delivering a linker friendly framework needs-author-action An issue or pull request that requires more info or actions from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Type.GetMember usage analysis properly with IL3050 warning (NAOT)
5 participants