Closed tannergooding closed 2 years ago
Tagging subscribers to this area: @dotnet/area-system-numerics See info in area-owners.md if you want to be subscribed.
Author: | tannergooding |
---|---|
Assignees: | - |
Labels: | `area-System.Numerics`, `untriaged` |
Milestone: | - |
CC. @rolfbjarne, @jeffhandley, @jkotas
Do we really believe that this will actually help Xamarin? The plan so far has been to ship MAUI and Xamarin on .NET 6.
Do we really believe that this will actually help Xamarin? The plan so far has been to ship MAUI and Xamarin on .NET 6.
Xamarin is making a breaking change for nint
/nuint
. As per https://github.com/xamarin/xamarin-macios/issues/13087#issuecomment-1012968826, this would allow them to make a similar breaking change for their System.nfloat
(which is taking a breaking change regardless due to the need to move into a separate namespace).
This ultimately allows a more consistent ecosystem long term, will avoid exposing/preserving types that may conflict with language keywords, and is not a lot of work on our end given we are reusing the underlying implementations from Single
/Double
.
I also don't think it would be "unreasonable" to look at backporting these changes to .NET 6 alongside MAUI/Xamarin. Given the benefits of keeping a "single type" in the ecosystem, it would likely be worth a bar check.
The downside of doing so would be that whether or not these APIs are available depends on which servicing patch you have installed; but that is realistically no different from consuming Xamarin/MAUI on .NET 6 in the first place.
Probably worth mentioning the conversation in the past 8 hours or so on https://github.com/xamarin/xamarin-macios/issues/13087 seems to indicate team Xamarin are on-board, and are fine with developers porting to .NET 6 instead of .NET 7 having to work a little harder.
I am still of an opinion that it would be better to just use double
as the common type if we were to make more breaking changes in Xamarin: https://github.com/xamarin/xamarin-macios/issues/13087#issuecomment-950928894 and use NFloat for low-level interop only.
Also, I do not think having NFloat in the platform for low-level interop only and nfloat in Xamarin for legacy compat is too bad.
If you would like to go out of your way to unify nfloat
and NFloat
, I am ok with it. Just make sure you have end-to-end plan figured out...
@tannergooding Is the intention to open another (non-blocking) issue for CLong and CULong for the Future or is expansion of these style of types just going to be use case driven for now?
Xamarin is making a breaking change for nint/nuint.
This breaking change was made to fix very ugly experience with nint
and nuint
. We had two different things with the same name that were colliding. This problem does not exist for nfloat
/ NFloat
. The benefit of unifying nfloat
/ NFloat
is tiny compared to benefit of unifying nint
/nuint
.
If you would like to go out of your way to unify nfloat and NFloat
I'm still not convinced its "out of our way". I think its general goodness to make it consistent with the underlying API and make it simple for devs to work with. We aren't telling or expecting users to use long
/ulong
rather than nint
/nuint
and other languages tend to have aliases or typedefs that allow correct sizing and operator usage as well for their CGFloat
alternatives.
Ultimately, we are mirroring the Single
/Double
surface area and forwarding down to it, so the risk of bugs is low. It makes it convenient and natural to use the type without having to worry about potentially lossy behavior at every boundary. It "makes sense" in the context of the general generic math work for .NET 7. It simplifies the exchange story between Xamarin and other libraries that may also be using NFloat
already. Etc.
Just make sure you have end-to-end plan figured out...
I'll make sure to coordinate with @rolfbjarne and @jeffhandley to ensure everything is covered here, including the experience that this would have on .NET 6.
This breaking change was made to fix very ugly experience with nint and nuint. We had two different things with the same name that were colliding. This problem does not exist for nfloat / NFloat. The benefit of unifying nfloat / NFloat is tiny compared to benefit of unifying nint/nuint.
Agreed and if as part of the move Xamarin renamed from nfloat
(lowercase) to NFloat
or CGFloat
, that would help alleviate future compat concerns among other things. However, I still believe it is ultimately and long-term worthwhile; including as part of the general Apple interop story.
Even with Apple largely dropping 32-bit support; there is no guarantee it stays that way "long term" and that they won't introduce new platforms that target 32-bit, even if simply for low-power/embedded scenarios.
I don't think it is worthwhile for us to make decisions about another platforms potential directions and so staying consistent with their definition, which is CGFloat
and matches the underlying pointer width, is the "best" and "most correct" choice.
Is the intention to open another (non-blocking) issue for CLong and CULong for the Future or is expansion of these style of types just going to be use case driven for now?
I will raise that this may also apply to CLong
and CULong
in API review. I'm specifically targeting "just" NFloat
here since that is a "blocking" decision for Xamarin and the direction they would take.
I'll make sure to coordinate with @rolfbjarne and @jeffhandley to ensure everything is covered here
Also include @marek-safar
I'm onboard with introducing the APIs in .NET 7. We cannot service .NET 6 to add the new API surface area, so customers and internal code would have to apply the workaround when targeting .NET 6.
Is it possible to back port to the ios/maccatalyst/macos TFM's for net6.0 only?
I believe the issue is that since its in System.Runtime
, it would functionally require publishing a .NET 6.1
release and that has cost that would need to be justified before it could happen.
The current plan here is:
nfloat
type (among the other benefits I gave above)The outcome of that will either be that Mono is willing to take this with the downsides or not. If they aren't, then we likely won't be implementing these helper operators on the type (at least not without additional justification, etc).
If they are willing to take it, but only if .NET 6 can get the same; that would have to goto shiproom and provide the relevant justification on revving .NET 6. I imagine it would not be easy to sell.
the final details including the caveats
Also, take the customer experience into account. I expect many people will be updating their old Xamarin projects. They will need to be adding extra usings, renaming nfloat to NFloat in number of places and doing other adjustments.
I am still of an opinion that it would be better to just use
double
as the common type if we were to make more breaking changes in Xamarin: xamarin/xamarin-macios#13087 (comment) and use NFloat for low-level interop only.
In the past we've sometimes been bitten when we've not exposed Apple's API faithfully, which is why we've decided to keep exposing nfloat
(or NFloat
).
On a different note, exposing double
and converting to float
becomes somewhat complex and not very performant when the native API takes (or returns) a 4x4 matrix for float
s or even an array of float
s (and this is in code customers typically want to perform reasonably fast).
Also, I do not think having NFloat in the platform for low-level interop only and nfloat in Xamarin for legacy compat is too bad.
If you would like to go out of your way to unify
nfloat
andNFloat
, I am ok with it. Just make sure you have end-to-end plan figured out...
We do have an end-to-end plan (time will tell if customers will accept that plan, but the reason we're doing it is because we've gotten push-back from customers on having keeping our nfloat
).
I'll make sure to coordinate with @rolfbjarne and @jeffhandley to ensure everything is covered here, including the experience that this would have on .NET 6.
If this proposal is approved, I'd like to first remove our nfloat
type in our codebase, and see how it affects our tests code, etc (which would be close to what the .NET 6 experience for customers). I'll then report my findings, and we can make a final decision whether we want to remove our type (and thus if we need this proposal).
public NFloat Epsilon { get; } public NFloat MaxValue { get; } public NFloat MinValue { get; } public NFloat NaN { get; } public NFloat NegativeInfinity { get; } public NFloat PositiveInfinity { get; } public NFloat Size { get; }
Are these really meant to be instance properties?
public static explicit operator nuint(decimal value); // Xamarin is missing this one
I'm guessing this should be:
public static explicit operator nuint(NFloat value); // Xamarin is missing this one
Are these really meant to be instance properties?
No, meant to be static
and mirror Double
/Half
/Single
. Fixed.
I'm guessing this should be:
Yes. Fixed.
Is it possible to back port to the ios/maccatalyst/macos TFM's for net6.0 only?
I think we should be able to add that API to these ios/maccatalyst TFM's only if that makes the developer's life easier as we are going to ship new runtime packs anyway. Adding it for macos too would be a bit tricky.
Is it possible to back port to the ios/maccatalyst/macos TFM's for net6.0 only?
I think we should be able to add that API to these ios/maccatalyst TFM's only if that makes the developer's life easier as we are going to ship new runtime packs anyway. Adding it for macos too would be a bit tricky.
How would that work if we compile using the reference assembly from the general Microsoft.NETCore.App.Ref
pack?
Right, the public API would have to in the ref packs too. netcoreapp ref pack is not OS specific. I agree with @tannergooding that we would have to create net6.1 and associated TFMs https://github.com/dotnet/runtime/issues/63801#issuecomment-1013921776 to back port this for the MAUI release only.
I agree with @tannergooding that we would have to create net6.1 and associated TFMs #63801 (comment) to back port this for the MAUI release only.
Yes, this was the feedback @ericstj and I had shared offline with @jeffhandley as well. I also do not see enough justification to incur such a release.
Unless there's a very strong need for IConvertable, we should leave that out. Otherwise, looks good as proposed.
namespace System.Runtime.InteropServices;
public readonly struct NFloat
: IComparable,
IComparable<NFloat>,
IEquatable<NFloat>, // Already exposed by 13788
IFormattable
{
// Xamarin exposes these as `static readonly` but that is "less optimizable" and not our normal convention
public static NFloat Epsilon { get; }
public static NFloat MaxValue { get; }
public static NFloat MinValue { get; }
public static NFloat NaN { get; }
public static NFloat NegativeInfinity { get; }
public static NFloat PositiveInfinity { get; }
public static NFloat Size { get; }
public static bool IsInfinity(NFloat value);
public static bool IsNaN(NFloat value);
public static bool IsNegativeInfinity(NFloat value);
public static bool IsPositiveInfinity(NFloat value);
// Parsing APIs, would match IParseable<NFloat> and IBinaryFloatingPoint<NFloat> if exposed
public static NFloat Parse(string s);
public static NFloat Parse(string s, NumberStyles style);
public static NFloat Parse(string s, IFormatProvider? provider);
public static NFloat Parse(string s, NumberStyles style, IFormatProvider? provider);
public static bool TryParse([NotNullWhen(true)] string? s, out NFloat result);
public static bool TryParse([NotNullWhen(true)] string? s, NumberStyles style, IFormatProvider? provider, out NFloat result);
// Arithmetic Operators
public static NFloat operator +(NFloat left, NFloat right);
public static NFloat operator --(NFloat value);
public static NFloat operator /(NFloat left, NFloat right);
public static NFloat operator ++(NFloat value);
public static NFloat operator %(NFloat left, NFloat right);
public static NFloat operator *(NFloat left, NFloat right);
public static NFloat operator -(NFloat left, NFloat right);
public static NFloat operator +(NFloat value);
public static NFloat operator -(NFloat value);
// Comparison Operators
public static bool operator ==(NFloat left, NFloat right);
public static bool operator >(NFloat left, NFloat right);
public static bool operator >=(NFloat left, NFloat right);
public static bool operator !=(NFloat left, NFloat right);
public static bool operator <(NFloat left, NFloat right);
public static bool operator <=(NFloat left, NFloat right);
// Explicit * to NFloat conversions
public static explicit operator NFloat(decimal value);
public static explicit operator NFloat(double value);
public static explicit operator NFloat(nint value); // Explicit is inconsistent with int and long
public static explicit operator NFloat(nuint value); // Explicit is inconsistent with uint and ulong, Xamarin is missing this one
// Explicit NFloat to * conversions
public static explicit operator byte(NFloat value);
public static explicit operator char(NFloat value);
public static explicit operator decimal(NFloat value);
public static explicit operator short(NFloat value);
public static explicit operator int(NFloat value);
public static explicit operator long(NFloat value);
public static explicit operator nint(NFloat value);
public static explicit operator sbyte(NFloat value);
public static explicit operator float(NFloat value);
public static explicit operator ushort(NFloat value);
public static explicit operator uint(NFloat value);
public static explicit operator ulong(NFloat value);
public static explicit operator nuint(NFloat value); // Xamarin is missing this one
// Implicit * to NFloat conversions
public static implicit operator NFloat(byte value);
public static implicit operator NFloat(char value);
public static implicit operator NFloat(short value);
public static implicit operator NFloat(int value);
public static implicit operator NFloat(long value);
public static implicit operator NFloat(sbyte value);
public static implicit operator NFloat(float value);
public static implicit operator NFloat(ushort value);
public static implicit operator NFloat(uint value);
public static implicit operator NFloat(ulong value);
// Implicit NFloat to * conversions
public static implicit operator double(NFloat value);
// From IComparable and IComparable<NFloat>
public bool CompareTo(NFloat other);
public bool CompareTo(object other);
// From object and IEquatable<NFloat>, already exposed
public bool Equals(NFloat other);
public bool Equals(object other);
// From object and IFormattable
public string ToString();
public string ToString(IFormatProvider? provider);
public string ToString(string? format);
public string ToString(string? format, IFormatProvider? provider);
}
Rationale
In https://github.com/dotnet/runtime/issues/13788 we exposed some new interchange types
CLong
,CULong
, andNFloat
to help ensure interop with underlying runtimes could be done in a stable and cross platform manner.We initiallizy decided that these shoulld not expose more complex support such as operators, parsing, or formatting. However, this decision means that
Xamarin
must continue maintaining their own version ofObjCRuntime.nfloat
.As per https://github.com/xamarin/xamarin-macios/issues/13087#issuecomment-1012968826, exposing this additional functionality on
NFloat
would allow them to remove their custom type and normalize on the "built-in" functionality instead.Proposal
Additionally, it may be worth making this consistent with
Half
/Single
/Double
with regards to the generic math work that is being done. This would additionally implementIBinaryFloatingPoint<float>
andIMinMaxValue<float>
.