Skip to content

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

Closed
hamarb123 wants to merge 20 commits intodotnet:mainfrom
hamarb123:main7
Closed

Implements more advanced ILLink/ILCompiler analysis for Type.GetMember#94879
hamarb123 wants to merge 20 commits intodotnet:mainfrom
hamarb123:main7

Conversation

@hamarb123
Copy link
Contributor

@hamarb123 hamarb123 commented Nov 16, 2023 •
edited
Loading

Fixes #94745

Allows narrowing members down by name & by member type.

@hamarb123
Implement masking based on MemberTypes
ced1292 
- Implements masking the required member types based on the MemberTypes parameter
@hamarb123
Update HandleCallAction.cs
d0df383
@hamarb123
Implement matching by name
65a5a1c 
- Matching by name is implemented here, including logic to handle the prefix name lookup scheme of GetMember & special handling for constructors
@ghost ghost added area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member labels 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
hamarb123 commented Nov 16, 2023
View reviewed changes
hamarb123
hamarb123 commented Nov 16, 2023
View reviewed changes
hamarb123
hamarb123 commented Nov 16, 2023
View reviewed changes
@hamarb123
Copy link
Contributor Author

hamarb123 commented Dec 7, 2023 •
edited
Loading

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 😆.

@hamarb123 hamarb123 requested a review from sbomer as a code owner December 7, 2023 20:11
@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: -

@hamarb123
Add some unit tests & fix some code
1053805 
- 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
@hamarb123
Fix typo & move ctors to top
02b6c45
@hamarb123
Update MemberUsedViaReflection.cs
40044cd
@hamarb123
Update MemberUsedViaReflection.cs
0646bb1
@hamarb123
Update some [Kept] attributes
b665d99 
- And add another .IsEmpty check like similar APIs have
sbomer
sbomer reviewed Dec 21, 2023
View reviewed changes
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!

src/tools/illink/src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs
// 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

@sbomer sbomer Dec 21, 2023

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

@hamarb123 hamarb123 Dec 27, 2023

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).

src/tools/illink/src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs
MemberTypes.Method |
MemberTypes.Property |
MemberTypes.TypeInfo |
MemberTypes.NestedType;
Copy link
Member

@sbomer sbomer Dec 21, 2023

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

@hamarb123 hamarb123 Jan 30, 2024

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.

@hamarb123
Apply some feedback (incomplete)
dd6379b 
- 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: -

@hamarb123
Update HandleCallAction.cs
ec45c87 
- Remove remaining prefix code
- Don't call AddReturnValue
@hamarb123
Merge branch 'main' into main7
ac1066a
@hamarb123
Update HandleCallAction.cs
4d8ccf3 
- Implement feedback for .ctor
- Remove calls to `AddReturnValue (MultiValueLattice.Top);`, since this returns an array
@hamarb123
Update HandleCallAction.cs
f8cfd86
@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

hamarb123 commented Apr 2, 2024

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.

@hamarb123 hamarb123 closed this Apr 2, 2024
@hamarb123 hamarb123 reopened this Apr 2, 2024
@hamarb123 hamarb123 closed this Apr 2, 2024
This was referenced Apr 2, 2024
Open
Closed
Closed
Closed
Closed
@github-actions github-actions bot locked and limited conversation to collaborators May 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sbomer sbomer sbomer left review comments

@vitek-karas vitek-karas vitek-karas left review comments

@marek-safar marek-safar Awaiting requested review from marek-safar

@MichalStrehovsky MichalStrehovsky Awaiting requested review from MichalStrehovsky

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

5 participants

@hamarb123 @sbomer @vitek-karas @jeffschwMSFT @MichalStrehovsky