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] Linker relies on recursive interfaceImpls to be applied by Roslyn #98536

Open
jtschuster opened this issue Feb 16, 2024 · 3 comments · May be fixed by #103317
Open

[ILLink] Linker relies on recursive interfaceImpls to be applied by Roslyn #98536

jtschuster opened this issue Feb 16, 2024 · 3 comments · May be fixed by #103317
Assignees
Labels
area-Tools-ILLink .NET linker development as well as trimming analyzers in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@jtschuster
Copy link
Member

Roslyn will apply InterfaceImpl's of the interfaces declared on the type, and all the interfaces that those interfaces implement, recursively. We rely on this fact in a few places in the linker when searching for DIMs, and determining which interfaces can be trimmed. However, this isn't required by IL, and if Roslyn doesn't do this, we get the wrong behavior.

In the following examples, if each type only has InterfaceImpls for the declared interfaces, the app will still run fine, but the linker would break it.

interface IFoo
{
	void Method();
}

interface IBar : IFoo
{
	void IFoo.Method() { }
}

interface IBaz: IBar /*, not IFoo */
{
	void Method() { }
}
    
class MyFoo : IBaz /* not IBar, not IFoo */
{ }

static void CallMethod(IFoo foo)
{
  foo.Method();
}

static void Main()
{
  CallMethod(new MyFoo());
}
interface IBase
{
  static abstract void Method();
}

interface I2 : IBase 
{ 
 static void IBase.Method() { }
}

interface I3 : I2 /* Not IBase */ { }

interface I4 : I3 /* not I2, not IBase */ { }

static void CallMethod<T>() where T : IBase
{
  T.Method();
}

static void Main()
{
  CallMethod<I4>();
}

See #98436 (comment) for additional context and discussion.

@jtschuster jtschuster added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Feb 16, 2024
@ghost
Copy link

ghost commented Feb 16, 2024

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

Issue Details

Roslyn will apply InterfaceImpl's of the interfaces declared on the type, and all the interfaces that those interfaces implement, recursively. We rely on this fact in a few places in the linker when searching for DIMs, and determining which interfaces can be trimmed. However, this isn't required by IL, and if Roslyn doesn't do this, we get the wrong behavior.

In the following examples, if each type only has InterfaceImpls for the declared interfaces, the app will still run fine, but the linker would break it.

interface IFoo
{
	void Method();
}

interface IBar : IFoo
{
	void IFoo.Method() { }
}

interface IBaz: IBar /*, not IFoo */
{
	void Method() { }
}
    
class MyFoo : IBaz /* not IBar, not IFoo */
{ }

static void CallMethod(IFoo foo)
{
  foo.Method();
}

static void Main()
{
  CallMethod(new MyFoo());
}
interface IBase
{
  static abstract void Method();
}

interface I2 : IBase 
{ 
 static void IBase.Method() { }
}

interface I3 : I2 /* Not IBase */ { }

interface I4 : I3 /* not I2, not IBase */ { }

static void CallMethod<T>() where T : IBase
{
  T.Method();
}

static void Main()
{
  CallMethod<I4>();
}

See #98436 (comment) for additional context and discussion.

Author: jtschuster
Assignees: -
Labels:

area-Tools-ILLink

Milestone: -

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Feb 16, 2024
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Mar 11, 2024
@agocke
Copy link
Member

agocke commented Apr 15, 2024

@jtschuster Has this been resolved?

@agocke agocke added this to the 9.0.0 milestone Apr 15, 2024
@dotnet-policy-service dotnet-policy-service bot removed the untriaged New issue has not been triaged by the area owner label Apr 15, 2024
@jtschuster
Copy link
Member Author

Not fully, I believe TypeMapInfo still doesn't look at recursive interfaces for default interface method mappings.

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 in-pr There is an active PR which will close this issue when it is merged
Projects
Status: No status
2 participants