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

[ILLink] Add interface implementing type to OverrideInformation and use where it makes sense #98274

Merged
merged 20 commits into from
Feb 29, 2024

Conversation

jtschuster
Copy link
Member

@jtschuster jtschuster commented Feb 11, 2024

Fixes #98255

Adds the type that implements the interface to the OverrideInformation class to be able to properly handle cases where the base provides the interface method or the type relies on a default interface method. Adds the InterfaceImplementor class that is basically a tuple of (TypeDefinition typeWithInterface, InterfaceImplementation ImplementedInterface) to do this. Also cleans some unnecessary code in OverrideInformation and ensures that OverrideInformation will have an InterfaceImplementor if the base method is an interface method.

This also reduces allocations of OverrideInformation in a hello world from ~35mb to ~1.2mb.

@ghost ghost added the linkable-framework Issues associated with delivering a linker friendly framework label Feb 11, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Feb 11, 2024
@ghost ghost assigned jtschuster Feb 11, 2024
@ghost
Copy link

ghost commented Feb 11, 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 #98255

Adds the type that implements the interface to the OverrideInformation class to be able to properly handle cases where the base provides the interface method or the type relies on a default interface method. Adds the InterfaceImplementor class that is basically a tuple of (TypeDefinition typeWithInterface, InterfaceImplementation ImplementedInterface) to do this. Also cleans some unnecessary code in OverrideInformation and ensures that OverrideInformation will have an InterfaceImplementor if the base method is an interface method.

Author: jtschuster
Assignees: -
Labels:

linkable-framework

Milestone: -

@jtschuster jtschuster requested a review from sbomer February 12, 2024 22:32
@jtschuster jtschuster changed the title Add interface implementing type to OverrideInformation and use where it makes sense [ILLink] Add interface implementing type to OverrideInformation and use where it makes sense Feb 14, 2024
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.

It's possible for a base to provide a static interface method, which seems like it might have broken the old logic. Could you add a testcase for this (using static interface methods, where the interface type isn't instantiated), and try to confirm that this change fixes it?

edit: addressed by your comment at #97487 (comment).

src/tools/illink/src/linker/Linker/InterfaceImplementor.cs Outdated Show resolved Hide resolved
src/tools/illink/src/linker/Linker/TypeMapInfo.cs Outdated Show resolved Hide resolved
src/tools/illink/src/linker/Linker/TypeMapInfo.cs Outdated Show resolved Hide resolved
src/tools/illink/src/linker/Linker/TypeMapInfo.cs Outdated Show resolved Hide resolved
src/tools/illink/src/linker/Linker/TypeMapInfo.cs Outdated Show resolved Hide resolved
src/tools/illink/src/linker/Linker/TypeMapInfo.cs Outdated Show resolved Hide resolved
Revert "Look on base types and interfaces for interfaceImpl"

This reverts commit 81ebc93.

Reapply "Look on base types and interfaces for interfaceImpl"

This reverts commit 4320d2195719a68b51efa70fcced1fc5c7c9f6f2.

Add test case

Simplify test case
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.

LGTM aside from the last few comments, thanks!

I'm not sure how well this InterfaceImplementor concept will translate once you improve support for recursive interfaces, since the impl will no longer always be on the "Implementor" type. But looks good for the current support.

src/tools/illink/src/linker/Linker/InterfaceImplementor.cs Outdated Show resolved Hide resolved
- Rename local
- Assert InterfaceType is resolved InterfaceImplementation.InterfaceType
@jtschuster jtschuster merged commit 1afd4bc into dotnet:main Feb 29, 2024
74 of 76 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 31, 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
2 participants