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

Analyze recursive interfaces in illink #99566

Closed
wants to merge 46 commits into from

Conversation

jtschuster
Copy link
Member

Fixes #98536

Creates a new mapping cache in TypeMapInfo to map a type to all its recursive interfaces, and uses this throughout when marking interface implementations, checking if a type implements a marked interface, and finding interface method implementations.

The InterfaceImplementor now has an array of InterfaceImplementationNodes which represent the graph of how an interface could be explicitly or recursively implemented by the type. The leaf nodes in the graph are InterfaceImplementations that point to the InterfaceImplementor.InflatedInterface.

Each InterfaceImplementationNode has an InterfaceImplementation, the TypeDefinition of the type that has the InterfaceImplementation on it, and an array of the next nodes in the graph that point to the interface type.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Mar 11, 2024
@dotnet-policy-service dotnet-policy-service bot added the linkable-framework Issues associated with delivering a linker friendly framework label Mar 11, 2024
@jtschuster jtschuster requested a review from sbomer March 11, 2024 23:53
while (ifacesToCheck.Count > 0) {
var currentIface = ifacesToCheck.Dequeue ();
foreach (var impl in Next) {
if (impl.IsMarked (annotations))
Copy link
Member

Choose a reason for hiding this comment

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

It looks like IsMarked returns true if there is a chain of marked .impls up to some leaf interface (without further impls) starting from the implementing type. What ensures that it's the interface we are asking about?

Copy link
Member Author

Choose a reason for hiding this comment

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

This assert makes sure that the terminal interface of each tree in InterfaceImplementor.InterfaceImplementationNodes is the InterfaceImplementor.InterfaceType. When we create these chains in TypeMapInfo, there is a whole new graph for each interface that can be implemented by a type.

var inflatedInterface = item.InflatedInterface.TryInflateFrom (type.BaseType, context);
Debug.Assert (inflatedInterface is not null);
foreach (var node in item.InterfaceImplementationNodes) {
waysToImplementIface.AddToList (inflatedInterface, node);
Copy link
Member

Choose a reason for hiding this comment

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

This is adding the .impl chains starting from the base type, to the list of .impl chains starting from the derived type. Why do we need to track those .impl chains?

I'm interested in how this example works:

I i = new Derived();
i.M();

interface I {
    void M();
}

class Base : I {
    public virtual void M() {}
}

class Derived : Base {
    public override void M() {}
}

Since we don't trim base classes, I think it could work by processing I.M, seeing that it's overridden by Base.M, and therefore marking Base's impl of I. Then separately when processing Base.M, we would see that it's overridden by Derived.M without having to ask "does Derived implement I?".

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 can't think of a time that wouldn't work for marking methods, but I think it's valuable to keep the recursive interfaces list accurate and be explicit about keeping it for the interface rather than relying on processing in other places to keep it. I'll look into whether the perf difference would be significant, but if it's not I'd rather keep this as is.

{
if (Annotations.IsMarked (iface))
if (interfaceImplementor.IsMostDirectImplementationMarked (Annotations))
Copy link
Member

Choose a reason for hiding this comment

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

I think it'd make sense to separate this out into another change to make it clear what that impact of this optimization is on the test cases.

Copy link
Member

Choose a reason for hiding this comment

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

In fact, would it be possible to start with a simpler change that does something like IsInterfaceImplementationMarkedRecursively here, without tracking the instantiated interface types?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I could probably start with that.

@jtschuster jtschuster closed this May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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] Linker relies on recursive interfaceImpls to be applied by Roslyn
2 participants