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

Add missing type forwards #90669

Merged
merged 8 commits into from
Aug 17, 2023
Merged

Add missing type forwards #90669

merged 8 commits into from
Aug 17, 2023

Conversation

ViktorHofer
Copy link
Member

Fixes #90578

IDispatchImplAttribute, IDispatchImplType and SetWin32ContextInIDispatchAttribute were removed with 9f1dd1a and 26a91ad

Those dropped types weren't flagged because APICompat only validates reference assemblies. We have three implementation only shim assemblies: mscorlib, System and System.Data. I verified that no other type forwards were lost between .NET 7 and .NET 8.

Fixes #90578

IDispatchImplAttribute, IDispatchImplType and SetWin32ContextInIDispatchAttribute were removed with 9f1dd1a and 26a91ad

Those dropped types weren't flagged because APICompat only validates reference assemblies. We have three implementation only shim assemblies: mscorlib, System and System.Data. I verified that no other type forwards were lost between .NET 7 and .NET 8.
@ghost
Copy link

ghost commented Aug 16, 2023

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #90578

IDispatchImplAttribute, IDispatchImplType and SetWin32ContextInIDispatchAttribute were removed with 9f1dd1a and 26a91ad

Those dropped types weren't flagged because APICompat only validates reference assemblies. We have three implementation only shim assemblies: mscorlib, System and System.Data. I verified that no other type forwards were lost between .NET 7 and .NET 8.

Author: ViktorHofer
Assignees: ViktorHofer
Labels:

area-Infrastructure-libraries

Milestone: -

@ViktorHofer
Copy link
Member Author

/backport to release/8.0-rc1

@github-actions
Copy link
Contributor

Started backporting to release/8.0-rc1: https://github.com/dotnet/runtime/actions/runs/5881442922

@jkotas
Copy link
Member

jkotas commented Aug 16, 2023

IDispatchImplAttribute, IDispatchImplType and SetWin32ContextInIDispatchAttribute

Do we need to add these (unused) types back for compat?

@jkotas
Copy link
Member

jkotas commented Aug 16, 2023

IDispatchImplAttribute, IDispatchImplType and SetWin32ContextInIDispatchAttribute

Do we need to add these (unused) types back for compat?

Both the types that you are adding the forwards for and the types that we happened to delete have same characteristics: Never exposed in .NET Core/5+ public surface, never actually used in .NET 5, included just to make compat shims work. If we are keeping them, we should keep all of them and undo the deletion.

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Aug 16, 2023

Do we need to add these (unused) types back for compat?

We could add them directly into mscorlib's implementation assembly as stubs (without type forwarding to System.Runtime.InteropServices) so that people only pay for them when they use the .NET Framework shims. cc @AaronRobinsonMSFT

It's not clear to me why they were added to the mscorlib runtime shim in the first place though. The mscorlib runtime shim was only created back then for binary formatter serialization payload compatibility. Those types might have unintentionally been added as part of that change...

@AaronRobinsonMSFT
Copy link
Member

We could add them directly into mscorlib's implementation assembly as stubs

Can someone clarify why/how these are coming up? As @jkotas mentioned and I did on the original PR, these types were never in a .NET Core ref assembly. There was no way for someone to target their code to a .NET Core TFM and compile successfully, right? That last part I am asking because I really thought that was true.

My preference here would be to not speak them ever again in a .NET Core scenario. I am unclear under what circumstances they would cause issues at this point.

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Aug 16, 2023

When loading a .NET Framework compiled assembly that uses these types on .NETCoreApp, you would see a TypeLoadException with their removal. These existed in mscorlib.dll in .NET Framework. In .NETCoreApp, we also have an mscorlib.dll assembly which is just a shim assembly that type forwards to the actual location (System.Runtime.InteropServices for the removed types).

@jkotas
Copy link
Member

jkotas commented Aug 16, 2023

The scenario is that you compiled a library for .NET Framework and then run that library on .NET Core. This is documented as .NET Framework compatibility mode: https://learn.microsoft.com/en-us/dotnet/core/porting/#net-framework-compatibility-mode

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Aug 16, 2023

I just pushed to the branch to experiment with what I mentioned above. Thoughts on that?

We could add them directly into mscorlib's implementation assembly as stubs (without type forwarding to System.Runtime.InteropServices) so that people only pay for them when they use the .NET Framework shims.

@AaronRobinsonMSFT
Copy link
Member

The scenario is that you compiled a library for .NET Framework and then run that library on .NET Core.

I have a hard time with this policy at the interop boundary. These APIs represent semantic behavior that is hard at the best of times to root out and in this case them becoming no-ops turns this into a really difficult support scenario. Personally, I think APIs that influence complex behavior (i.e., interop scenarios) should fail to load because users will have no idea what may or may not happen given the context of their usage in most cases.

I understand the policy in general, but I think interop here is best left to "fail to load" rather than "sure, why not?".

@jkotas
Copy link
Member

jkotas commented Aug 16, 2023

I understand the policy in general, but I think interop here is best left to "fail to load" rather than "sure, why not?".

If you would like to prefer this instead of adding the delete types back, it is fine with me - it needs a breaking change notification filled.

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Aug 16, 2023

I'm fine with that as well but regarding Jan's point above:

Both the types that you are adding the forwards for and the types that we happened to delete have same characteristics: Never exposed in .NET Core/5+ public surface, never actually used in .NET 5, included just to make compat shims work.

These other types, i.e. https://github.com/dotnet/runtime/blob/7a11cff914fa6fcf00eb59be33306e3826e8b958/src/libraries/System.Runtime.InteropServices/src/System/Runtime/InteropServices/RegistrationConnectionType.cs#L7C21-L7C21 aren't referenced anywhere and aren't exposed. Why do we keep those but delete the others?

@AaronRobinsonMSFT
Copy link
Member

I understand the policy in general, but I think interop here is best left to "fail to load" rather than "sure, why not?".

If you would like to prefer this instead of adding the delete types back, it is fine with me - it needs a breaking change notification filled.

BCN: dotnet/docs#36729

@ViktorHofer ViktorHofer merged commit 296a1d5 into main Aug 17, 2023
102 of 105 checks passed
@ViktorHofer ViktorHofer deleted the ViktorHofer-patch-2 branch August 17, 2023 07:12
@ghost ghost locked as resolved and limited conversation to collaborators Sep 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NET8 breaking change due to TypeForwardedTo attribute missing for PEFileKinds
5 participants