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

Deprecate and remove Xamarin types which conflict with predefined .NET types #10508

Closed
Tracked by #44736
marek-safar opened this issue Jan 24, 2021 · 21 comments · Fixed by #13490
Closed
Tracked by #44736

Deprecate and remove Xamarin types which conflict with predefined .NET types #10508

marek-safar opened this issue Jan 24, 2021 · 21 comments · Fixed by #13490
Assignees
Labels
breaking-change If an issue or a pull request represents a breaking change dotnet An issue or pull request related to .NET (6) estimate-4w iOS Issues affecting iOS macOS Issues affecting macOS
Milestone

Comments

@marek-safar
Copy link
Contributor

marek-safar commented Jan 24, 2021

As part of .NET 6 migration existing types that are not compatible or serve for exactly the same purpose should be removed or at least deprecated to allow Xamarin SDK to work with all .NET libraries.

nint/nuint types

These types in Xamarin have a bit different semantic than in C# and I think the best approach would be to remove them from public API. I didn't try what happens when C# compiler see them defined as types but they should certainly not be prefered types over C# built-in version.

CLong/CULong/NFloat types

The existing types which serve exactly the same needs should be deprecated to allow easier sharing for .NET libraries.


Time estimate: depends on what we decide to do. Worst case is probably ~4 weeks to remove the Xamarin types and deal with the fallout, best case is no time at all if we decide to do nothing.

@AaronRobinsonMSFT
Copy link
Contributor

I believe an issue with removing these types and not handling this in CoreCLR is that legacy Xamarin assemblies wouldn't be supported. Please let me know if I am missing something.

/cc @Redth @rolfbjarne @jkotas

@Redth
Copy link
Member

Redth commented Jan 24, 2021

@dalexsoto @spouliot @samhouts @jonpryor as well.

@rolfbjarne
Copy link
Member

These types in Xamarin have a bit different semantic than in C# and I think the best approach would be to remove them from public API.

This means that pretty much every existing Xamarin.iOS assembly (including NuGets) will not be compatible with .NET 6.

Also people will probably have to update their source code to make it compile.

I didn't try what happens when C# compiler see them defined as types but they should certainly not be prefered types over C# built-in version.

The spec says:

"The identifiers are only treated as keywords when name lookup does not find a viable result at that program location."

@rolfbjarne rolfbjarne added iOS Issues affecting iOS macOS Issues affecting macOS labels Jan 25, 2021
@rolfbjarne rolfbjarne added this to the .NET 6 milestone Jan 25, 2021
@rolfbjarne rolfbjarne added the request-for-comments The issue is a suggested idea seeking feedback label Jan 25, 2021
@marek-safar
Copy link
Contributor Author

This means that pretty much every existing Xamarin.iOS assembly (including NuGets) will not be compatible with .NET 6.

It's not a new problem in .NET6, a similar problem exists today. It's not as visible because we didn't upgrade Mono bundled C# to version 9 but it's the same problem. .NET library targetting multiple platforms cannot use these types in the same way.

Also people will probably have to update their source code to make it compile.

I think the migration tool should try to cover this as much as possible

@dalexsoto dalexsoto added the breaking-change If an issue or a pull request represents a breaking change label Jan 25, 2021
@rolfbjarne rolfbjarne added dotnet-pri0 .NET 6: required for stable release dotnet An issue or pull request related to .NET (6) labels Feb 3, 2021
@rolfbjarne
Copy link
Member

I haven't heard about a single customer (internal or external) running into any problem with the existing situation (where we have and use our own version of these types), so I don't think we need to do anything here.

Everything just works as-is (and I'm quite reluctant to rock that boat).

One point to have in mind is that our types are Apple-specific, which means they only show up in our bindings, which also means they will never be a part of platform-neutral code.

@Perksey
Copy link

Perksey commented Aug 18, 2021

Just attempted to add support for .NET 6 in Silk.NET and it failed because we use C# 9 nint and nuint. This creates massive complications in upgrade paths, particularly in more native-oriented applications which took advantage of C# 9's new types (such as bindings libraries).

The most important scenario not considered here is source generators - they will expect using System to just work and may also generate public APIs using these new language features - massively blocking!

@tannergooding
Copy link

tannergooding commented Aug 18, 2021

Everything just works as-is

I don't think this is strictly true. There are a number of things that don't or won't work with the System.nint and System.nuint types including constants, default parameter values, interop (for certain scenarios, possibly not applicable to iOS), checked arithmetic, and others.

I'd expect its simply that customers haven't hit them yet and will in due time as codebases which take/use/expose C# nint/nuint start interacting more with Xamarin apps using System.nint and System.nuint.

Edit: I'm not saying this does or does not warrant a break. Just calling out that there will be downstream repercussions in either scenario and that will expose themselves over time, particularly when non-preview bits start getting picked up and applications are ported. The BCL itself is going to continue using and exposing C# nint/nuint in APIs, including core performance APIs such as System.Numerics.BitOperations, System.Math, and the SIMD/HWIntrinsic APIs (many of which were already extended to support C# nint/nuint for .NET 6 and which have additional approved surface area for .NET 7).

@Perksey
Copy link

Perksey commented Aug 20, 2021

I definitely think leaving these types in will cause more long-term hurt to the ecosystem that short-term - it is massively preventing the adoption of language features which will become much more prominent (as Tanner has discussed). Old codebases will port over fine, but any new codebase is essentially banned from importing System (as we have found in the Silk.NET migration) and required to disable implicit usings (for the same reason) - another language feature which can't be adopted because of these types.

These types will cause more harm than good in the "One .NET" ecosystem which in my opinion isn't worth it (is it really "One .NET" if you have to make special considerations for just a couple of TFMs?).

.NET 6 iOS is effectively a new framework (despite carrying old baggage), and I think work in making this as easy as possible should be done in migration paths (tools).

Yes, it's nice being able to just drop in existing Xamarin DLLs and it just work. But considering this is the future of .NET, I would favour the future of the C# language and the future codebases over the old ones which have fallen into the pit created by these types, and give existing users a ladder out of said pit in the form of migration tooling.

"Evidence":

@rolfbjarne rolfbjarne self-assigned this Aug 27, 2021
@hamarb123
Copy link

Hi, I just ran into this issue today (noticed it before though). I think a good solution (that preserves both compatibility and solves the C# 9 issue) could be to add a property to the .csproj file when targeting netx.y-macos that disables importing System.nuint and System.nint (and whatever other relevant types) when using System etc. It could be enabled and disabled and users would just have to write System.nuint and System.nint instead of nint and nuint to be able to use these types, thus preserving the ability to use the C# 9 types. I think it could be enabled by default (when langversion ≥ 9) but easily disabled with something like:

<DisableUsingsForNativeMacNumericalTypes>false</DisableUsingsForNativeMacNumericalTypes>

The issue with this solution is it could possibly require significant changes to the C# compiler, Visual Studio? (for intellisense), and a new command-line option for csc, but the work here could possibly be useful in the future if there are any other similar issues with other platform-specific types. But I personally think that this is the best solution as it preserves compatibility and allows the use of new features (https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-9.0/native-integers).
Also keep in mind, C# 9 nint has different compile-time semantics to IntPtr, so this does need to be fixed at some point so that nint can be used.
Thanks for a wonderful platform!

@filipnavara
Copy link
Contributor

filipnavara commented Sep 7, 2021

I am afraid there's no easy way to keep the binary compatibility and at the same time use the new types on .NET 6. The closest idea I had to get that working was the following:

  1. Keep System.nint/System.nuint in Xamarin.iOS/Mac as-is
  2. Add overloads with IntPtr/UIntPtr to all APIs where nint/nuint overloads exist with [NativeIntegerAttribute] attribute (or whatever the new C# compiler generates)
  3. Ship a C#9+ reference assembly where the old System.nint/System.nuint overloads don't exist and neither do the types

Old binaries would still use the System.nint/System.nuint overloads and types. New binaries would be compiled against the reference assembly and have no idea about the nint/nuint types.

However, if this even worked in the first place it potentially duplicates a lot of code (generated one for the most part).

@rolfbjarne
Copy link
Member

@filipnavara,

Unfortunately it won't work all the time:

2. Add overloads with IntPtr/UIntPtr to all APIs where nint/nuint overloads exist with [NativeIntegerAttribute] attribute (or whatever the new C# compiler generates)

You can't overload based on return type (so for API that return nint/nuint, you can't create an IntPtr/UIntPtr version). The most frequent example is probably NSArray.Length (and related APIs).

There might also already be an IntPtr overload (for some other purpose), in which case you can't add a new one either.

@filipnavara
Copy link
Contributor

Yeah, I figured there would probably be cases like that (hence "if this even worked in the first place")... Too bad :-/

@Perksey
Copy link

Perksey commented Sep 7, 2021

I agree the situation isn't really easily solvable. When would be the time for such a break? I know there were talks of a XAMCORE4_0 back when we were battling with the OpenTK issue. It feels the introduction of a brand new framework is a perfect opportunity to break.

Given confirmation here from someone on the libraries team that long-term these new language features are going to be adopted throughout the BCL herein, it seems disingenuous to not take the opportunity to remove APIs that prevent adoption of those same features - it's not "one .NET" if one team developing the single platform is going directly against another, and causing widespread issues in the adoption of new features in the ever evolving C# language.

Issues such as these completely get in the way of the breath of fresh air many were hoping for with "one .NET". As it currently stands, our internal applications are going to only be using the iOS infrastructure to call into a regular .NET library with custom bindings because of these blockers - a duplication of effort which can be avoided.

@rolfbjarne rolfbjarne modified the milestones: .NET 6, .NET 7 Sep 10, 2021
@HurricanKai
Copy link

Given this is now in the .NET 7 milestone, can we get some guidance on what library authors should do right now?
In a library I want to use the new C# definitions, but it appears there is no real way for .NET 6 users to use the library given they are targeting iOS.
I effectively have to pass this headache down to all users or refrain from using a new language feature.

@rolfbjarne
Copy link
Member

In a library I want to use the new C# definitions, but it appears there is no real way for .NET 6 users to use the library given they are targeting iOS.

As far as I know, it's not impossible, just quite clunky.

The workaround (for any consumer of your library that wants to use your library with the C# nint types and our System.nint type in their own code) is to (for every source file where they want to use both types):

  • Avoid using System; (because this brings in our nint type, and there's no way to get the C# nint type).
    • The System namespace is imported by default in new projects in .NET 6; IIRC this can be turned off by adding something like this to the project file:
    <ItemGroup>
        <Using Remove="System" />
    </ItemGroup>
  • Explicitly write System.nint every time they want to use that type.

@HurricanKai
Copy link

imo this isn't a real solution, it passes a huge headache down to all users targeting iOS, and they probably end up having to somehow move usage of my library out of the main code, as the System namespace isn't really something you can just avoid...

@hamarb123
Copy link

hamarb123 commented Sep 17, 2021

I agree that this is not a good solution. Perhaps the types could be moved somewhere else eg. System.Runtime.InteropServices.XamarinMac (namespace) and then people who need to have their code maintained across 2 versions for a while can do conditional usings (all that would be necessary for these users is to import the additional namespace with a #if - this could even be done with a global import and it would essentially be a non-issue at that point), because in the future this is going to be a big issue. I also think it's a shame this isn't going to be fixed for .NET 6, because that will just increase the amount of conditional statements that are required from essentially 'am I using .NET Core' to 'am I using .NET core that is at least 7.0' which will probably require the programmer to make conditional constants in their csproj themselves. But I can see how this will cause major issues with using old Xamarin.Mac libraries.
Note, I specifically suggest System.Runtime.InteropServices.XamarinMac rather than System.Runtime.InteropServices because it's not uncommon to have a use for nints if you're using interopsevices. I also think nfloat should be moved there because I wouldn't be surprised if future .NET has an nfloat type (but I also wouldn't be surprised if it didn't).

@tannergooding
Copy link

Note, I specifically suggest System.Runtime.InteropServices.XamarinMac rather than System.Runtime.InteropServices because it's not uncommon to have a use for nints if you're using interopsevices. I also think nfloat should be moved there because I wouldn't be surprised if future .NET has an nfloat type (but I also wouldn't be surprised if it didn't).

.NET 6 actually has this already as NFloat (native sized floating-point): https://docs.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.nfloat?view=net-6.0. Alongside CLong (C/C++ long) and CULong (C/C++ unsigned long) these types are explicitly for interop purposes and are specially handled by the marshaller to treat them as "ABI primitives" rather than as "User-defined structs" (because those two types behave differently in some scenarios).

The major difference is that the general .NET 6 types follow the standard naming convention (PascalCase) and don't support operators as of today (they could in the future if enough demand existed).

@rolfbjarne rolfbjarne removed the dotnet-pri0 .NET 6: required for stable release label Oct 5, 2021
@NogginBops
Copy link

NogginBops commented Oct 10, 2021

If this ever gets tackled I think that is the perfect time to finally solve the OpenTK issue that has been present since 2014 and continues to be present to this day for users of OpenTK (#5275 #5107 mono/opentk#19).

People are stuck with the 1.0 version of OpenTK.dll and I can say as a maintainer that the only reason why OpenTK doesn't support android and iOS is because of this issue. And it's also the only reason OpenTK isn't used for android/iOS development.

This issue has had some heated discussion before and it definitely is a bad look for Xamarin (and now by extension Microsoft) to be continually blocking users from using OpenTK and us from developing OpenTK.

I understand that this is a complicated issue, but it should really have been solved with .net 5 but wasn't so now we are waiting for .net 7 to become what .net 5 and .net 6 promised to be.

@rolfbjarne rolfbjarne removed the request-for-comments The issue is a suggested idea seeking feedback label Oct 22, 2021
@rolfbjarne rolfbjarne modified the milestones: .NET 7, .NET 6 Oct 22, 2021
@rolfbjarne
Copy link
Member

We've decided that we're going to remove the existing System.nint and System.nuint types in .NET 6, so this will be fixed.

More information here: #13087

Note that this is a breaking change, and everything will have to be recompiled for .NET 6 (existing NuGets and assemblies built for legacy Xamarin.iOS or Xamarin.Mac won't work).

@Perksey
Copy link

Perksey commented Oct 22, 2021

@rolfbjarne Life saver, thank you so much :)

rolfbjarne added a commit that referenced this issue Dec 7, 2021
…13490)

* Remove System.nint and System.nuint from .NET
* Add support for C#'s nint/nuint types to the generator.
* Accept IntPtr/UIntPtr as target types for BindAs attributes for NSNumber conversions.
* Fix a few APIs to take/return NativeHandle instead of IntPtr.

Fixes #10508.
@ghost ghost locked as resolved and limited conversation to collaborators Apr 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking-change If an issue or a pull request represents a breaking change dotnet An issue or pull request related to .NET (6) estimate-4w iOS Issues affecting iOS macOS Issues affecting macOS
Projects
None yet
Development

Successfully merging a pull request may close this issue.