Ensure the generic math interfaces are implemented implicitly where possible #66748

tannergooding · 2022-03-17T01:47:51Z

Notably this deviates from API review slightly. In API review, it was determined that interfaces should be implicitly implemented. However, most types cannot implement the operators implicitly. This comes down to causing issues with System.Linq.Expressions for a couple types and it being a technically problematic scenario from the perspective of the C# Language Spec (but not the current Roslyn implementation). Likewise, IntPtr and UIntPtr have a pending design discussion with relevant folks from the C# Language Design Team to determine what the penultimate behavior is going to be.

The meeting around IntPtr/UIntPtr is going to happen before the next API review and the deviation here is going to be raised in the next API review. It shouldn't be considered a blocker, however.

…ossible

dotnet-issue-labeler · 2022-03-17T01:47:56Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

dotnet-issue-labeler · 2022-03-17T01:47:57Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

src/libraries/System.Data.Odbc/src/System/Data/Odbc/OdbcConnection.cs

tannergooding · 2022-03-17T01:52:12Z

src/libraries/System.Private.CoreLib/src/System/Char.cs

+        /// <inheritdoc cref="IAdditiveIdentity{TSelf, TResult}.AdditiveIdentity" />
        static char IAdditiveIdentity<char, char>.AdditiveIdentity => (char)0;


API review will need to discuss if exposing AdditiveIdentity, MultiplicativeIdentity, One, and Zero make sense for char. They are not exposed until that discussion happens.

All of the operators exist in regular C#, but unlike the other types the members being exposed publicly may be potentially confusing or "incorrect" terminology for char.

tannergooding · 2022-03-17T01:56:19Z

src/libraries/System.Private.CoreLib/src/System/Char.cs

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        static char INumber<char>.Create<TOther>(TOther value)
+        public static char Create<TOther>(TOther value)


The Create APIs are still pending being extracted to IConvertible interfaces as per API review (dotnet/designs#205 (comment))

This will occur in a follow up PR. This PR is just making the incremental steps of taking the preview surface and exposing it publicly for simplicity

tannergooding · 2022-03-17T01:57:53Z

src/libraries/System.Private.CoreLib/src/System/Decimal.cs

+        public const decimal MultiplicativeIdentity = 1m;
+
+        /// <inheritdoc cref="ISignedNumber{TSelf}.NegativeOne" />
+        public const decimal NegativeOne = -1m;


Decimal already defines and exposes MinusOne. This will need to be discussed in API review.

My own preference is MinusOne is the "incorrect" name and we shouldn't propagate it to the other types or interfaces.

tannergooding · 2022-03-17T02:00:02Z

src/libraries/System.Private.CoreLib/src/System/Decimal.cs

-        static decimal INumber<decimal>.Sign(decimal value)
-            => Math.Sign(value);
+        /// <inheritdoc cref="INumber{TSelf}.Sign(TSelf)" />
+        static decimal INumber<decimal>.Sign(decimal value) => Math.Sign(value);


decimal already exposes int Sign(decimal). We'll want to discuss in API review whether or not normalizing Sign(T) as being special and always returning int is desirable and how to reconcile the exposed API surface otherwise.

tannergooding · 2022-03-17T02:02:03Z

src/libraries/System.Runtime/ref/System.Runtime.cs

+        static byte System.IMultiplicativeIdentity<System.Byte,System.Byte>.MultiplicativeIdentity { get { throw null; } }
+        static byte System.INumber<System.Byte>.One { get { throw null; } }
+        static byte System.INumber<System.Byte>.Zero { get { throw null; } }
+        public static System.Byte Abs(System.Byte value) { throw null; }


Just noting that GenerateReferenceSource isn't consistent in when it picks System.Byte vs byte. It should likely prefer the latter since that's the default we use in source and is unambiguous.

Likewise noting that GenerateReferenceSource doesn't like static abstracts in interfaces today. It doesn't include the relevant accessibility modifier (such as public) and it treats operators as their IL name, rather than their C# syntax (e.g. op_Addition rather than operator +).

Thanks for commenting on these. Please accumulate all of the GenerateReferenceSource issues you're encountering and log an issue in dotnet/arcade.
(some existing issues for reference)

tannergooding · 2022-03-17T02:04:53Z

src/libraries/System.Runtime/ref/System.Runtime.cs

@@ -3588,6 +3302,13 @@ public sealed partial class InsufficientMemoryException : System.OutOfMemoryExce
        public static System.IntPtr MaxValue { get { throw null; } }
        public static System.IntPtr MinValue { get { throw null; } }
        public static int Size { get { throw null; } }
+        static nint System.IAdditiveIdentity<nint,nint>.AdditiveIdentity { get { throw null; } }


Noting that GenerateReferenceSource is not nint vs IntPtr aware and will not pick the appropriate of the type vs the keyword based on what's in source.

It happens to error in most of the right places if they don't line up due to the difference in attributes, however. It would just be nice if the task generated the right source from the start.

ghost · 2022-03-17T02:05:27Z

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

Issue Details

Notably this deviates from API review slightly. In API review, it was determined that interfaces should be implicitly implemented. However, most types cannot implement the operators implicitly. This comes down to causing issues with System.Linq.Expressions for a couple types and it being a technically problematic scenario from the perspective of the C# Language Spec (but not the current Roslyn implementation). Likewise, IntPtr and UIntPtr have a pending design discussion with relevant folks from the C# Language Design Team to determine what the penultimate behavior is going to be.

The meeting around IntPtr/UIntPtr is going to happen before the next API review and the deviation here is going to be raised in the next API review. It shouldn't be considered a blocker, however.

Author:	tannergooding
Assignees:	tannergooding
Labels:	`area-System.Runtime`, `new-api-needs-documentation`
Milestone:	-

tannergooding · 2022-03-17T02:05:27Z

CC. @jeffhandley

stephentoub · 2022-03-17T02:11:49Z

Likewise, IntPtr and UIntPtr have a pending design discussion with relevant folks from the C# Language Design Team to determine what the penultimate behavior is going to be.

penultimate? What comes after that design decision?

src/libraries/System.Private.CoreLib/src/System/Byte.cs

tannergooding · 2022-03-17T02:18:49Z

penultimate? What comes after that design decision?

The upcoming meeting will make the decision on whether IntPtr/UIntPtr has the interface members implicitly or explicitly implemented or if some other modification will exist here. It will decide whether LeadingZeroCount (and other APIs) are legal to invoke off nint and/or IntPtr or neither (only being valid in a generic context on a properly constrained T).

stephentoub · 2022-03-17T02:20:10Z

Right, I'm confused by the penultimate (i.e. next to last) part. There's another IntPtr/UIntPtr decision after that?

tannergooding · 2022-03-17T02:24:15Z

Right, I'm confused by the penultimate (i.e. next to last) part. There's another IntPtr/UIntPtr decision after that?

The decision reached by the C# team and the relevant API review members present will need to be reiterated as part of the API review that occurs after (currently scheduled the next Tuesday after). It likely won't influence IntPtr/UIntPtr any further than that, but may influence our decisions around other APIs and how they're exposed.

src/libraries/System.Private.CoreLib/src/System/DateTimeOffset.cs

stephentoub · 2022-03-17T02:25:16Z

The decision reached by the C# team and the relevant API review members present will need to be reiterated as part of the API review that occurs after (currently scheduled the next Tuesday after). It likely won't influence IntPtr/UIntPtr any further than that, but may influence our decisions around other APIs and how they're exposed.

Ok, got it, good. I was worried there was some big lurking thing beyond I wasn't aware of :)

stephentoub

I skimmed the changes. Other than fixing the ODBC build and fixing the comments on the consts, LGTM.

jeffhandley

I reviewed all of the changes at ~65% thoroughness. It seems pretty mechanical, @tannergooding. Other than the places where you left comments, the APIs that are still blocked by language features, and the <inheritdoc> feedback, is there anything else you'd like particular attention on?

jeffhandley · 2022-03-17T03:31:47Z

src/libraries/System.Private.CoreLib/src/System/Byte.cs

-        // static checked byte IAdditionOperators<byte, byte, byte>.operator +(byte left, byte right)
-        //     => checked((byte)(left + right));
+        // /// <inheritdoc cref="IAdditionOperators{TSelf, TOther, TResult}.op_CheckedAddition(TSelf, TOther)" />
+        // static byte IAdditionOperators<byte, byte, byte>.operator checked +(byte left, byte right) => checked((byte)(left + right));


Note to other reviewers:
The checked operators are blocked by [Proposal]: Checked Operators · Issue #4665 · dotnet/csharplang

jeffhandley · 2022-03-17T04:25:44Z

src/libraries/System.Private.CoreLib/src/System/Double.cs

+        // /// <inheritdoc cref="IFloatingPoint{TSelf}.Exp10(TSelf)" />
+        // public static double Exp10(double x) => Math.Exp10(x);


Please remind me while all of these are still commented out? What are these APIs blocked on?

These are APIs that are approved but not implemented on System.Math or System.MathF

Ideally we get them also implemented and exposed for .NET 7, but the "worst case" is they become DIMs in .NET vNext

jeffhandley · 2022-03-17T05:10:05Z

src/libraries/System.Private.CoreLib/src/System/SByte.cs

-        // static sbyte IShiftOperators<sbyte, sbyte>.operator >>>(sbyte value, int shiftAmount)
-        //     => (sbyte)((byte)value >> shiftAmount);
+        // /// <inheritdoc cref="IShiftOperators{TSelf, TResult}.op_UnsignedRightShift(TSelf, int)" />
+        // static sbyte IShiftOperators<sbyte, sbyte>.operator >>>(sbyte value, int shiftAmount) => (sbyte)((byte)value >> shiftAmount);


Note for other reviewers:
This is blocked by [Proposal]: Unsigned right shift operator · Issue #4682 · dotnet/csharplang

…PIs" This reverts commit 6fd7ec7.

dakersnar · 2022-03-21T19:50:29Z

Thanks for looping me in on this. I skimmed the changes, as well as the reviewer comments. I agree with @jeffhandley that this seems mostly mechanical. LGTM at a high level.

…ossible (dotnet#66748) * Updating generic math interfaces to be implicitly implemented where possible * Resolve a build error in OdbcConnection caused by new Parse APIs * Revert "Resolve a build error in OdbcConnection caused by new Parse APIs" This reverts commit 6fd7ec7. * Suppress CA1846 for System.Data.Odbc * Updating the xml doc comments for exposed constants

tannergooding added 2 commits March 16, 2022 18:34

Updating generic math interfaces to be implicitly implemented where p…

1e405a3

…ossible

Resolve a build error in OdbcConnection caused by new Parse APIs

6fd7ec7

ghost assigned tannergooding Mar 17, 2022

dotnet-issue-labeler bot added the new-api-needs-documentation label Mar 17, 2022

tannergooding commented Mar 17, 2022

View reviewed changes

src/libraries/System.Data.Odbc/src/System/Data/Odbc/OdbcConnection.cs Outdated Show resolved Hide resolved

tannergooding commented Mar 17, 2022

View reviewed changes

tannergooding added the area-System.Runtime label Mar 17, 2022

stephentoub reviewed Mar 17, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Byte.cs Show resolved Hide resolved

stephentoub reviewed Mar 17, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Byte.cs Show resolved Hide resolved

stephentoub reviewed Mar 17, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/DateTimeOffset.cs Show resolved Hide resolved

stephentoub approved these changes Mar 17, 2022

View reviewed changes

jeffhandley requested a review from dakersnar March 17, 2022 03:11

jeffhandley approved these changes Mar 17, 2022

View reviewed changes

tannergooding added 3 commits March 17, 2022 10:04

Revert "Resolve a build error in OdbcConnection caused by new Parse A…

0fef412

…PIs" This reverts commit 6fd7ec7.

Suppress CA1846 for System.Data.Odbc

1b6c67c

Updating the xml doc comments for exposed constants

b0cc452

This was referenced Mar 17, 2022

GenAPI should consistently choose to use the language keywords where possible dotnet/arcade#8638

Closed

GenAPI should understand the static abstracts in interfaces feature dotnet/arcade#8639

Closed

runfoapp bot mentioned this pull request Mar 17, 2022

ThreadPoolTests.CooperativeBlockingCanCreateThreadsFaster timed out #64964

Closed

tannergooding merged commit 7f2cdc5 into dotnet:main Mar 17, 2022

runfoapp bot mentioned this pull request Mar 18, 2022

Threadpool test CooperativeBlockingCanCreateThreadsFaster failing on Mac #66852

Closed

halter73 mentioned this pull request Apr 8, 2022

Make Results<TResult1, TResultN> union types implement IEndpointMetadataProvider dotnet/aspnetcore#41109

Merged

ghost locked as resolved and limited conversation to collaborators Apr 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure the generic math interfaces are implemented implicitly where possible #66748

Ensure the generic math interfaces are implemented implicitly where possible #66748

tannergooding commented Mar 17, 2022 •

edited

Loading

dotnet-issue-labeler bot commented Mar 17, 2022

dotnet-issue-labeler bot commented Mar 17, 2022

tannergooding Mar 17, 2022

tannergooding Mar 17, 2022

tannergooding Mar 17, 2022

tannergooding Mar 17, 2022

tannergooding Mar 17, 2022

tannergooding Mar 17, 2022

jeffhandley Mar 17, 2022

tannergooding Mar 17, 2022

ghost commented Mar 17, 2022

tannergooding commented Mar 17, 2022

stephentoub commented Mar 17, 2022

tannergooding commented Mar 17, 2022

stephentoub commented Mar 17, 2022

tannergooding commented Mar 17, 2022

stephentoub commented Mar 17, 2022 •

edited

Loading

stephentoub left a comment

jeffhandley left a comment

jeffhandley Mar 17, 2022

jeffhandley Mar 17, 2022

tannergooding Mar 17, 2022

jeffhandley Mar 17, 2022

dakersnar commented Mar 21, 2022

		/// <inheritdoc cref="IAdditiveIdentity{TSelf, TResult}.AdditiveIdentity" />
		static char IAdditiveIdentity<char, char>.AdditiveIdentity => (char)0;

		// /// <inheritdoc cref="IFloatingPoint{TSelf}.Exp10(TSelf)" />
		// public static double Exp10(double x) => Math.Exp10(x);

Ensure the generic math interfaces are implemented implicitly where possible #66748

Ensure the generic math interfaces are implemented implicitly where possible #66748

Conversation

tannergooding commented Mar 17, 2022 • edited Loading

dotnet-issue-labeler bot commented Mar 17, 2022

dotnet-issue-labeler bot commented Mar 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Mar 17, 2022

tannergooding commented Mar 17, 2022

stephentoub commented Mar 17, 2022

tannergooding commented Mar 17, 2022

stephentoub commented Mar 17, 2022

tannergooding commented Mar 17, 2022

stephentoub commented Mar 17, 2022 • edited Loading

stephentoub left a comment

Choose a reason for hiding this comment

jeffhandley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dakersnar commented Mar 21, 2022

tannergooding commented Mar 17, 2022 •

edited

Loading

stephentoub commented Mar 17, 2022 •

edited

Loading