Revert "Emit less metadata for not-reflection-visible types (#91660)"

[MichalStrehovsky]

This is a revert of #91660 (first commit, no need to review that, really) and a regression test (second commit).

This fixes an issue discovered in the SDK repo: dotnet/sdk#35293 (comment)

The compiler assumes MethodTables used in casts and interface dispatch are not reflection visible. As we found the hard way in the above PR, this is not true when IDynamicInterfaceCastable is present. With IDynamicInterfaceCastable, any (interface) type handle used in cast or interface dispatch could be reflection visible in user code.

The compiler had this wrong assumption for a while (the regression test I'm adding would fail on .NET 7). The only reason why we haven't already seen this in .NET 7 was that crossgen2 PDB writing was written with a manual ComWrapper in .NET 7, vs in .NET 8 it uses the generator that does a bunch of reflection on the type handle for whatever reason.

We'll probably also want to upgrade the places within the compiler that hand out NecessaryEEType for dispatch/cast to interfaces to hand out ConstructedEEType instead but I don't want to do that as part of a .NET 8 fix. This has been broken in 7.0 too and it is also observable (NecessaryEETypes don't have a populated interface list), but nothing known fails on this right now.

So thanks to IDynamicInterfaceCastable, the problem #91660 was trying to solve is totally unfixable and WinForms apps will always bring half of WPF with them.

Cc @dotnet/ilc-contrib

[ghost]

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

Issue Details

---

This is a revert of #91660 (first commit, no need to review that, really) and a regression test (second commit).

This fixes an issue discovered in the SDK repo: dotnet/sdk#35293 (comment)

The compiler assumes MethodTables used in casts and interface dispatch are not reflection visible. As we found the hard way in the above PR, this is not true when IDynamicInterfaceCastable is present. With IDynamicInterfaceCastable, any (interface) type handle used in cast or interface dispatch could be reflection visible in user code.

The compiler had this wrong assumption for a while (the regression test I'm adding would fail on .NET 7). The only reason why we haven't already seen this in .NET 7 was that crossgen2 PDB writing was written with a manual ComWrapper in .NET 7, vs in .NET 8 it uses the generator that does a bunch of reflection on the type handle for whatever reason.

We'll probably also want to upgrade the places within the compiler that hand out NecessaryEEType for dispatch/cast to interfaces to hand out ConstructedEEType instead but I don't want to do that as part of a .NET 8 fix. This has been broken in 7.0 too and it is also observable (NecessaryEETypes don't have a populated interface list), but nothing known fails on this right now.

So thanks to IDynamicInterfaceCastable, the problem #91660 was trying to solve is totally unfixable and WinForms apps will always bring half of WPF with them.

Cc @dotnet/ilc-contrib

Author:	MichalStrehovsky
Assignees:	-
Labels:	area-NativeAOT-coreclr
Milestone:	-

[MichalStrehovsky]

/backport to release/8.0

[github-actions]

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6169862067

[jkotas]

We have DynamicInterfaceCastableImplementationAttribute that limits impact of IDynamicInterfaceCastable. Can we use that to only reflection-enable interfaces that have possibly valid IDynamicInterfaceCastable implementations?

[MichalStrehovsky]

We have DynamicInterfaceCastableImplementationAttribute that limits impact of IDynamicInterfaceCastable. Can we use that to only reflection-enable interfaces that have possibly valid IDynamicInterfaceCastable implementations?

The interface that the COM source generator reflects on is not the implementation interface.

In theory we don't even have a guarantee there is a DynamicInterfaceCastableImplementationAttribute interface in the system. The test I'm adding doesn't have that anywhere and still needs to see all the metadata.

[jkotas]

I think it would be perfectly fine to skip calling IDynamicInterfaceCastable callbacks for interfaces that cannot be possibly implemented by IDynamicInterfaceCastable. In other words, if the system can prove that the interface cannot be possibly implemented by an object by some means, it is fine to skip IDynamicInterfaceCastable callbacks.

[MichalStrehovsky]

I think it would be perfectly fine to skip calling IDynamicInterfaceCastable callbacks for interfaces that cannot be possibly implemented by IDynamicInterfaceCastable. In other words, if the system can prove that the interface cannot be possibly implemented by an object by some means, it is fine to skip IDynamicInterfaceCastable callbacks.

It's a lot of work to make an observable optimization around this that could break appcompat.

I don't think the savings we get from this are huge in general. We get a lot of savings when we can optimize constructed methodtable to unconstructed ones for classes (because classes have vtables). Interfaces are less interesting.

We'll have to tell WinForms team to divorce their package from WPF if they want good size so that the problematic attribute on ICommand doesn't resolve unless one actually references WPF. This would help everywhere, including untrimmed selfcontained deployments of WinForms apps.

[RussKie]

We're observing the failures on the release/8.0 branch -> dotnet/aspnetcore#50648 (comment). Is the fix being backported?

Didn't see https://github.com/dotnet/runtime/actions/runs/6169862067. All good.

[MichalStrehovsky]

This is a revert of #91660 (first commit, no need to review that, really) and a regression test (second commit).

This fixes an issue discovered in the SDK repo: dotnet/sdk#35293 (comment)

The compiler assumes MethodTables used in casts and interface dispatch are not reflection visible. As we found the hard way in the above PR, this is not true when IDynamicInterfaceCastable is present. With IDynamicInterfaceCastable, any (interface) type handle used in cast or interface dispatch could be reflection visible in user code.

The compiler had this wrong assumption for a while (the regression test I'm adding would fail on .NET 7). The only reason why we haven't already seen this in .NET 7 was that crossgen2 PDB writing was written with a manual ComWrapper in .NET 7, vs in .NET 8 it uses the generator that does a bunch of reflection on the type handle for whatever reason.

We'll probably also want to upgrade the places within the compiler that hand out NecessaryEEType for dispatch/cast to interfaces to hand out ConstructedEEType instead but I don't want to do that as part of a .NET 8 fix. This has been broken in 7.0 too and it is also observable (NecessaryEETypes don't have a populated interface list), but nothing known fails on this right now.

So thanks to IDynamicInterfaceCastable, the problem #91660 was trying to solve is totally unfixable and WinForms apps will always bring half of WPF with them.

Cc @dotnet/ilc-contrib

[ghost]

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

Issue Details

---

This is a revert of #91660 (first commit, no need to review that, really) and a regression test (second commit).

This fixes an issue discovered in the SDK repo: dotnet/sdk#35293 (comment)

The compiler assumes MethodTables used in casts and interface dispatch are not reflection visible. As we found the hard way in the above PR, this is not true when IDynamicInterfaceCastable is present. With IDynamicInterfaceCastable, any (interface) type handle used in cast or interface dispatch could be reflection visible in user code.

The compiler had this wrong assumption for a while (the regression test I'm adding would fail on .NET 7). The only reason why we haven't already seen this in .NET 7 was that crossgen2 PDB writing was written with a manual ComWrapper in .NET 7, vs in .NET 8 it uses the generator that does a bunch of reflection on the type handle for whatever reason.

We'll probably also want to upgrade the places within the compiler that hand out NecessaryEEType for dispatch/cast to interfaces to hand out ConstructedEEType instead but I don't want to do that as part of a .NET 8 fix. This has been broken in 7.0 too and it is also observable (NecessaryEETypes don't have a populated interface list), but nothing known fails on this right now.

So thanks to IDynamicInterfaceCastable, the problem #91660 was trying to solve is totally unfixable and WinForms apps will always bring half of WPF with them.

Cc @dotnet/ilc-contrib

Author:	MichalStrehovsky
Assignees:	-
Labels:	area-NativeAOT-coreclr
Milestone:	-

[MichalStrehovsky]

/backport to release/8.0

[github-actions]

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6169862067

[jkotas]

We have DynamicInterfaceCastableImplementationAttribute that limits impact of IDynamicInterfaceCastable. Can we use that to only reflection-enable interfaces that have possibly valid IDynamicInterfaceCastable implementations?

[MichalStrehovsky]

We have DynamicInterfaceCastableImplementationAttribute that limits impact of IDynamicInterfaceCastable. Can we use that to only reflection-enable interfaces that have possibly valid IDynamicInterfaceCastable implementations?

The interface that the COM source generator reflects on is not the implementation interface.

In theory we don't even have a guarantee there is a DynamicInterfaceCastableImplementationAttribute interface in the system. The test I'm adding doesn't have that anywhere and still needs to see all the metadata.

[jkotas]

I think it would be perfectly fine to skip calling IDynamicInterfaceCastable callbacks for interfaces that cannot be possibly implemented by IDynamicInterfaceCastable. In other words, if the system can prove that the interface cannot be possibly implemented by an object by some means, it is fine to skip IDynamicInterfaceCastable callbacks.

[MichalStrehovsky]

I think it would be perfectly fine to skip calling IDynamicInterfaceCastable callbacks for interfaces that cannot be possibly implemented by IDynamicInterfaceCastable. In other words, if the system can prove that the interface cannot be possibly implemented by an object by some means, it is fine to skip IDynamicInterfaceCastable callbacks.

It's a lot of work to make an observable optimization around this that could break appcompat.

I don't think the savings we get from this are huge in general. We get a lot of savings when we can optimize constructed methodtable to unconstructed ones for classes (because classes have vtables). Interfaces are less interesting.

We'll have to tell WinForms team to divorce their package from WPF if they want good size so that the problematic attribute on ICommand doesn't resolve unless one actually references WPF. This would help everywhere, including untrimmed selfcontained deployments of WinForms apps.

[RussKie]

We're observing the failures on the release/8.0 branch -> dotnet/aspnetcore#50648 (comment). Is the fix being backported?

Didn't see https://github.com/dotnet/runtime/actions/runs/6169862067. All good.

[MichalStrehovsky]

This is a revert of #91660 (first commit, no need to review that, really) and a regression test (second commit).

This fixes an issue discovered in the SDK repo: dotnet/sdk#35293 (comment)

The compiler assumes MethodTables used in casts and interface dispatch are not reflection visible. As we found the hard way in the above PR, this is not true when IDynamicInterfaceCastable is present. With IDynamicInterfaceCastable, any (interface) type handle used in cast or interface dispatch could be reflection visible in user code.

The compiler had this wrong assumption for a while (the regression test I'm adding would fail on .NET 7). The only reason why we haven't already seen this in .NET 7 was that crossgen2 PDB writing was written with a manual ComWrapper in .NET 7, vs in .NET 8 it uses the generator that does a bunch of reflection on the type handle for whatever reason.

We'll probably also want to upgrade the places within the compiler that hand out NecessaryEEType for dispatch/cast to interfaces to hand out ConstructedEEType instead but I don't want to do that as part of a .NET 8 fix. This has been broken in 7.0 too and it is also observable (NecessaryEETypes don't have a populated interface list), but nothing known fails on this right now.

So thanks to IDynamicInterfaceCastable, the problem #91660 was trying to solve is totally unfixable and WinForms apps will always bring half of WPF with them.

Cc @dotnet/ilc-contrib

[ghost]

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

Issue Details

---

This is a revert of #91660 (first commit, no need to review that, really) and a regression test (second commit).

This fixes an issue discovered in the SDK repo: dotnet/sdk#35293 (comment)

The compiler assumes MethodTables used in casts and interface dispatch are not reflection visible. As we found the hard way in the above PR, this is not true when IDynamicInterfaceCastable is present. With IDynamicInterfaceCastable, any (interface) type handle used in cast or interface dispatch could be reflection visible in user code.

The compiler had this wrong assumption for a while (the regression test I'm adding would fail on .NET 7). The only reason why we haven't already seen this in .NET 7 was that crossgen2 PDB writing was written with a manual ComWrapper in .NET 7, vs in .NET 8 it uses the generator that does a bunch of reflection on the type handle for whatever reason.

We'll probably also want to upgrade the places within the compiler that hand out NecessaryEEType for dispatch/cast to interfaces to hand out ConstructedEEType instead but I don't want to do that as part of a .NET 8 fix. This has been broken in 7.0 too and it is also observable (NecessaryEETypes don't have a populated interface list), but nothing known fails on this right now.

So thanks to IDynamicInterfaceCastable, the problem #91660 was trying to solve is totally unfixable and WinForms apps will always bring half of WPF with them.

Cc @dotnet/ilc-contrib

Author:	MichalStrehovsky
Assignees:	-
Labels:	area-NativeAOT-coreclr
Milestone:	-

[MichalStrehovsky]

/backport to release/8.0

[github-actions]

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6169862067

[jkotas]

We have DynamicInterfaceCastableImplementationAttribute that limits impact of IDynamicInterfaceCastable. Can we use that to only reflection-enable interfaces that have possibly valid IDynamicInterfaceCastable implementations?

[MichalStrehovsky]

We have DynamicInterfaceCastableImplementationAttribute that limits impact of IDynamicInterfaceCastable. Can we use that to only reflection-enable interfaces that have possibly valid IDynamicInterfaceCastable implementations?

The interface that the COM source generator reflects on is not the implementation interface.

In theory we don't even have a guarantee there is a DynamicInterfaceCastableImplementationAttribute interface in the system. The test I'm adding doesn't have that anywhere and still needs to see all the metadata.

[jkotas]

I think it would be perfectly fine to skip calling IDynamicInterfaceCastable callbacks for interfaces that cannot be possibly implemented by IDynamicInterfaceCastable. In other words, if the system can prove that the interface cannot be possibly implemented by an object by some means, it is fine to skip IDynamicInterfaceCastable callbacks.

[MichalStrehovsky]

I think it would be perfectly fine to skip calling IDynamicInterfaceCastable callbacks for interfaces that cannot be possibly implemented by IDynamicInterfaceCastable. In other words, if the system can prove that the interface cannot be possibly implemented by an object by some means, it is fine to skip IDynamicInterfaceCastable callbacks.

It's a lot of work to make an observable optimization around this that could break appcompat.

I don't think the savings we get from this are huge in general. We get a lot of savings when we can optimize constructed methodtable to unconstructed ones for classes (because classes have vtables). Interfaces are less interesting.

We'll have to tell WinForms team to divorce their package from WPF if they want good size so that the problematic attribute on ICommand doesn't resolve unless one actually references WPF. This would help everywhere, including untrimmed selfcontained deployments of WinForms apps.

[RussKie]

We're observing the failures on the release/8.0 branch -> dotnet/aspnetcore#50648 (comment). Is the fix being backported?

Didn't see https://github.com/dotnet/runtime/actions/runs/6169862067. All good.

[MichalStrehovsky]

This is a revert of #91660 (first commit, no need to review that, really) and a regression test (second commit).

This fixes an issue discovered in the SDK repo: dotnet/sdk#35293 (comment)

The compiler assumes MethodTables used in casts and interface dispatch are not reflection visible. As we found the hard way in the above PR, this is not true when IDynamicInterfaceCastable is present. With IDynamicInterfaceCastable, any (interface) type handle used in cast or interface dispatch could be reflection visible in user code.

The compiler had this wrong assumption for a while (the regression test I'm adding would fail on .NET 7). The only reason why we haven't already seen this in .NET 7 was that crossgen2 PDB writing was written with a manual ComWrapper in .NET 7, vs in .NET 8 it uses the generator that does a bunch of reflection on the type handle for whatever reason.

We'll probably also want to upgrade the places within the compiler that hand out NecessaryEEType for dispatch/cast to interfaces to hand out ConstructedEEType instead but I don't want to do that as part of a .NET 8 fix. This has been broken in 7.0 too and it is also observable (NecessaryEETypes don't have a populated interface list), but nothing known fails on this right now.

So thanks to IDynamicInterfaceCastable, the problem #91660 was trying to solve is totally unfixable and WinForms apps will always bring half of WPF with them.

Cc @dotnet/ilc-contrib

[ghost]

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

Issue Details

---

This is a revert of #91660 (first commit, no need to review that, really) and a regression test (second commit).

This fixes an issue discovered in the SDK repo: dotnet/sdk#35293 (comment)

The compiler assumes MethodTables used in casts and interface dispatch are not reflection visible. As we found the hard way in the above PR, this is not true when IDynamicInterfaceCastable is present. With IDynamicInterfaceCastable, any (interface) type handle used in cast or interface dispatch could be reflection visible in user code.

The compiler had this wrong assumption for a while (the regression test I'm adding would fail on .NET 7). The only reason why we haven't already seen this in .NET 7 was that crossgen2 PDB writing was written with a manual ComWrapper in .NET 7, vs in .NET 8 it uses the generator that does a bunch of reflection on the type handle for whatever reason.

We'll probably also want to upgrade the places within the compiler that hand out NecessaryEEType for dispatch/cast to interfaces to hand out ConstructedEEType instead but I don't want to do that as part of a .NET 8 fix. This has been broken in 7.0 too and it is also observable (NecessaryEETypes don't have a populated interface list), but nothing known fails on this right now.

So thanks to IDynamicInterfaceCastable, the problem #91660 was trying to solve is totally unfixable and WinForms apps will always bring half of WPF with them.

Cc @dotnet/ilc-contrib

Author:	MichalStrehovsky
Assignees:	-
Labels:	area-NativeAOT-coreclr
Milestone:	-

[MichalStrehovsky]

/backport to release/8.0

[github-actions]

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6169862067

[jkotas]

We have DynamicInterfaceCastableImplementationAttribute that limits impact of IDynamicInterfaceCastable. Can we use that to only reflection-enable interfaces that have possibly valid IDynamicInterfaceCastable implementations?

[MichalStrehovsky]

We have DynamicInterfaceCastableImplementationAttribute that limits impact of IDynamicInterfaceCastable. Can we use that to only reflection-enable interfaces that have possibly valid IDynamicInterfaceCastable implementations?

The interface that the COM source generator reflects on is not the implementation interface.

In theory we don't even have a guarantee there is a DynamicInterfaceCastableImplementationAttribute interface in the system. The test I'm adding doesn't have that anywhere and still needs to see all the metadata.

[jkotas]

I think it would be perfectly fine to skip calling IDynamicInterfaceCastable callbacks for interfaces that cannot be possibly implemented by IDynamicInterfaceCastable. In other words, if the system can prove that the interface cannot be possibly implemented by an object by some means, it is fine to skip IDynamicInterfaceCastable callbacks.

[MichalStrehovsky]

I think it would be perfectly fine to skip calling IDynamicInterfaceCastable callbacks for interfaces that cannot be possibly implemented by IDynamicInterfaceCastable. In other words, if the system can prove that the interface cannot be possibly implemented by an object by some means, it is fine to skip IDynamicInterfaceCastable callbacks.

It's a lot of work to make an observable optimization around this that could break appcompat.

I don't think the savings we get from this are huge in general. We get a lot of savings when we can optimize constructed methodtable to unconstructed ones for classes (because classes have vtables). Interfaces are less interesting.

We'll have to tell WinForms team to divorce their package from WPF if they want good size so that the problematic attribute on ICommand doesn't resolve unless one actually references WPF. This would help everywhere, including untrimmed selfcontained deployments of WinForms apps.

[RussKie]

We're observing the failures on the release/8.0 branch -> dotnet/aspnetcore#50648 (comment). Is the fix being backported?

Didn't see https://github.com/dotnet/runtime/actions/runs/6169862067. All good.

[MichalStrehovsky]

This is a revert of #91660 (first commit, no need to review that, really) and a regression test (second commit).

This fixes an issue discovered in the SDK repo: dotnet/sdk#35293 (comment)

The compiler assumes MethodTables used in casts and interface dispatch are not reflection visible. As we found the hard way in the above PR, this is not true when IDynamicInterfaceCastable is present. With IDynamicInterfaceCastable, any (interface) type handle used in cast or interface dispatch could be reflection visible in user code.

The compiler had this wrong assumption for a while (the regression test I'm adding would fail on .NET 7). The only reason why we haven't already seen this in .NET 7 was that crossgen2 PDB writing was written with a manual ComWrapper in .NET 7, vs in .NET 8 it uses the generator that does a bunch of reflection on the type handle for whatever reason.

We'll probably also want to upgrade the places within the compiler that hand out NecessaryEEType for dispatch/cast to interfaces to hand out ConstructedEEType instead but I don't want to do that as part of a .NET 8 fix. This has been broken in 7.0 too and it is also observable (NecessaryEETypes don't have a populated interface list), but nothing known fails on this right now.

So thanks to IDynamicInterfaceCastable, the problem #91660 was trying to solve is totally unfixable and WinForms apps will always bring half of WPF with them.

Cc @dotnet/ilc-contrib

[ghost]

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

Issue Details

---

This is a revert of #91660 (first commit, no need to review that, really) and a regression test (second commit).

This fixes an issue discovered in the SDK repo: dotnet/sdk#35293 (comment)

The compiler assumes MethodTables used in casts and interface dispatch are not reflection visible. As we found the hard way in the above PR, this is not true when IDynamicInterfaceCastable is present. With IDynamicInterfaceCastable, any (interface) type handle used in cast or interface dispatch could be reflection visible in user code.

The compiler had this wrong assumption for a while (the regression test I'm adding would fail on .NET 7). The only reason why we haven't already seen this in .NET 7 was that crossgen2 PDB writing was written with a manual ComWrapper in .NET 7, vs in .NET 8 it uses the generator that does a bunch of reflection on the type handle for whatever reason.

We'll probably also want to upgrade the places within the compiler that hand out NecessaryEEType for dispatch/cast to interfaces to hand out ConstructedEEType instead but I don't want to do that as part of a .NET 8 fix. This has been broken in 7.0 too and it is also observable (NecessaryEETypes don't have a populated interface list), but nothing known fails on this right now.

So thanks to IDynamicInterfaceCastable, the problem #91660 was trying to solve is totally unfixable and WinForms apps will always bring half of WPF with them.

Cc @dotnet/ilc-contrib

Author:	MichalStrehovsky
Assignees:	-
Labels:	area-NativeAOT-coreclr
Milestone:	-

[MichalStrehovsky]

/backport to release/8.0

[github-actions]

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6169862067

[jkotas]

We have DynamicInterfaceCastableImplementationAttribute that limits impact of IDynamicInterfaceCastable. Can we use that to only reflection-enable interfaces that have possibly valid IDynamicInterfaceCastable implementations?

[MichalStrehovsky]

We have DynamicInterfaceCastableImplementationAttribute that limits impact of IDynamicInterfaceCastable. Can we use that to only reflection-enable interfaces that have possibly valid IDynamicInterfaceCastable implementations?

The interface that the COM source generator reflects on is not the implementation interface.

In theory we don't even have a guarantee there is a DynamicInterfaceCastableImplementationAttribute interface in the system. The test I'm adding doesn't have that anywhere and still needs to see all the metadata.

[jkotas]

I think it would be perfectly fine to skip calling IDynamicInterfaceCastable callbacks for interfaces that cannot be possibly implemented by IDynamicInterfaceCastable. In other words, if the system can prove that the interface cannot be possibly implemented by an object by some means, it is fine to skip IDynamicInterfaceCastable callbacks.

[MichalStrehovsky]

I think it would be perfectly fine to skip calling IDynamicInterfaceCastable callbacks for interfaces that cannot be possibly implemented by IDynamicInterfaceCastable. In other words, if the system can prove that the interface cannot be possibly implemented by an object by some means, it is fine to skip IDynamicInterfaceCastable callbacks.

It's a lot of work to make an observable optimization around this that could break appcompat.

I don't think the savings we get from this are huge in general. We get a lot of savings when we can optimize constructed methodtable to unconstructed ones for classes (because classes have vtables). Interfaces are less interesting.

We'll have to tell WinForms team to divorce their package from WPF if they want good size so that the problematic attribute on ICommand doesn't resolve unless one actually references WPF. This would help everywhere, including untrimmed selfcontained deployments of WinForms apps.

[RussKie]

We're observing the failures on the release/8.0 branch -> dotnet/aspnetcore#50648 (comment). Is the fix being backported?

Didn't see https://github.com/dotnet/runtime/actions/runs/6169862067. All good.

[MichalStrehovsky]

This is a revert of #91660 (first commit, no need to review that, really) and a regression test (second commit).

This fixes an issue discovered in the SDK repo: dotnet/sdk#35293 (comment)

The compiler assumes MethodTables used in casts and interface dispatch are not reflection visible. As we found the hard way in the above PR, this is not true when IDynamicInterfaceCastable is present. With IDynamicInterfaceCastable, any (interface) type handle used in cast or interface dispatch could be reflection visible in user code.

The compiler had this wrong assumption for a while (the regression test I'm adding would fail on .NET 7). The only reason why we haven't already seen this in .NET 7 was that crossgen2 PDB writing was written with a manual ComWrapper in .NET 7, vs in .NET 8 it uses the generator that does a bunch of reflection on the type handle for whatever reason.

We'll probably also want to upgrade the places within the compiler that hand out NecessaryEEType for dispatch/cast to interfaces to hand out ConstructedEEType instead but I don't want to do that as part of a .NET 8 fix. This has been broken in 7.0 too and it is also observable (NecessaryEETypes don't have a populated interface list), but nothing known fails on this right now.

So thanks to IDynamicInterfaceCastable, the problem #91660 was trying to solve is totally unfixable and WinForms apps will always bring half of WPF with them.

Cc @dotnet/ilc-contrib

[ghost]

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

Issue Details

---

This is a revert of #91660 (first commit, no need to review that, really) and a regression test (second commit).

This fixes an issue discovered in the SDK repo: dotnet/sdk#35293 (comment)

The compiler assumes MethodTables used in casts and interface dispatch are not reflection visible. As we found the hard way in the above PR, this is not true when IDynamicInterfaceCastable is present. With IDynamicInterfaceCastable, any (interface) type handle used in cast or interface dispatch could be reflection visible in user code.

The compiler had this wrong assumption for a while (the regression test I'm adding would fail on .NET 7). The only reason why we haven't already seen this in .NET 7 was that crossgen2 PDB writing was written with a manual ComWrapper in .NET 7, vs in .NET 8 it uses the generator that does a bunch of reflection on the type handle for whatever reason.

We'll probably also want to upgrade the places within the compiler that hand out NecessaryEEType for dispatch/cast to interfaces to hand out ConstructedEEType instead but I don't want to do that as part of a .NET 8 fix. This has been broken in 7.0 too and it is also observable (NecessaryEETypes don't have a populated interface list), but nothing known fails on this right now.

So thanks to IDynamicInterfaceCastable, the problem #91660 was trying to solve is totally unfixable and WinForms apps will always bring half of WPF with them.

Cc @dotnet/ilc-contrib

Author:	MichalStrehovsky
Assignees:	-
Labels:	area-NativeAOT-coreclr
Milestone:	-

[MichalStrehovsky]

/backport to release/8.0

[github-actions]

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6169862067

[jkotas]

We have DynamicInterfaceCastableImplementationAttribute that limits impact of IDynamicInterfaceCastable. Can we use that to only reflection-enable interfaces that have possibly valid IDynamicInterfaceCastable implementations?

[MichalStrehovsky]

We have DynamicInterfaceCastableImplementationAttribute that limits impact of IDynamicInterfaceCastable. Can we use that to only reflection-enable interfaces that have possibly valid IDynamicInterfaceCastable implementations?

The interface that the COM source generator reflects on is not the implementation interface.

In theory we don't even have a guarantee there is a DynamicInterfaceCastableImplementationAttribute interface in the system. The test I'm adding doesn't have that anywhere and still needs to see all the metadata.

[jkotas]

I think it would be perfectly fine to skip calling IDynamicInterfaceCastable callbacks for interfaces that cannot be possibly implemented by IDynamicInterfaceCastable. In other words, if the system can prove that the interface cannot be possibly implemented by an object by some means, it is fine to skip IDynamicInterfaceCastable callbacks.

[MichalStrehovsky]

I think it would be perfectly fine to skip calling IDynamicInterfaceCastable callbacks for interfaces that cannot be possibly implemented by IDynamicInterfaceCastable. In other words, if the system can prove that the interface cannot be possibly implemented by an object by some means, it is fine to skip IDynamicInterfaceCastable callbacks.

It's a lot of work to make an observable optimization around this that could break appcompat.

I don't think the savings we get from this are huge in general. We get a lot of savings when we can optimize constructed methodtable to unconstructed ones for classes (because classes have vtables). Interfaces are less interesting.

We'll have to tell WinForms team to divorce their package from WPF if they want good size so that the problematic attribute on ICommand doesn't resolve unless one actually references WPF. This would help everywhere, including untrimmed selfcontained deployments of WinForms apps.

[RussKie]

We're observing the failures on the release/8.0 branch -> dotnet/aspnetcore#50648 (comment). Is the fix being backported?

Didn't see https://github.com/dotnet/runtime/actions/runs/6169862067. All good.

[MichalStrehovsky]

This is a revert of #91660 (first commit, no need to review that, really) and a regression test (second commit).

This fixes an issue discovered in the SDK repo: dotnet/sdk#35293 (comment)

The compiler assumes MethodTables used in casts and interface dispatch are not reflection visible. As we found the hard way in the above PR, this is not true when IDynamicInterfaceCastable is present. With IDynamicInterfaceCastable, any (interface) type handle used in cast or interface dispatch could be reflection visible in user code.

The compiler had this wrong assumption for a while (the regression test I'm adding would fail on .NET 7). The only reason why we haven't already seen this in .NET 7 was that crossgen2 PDB writing was written with a manual ComWrapper in .NET 7, vs in .NET 8 it uses the generator that does a bunch of reflection on the type handle for whatever reason.

We'll probably also want to upgrade the places within the compiler that hand out NecessaryEEType for dispatch/cast to interfaces to hand out ConstructedEEType instead but I don't want to do that as part of a .NET 8 fix. This has been broken in 7.0 too and it is also observable (NecessaryEETypes don't have a populated interface list), but nothing known fails on this right now.

So thanks to IDynamicInterfaceCastable, the problem #91660 was trying to solve is totally unfixable and WinForms apps will always bring half of WPF with them.

Cc @dotnet/ilc-contrib

[ghost]

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

Issue Details

---

This is a revert of #91660 (first commit, no need to review that, really) and a regression test (second commit).

This fixes an issue discovered in the SDK repo: dotnet/sdk#35293 (comment)

The compiler assumes MethodTables used in casts and interface dispatch are not reflection visible. As we found the hard way in the above PR, this is not true when IDynamicInterfaceCastable is present. With IDynamicInterfaceCastable, any (interface) type handle used in cast or interface dispatch could be reflection visible in user code.

The compiler had this wrong assumption for a while (the regression test I'm adding would fail on .NET 7). The only reason why we haven't already seen this in .NET 7 was that crossgen2 PDB writing was written with a manual ComWrapper in .NET 7, vs in .NET 8 it uses the generator that does a bunch of reflection on the type handle for whatever reason.

We'll probably also want to upgrade the places within the compiler that hand out NecessaryEEType for dispatch/cast to interfaces to hand out ConstructedEEType instead but I don't want to do that as part of a .NET 8 fix. This has been broken in 7.0 too and it is also observable (NecessaryEETypes don't have a populated interface list), but nothing known fails on this right now.

So thanks to IDynamicInterfaceCastable, the problem #91660 was trying to solve is totally unfixable and WinForms apps will always bring half of WPF with them.

Cc @dotnet/ilc-contrib

Author:	MichalStrehovsky
Assignees:	-
Labels:	area-NativeAOT-coreclr
Milestone:	-

[MichalStrehovsky]

/backport to release/8.0

[github-actions]

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6169862067

[jkotas]

We have DynamicInterfaceCastableImplementationAttribute that limits impact of IDynamicInterfaceCastable. Can we use that to only reflection-enable interfaces that have possibly valid IDynamicInterfaceCastable implementations?

[MichalStrehovsky]

We have DynamicInterfaceCastableImplementationAttribute that limits impact of IDynamicInterfaceCastable. Can we use that to only reflection-enable interfaces that have possibly valid IDynamicInterfaceCastable implementations?

The interface that the COM source generator reflects on is not the implementation interface.

In theory we don't even have a guarantee there is a DynamicInterfaceCastableImplementationAttribute interface in the system. The test I'm adding doesn't have that anywhere and still needs to see all the metadata.

[jkotas]

I think it would be perfectly fine to skip calling IDynamicInterfaceCastable callbacks for interfaces that cannot be possibly implemented by IDynamicInterfaceCastable. In other words, if the system can prove that the interface cannot be possibly implemented by an object by some means, it is fine to skip IDynamicInterfaceCastable callbacks.

[MichalStrehovsky]

I think it would be perfectly fine to skip calling IDynamicInterfaceCastable callbacks for interfaces that cannot be possibly implemented by IDynamicInterfaceCastable. In other words, if the system can prove that the interface cannot be possibly implemented by an object by some means, it is fine to skip IDynamicInterfaceCastable callbacks.

It's a lot of work to make an observable optimization around this that could break appcompat.

I don't think the savings we get from this are huge in general. We get a lot of savings when we can optimize constructed methodtable to unconstructed ones for classes (because classes have vtables). Interfaces are less interesting.

We'll have to tell WinForms team to divorce their package from WPF if they want good size so that the problematic attribute on ICommand doesn't resolve unless one actually references WPF. This would help everywhere, including untrimmed selfcontained deployments of WinForms apps.

[RussKie]

We're observing the failures on the release/8.0 branch -> dotnet/aspnetcore#50648 (comment). Is the fix being backported?

Didn't see https://github.com/dotnet/runtime/actions/runs/6169862067. All good.

[MichalStrehovsky]

This is a revert of #91660 (first commit, no need to review that, really) and a regression test (second commit).

This fixes an issue discovered in the SDK repo: dotnet/sdk#35293 (comment)

The compiler assumes MethodTables used in casts and interface dispatch are not reflection visible. As we found the hard way in the above PR, this is not true when IDynamicInterfaceCastable is present. With IDynamicInterfaceCastable, any (interface) type handle used in cast or interface dispatch could be reflection visible in user code.

The compiler had this wrong assumption for a while (the regression test I'm adding would fail on .NET 7). The only reason why we haven't already seen this in .NET 7 was that crossgen2 PDB writing was written with a manual ComWrapper in .NET 7, vs in .NET 8 it uses the generator that does a bunch of reflection on the type handle for whatever reason.

We'll probably also want to upgrade the places within the compiler that hand out NecessaryEEType for dispatch/cast to interfaces to hand out ConstructedEEType instead but I don't want to do that as part of a .NET 8 fix. This has been broken in 7.0 too and it is also observable (NecessaryEETypes don't have a populated interface list), but nothing known fails on this right now.

So thanks to IDynamicInterfaceCastable, the problem #91660 was trying to solve is totally unfixable and WinForms apps will always bring half of WPF with them.

Cc @dotnet/ilc-contrib

[ghost]

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

Issue Details

---

This is a revert of #91660 (first commit, no need to review that, really) and a regression test (second commit).

This fixes an issue discovered in the SDK repo: dotnet/sdk#35293 (comment)

The compiler assumes MethodTables used in casts and interface dispatch are not reflection visible. As we found the hard way in the above PR, this is not true when IDynamicInterfaceCastable is present. With IDynamicInterfaceCastable, any (interface) type handle used in cast or interface dispatch could be reflection visible in user code.

The compiler had this wrong assumption for a while (the regression test I'm adding would fail on .NET 7). The only reason why we haven't already seen this in .NET 7 was that crossgen2 PDB writing was written with a manual ComWrapper in .NET 7, vs in .NET 8 it uses the generator that does a bunch of reflection on the type handle for whatever reason.

We'll probably also want to upgrade the places within the compiler that hand out NecessaryEEType for dispatch/cast to interfaces to hand out ConstructedEEType instead but I don't want to do that as part of a .NET 8 fix. This has been broken in 7.0 too and it is also observable (NecessaryEETypes don't have a populated interface list), but nothing known fails on this right now.

So thanks to IDynamicInterfaceCastable, the problem #91660 was trying to solve is totally unfixable and WinForms apps will always bring half of WPF with them.

Cc @dotnet/ilc-contrib

Author:	MichalStrehovsky
Assignees:	-
Labels:	area-NativeAOT-coreclr
Milestone:	-

[MichalStrehovsky]

/backport to release/8.0

[github-actions]

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6169862067

[jkotas]

We have DynamicInterfaceCastableImplementationAttribute that limits impact of IDynamicInterfaceCastable. Can we use that to only reflection-enable interfaces that have possibly valid IDynamicInterfaceCastable implementations?

[MichalStrehovsky]

We have DynamicInterfaceCastableImplementationAttribute that limits impact of IDynamicInterfaceCastable. Can we use that to only reflection-enable interfaces that have possibly valid IDynamicInterfaceCastable implementations?

The interface that the COM source generator reflects on is not the implementation interface.

In theory we don't even have a guarantee there is a DynamicInterfaceCastableImplementationAttribute interface in the system. The test I'm adding doesn't have that anywhere and still needs to see all the metadata.

[jkotas]

I think it would be perfectly fine to skip calling IDynamicInterfaceCastable callbacks for interfaces that cannot be possibly implemented by IDynamicInterfaceCastable. In other words, if the system can prove that the interface cannot be possibly implemented by an object by some means, it is fine to skip IDynamicInterfaceCastable callbacks.

[MichalStrehovsky]

I think it would be perfectly fine to skip calling IDynamicInterfaceCastable callbacks for interfaces that cannot be possibly implemented by IDynamicInterfaceCastable. In other words, if the system can prove that the interface cannot be possibly implemented by an object by some means, it is fine to skip IDynamicInterfaceCastable callbacks.

It's a lot of work to make an observable optimization around this that could break appcompat.

I don't think the savings we get from this are huge in general. We get a lot of savings when we can optimize constructed methodtable to unconstructed ones for classes (because classes have vtables). Interfaces are less interesting.

We'll have to tell WinForms team to divorce their package from WPF if they want good size so that the problematic attribute on ICommand doesn't resolve unless one actually references WPF. This would help everywhere, including untrimmed selfcontained deployments of WinForms apps.

[RussKie]

We're observing the failures on the release/8.0 branch -> dotnet/aspnetcore#50648 (comment). Is the fix being backported?

Didn't see https://github.com/dotnet/runtime/actions/runs/6169862067. All good.