Expose and implement generic math interfaces on core types #54650

tannergooding · 2021-06-23T22:14:10Z

This exposes the library side of the Static Abstracts in Interfaces feature as per dotnet/designs#205.

This requires pinning the Roslyn version as main-vs-deps won't integrate into another branch until sometime next week and then that will need to flow through Arcade (which is currently on release/dev16.10-vs-deps, while main-vs-deps will integrate into some release/dev17-*-vs-deps branch).

This does not currently include anything more than the basic tests. I have more tests locally, but they are not ready to be pushed quite yet.

…atic Abstracts in Interfaces

- Disable constraint checking during EEInit - Disable il linker running on CoreLib - Fixup generic math tests to actually build

dotnet-issue-labeler · 2021-06-23T22:14:15Z

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 · 2021-06-23T22:14:16Z

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.

tannergooding · 2021-06-23T22:15:13Z

CC. @ViktorHofer, @ericstj for the pinning of the Roslyn version.

CC. @jeffhandley, @pgovind, @stephentoub for the library side changes

CC. @MadsTorgersen, @davidwrighton, @terrajobst as an FYI that this part is up for review.

ghost · 2021-06-23T22:15:53Z

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

Issue Details

This exposes the library side of the Static Abstracts in Interfaces feature as per dotnet/designs#205.

This requires pinning the Roslyn version as main-vs-deps won't integrate into another branch until sometime next week and then that will need to flow through Arcade (which is currently on release/dev16.10-vs-deps, while main-vs-deps will integrate into some release/dev17-*-vs-deps branch).

This does not currently include anything more than the basic tests. I have more tests locally, but they are not ready to be pushed quite yet.

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

src/coreclr/vm/typedesc.cpp

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

davidwrighton · 2021-06-23T23:51:01Z

@tannergooding I was looking at the failures in the test results, and it looks like there is a bug with static virtual method implementations existing on a type exposed to COM. I'll take a look and see what I can do about that.

davidwrighton · 2021-06-24T01:21:24Z

classcompat loader failures are fixed by #54658 I'll get that pushed through tomorrow.

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

stephentoub · 2021-06-24T11:11:33Z

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

+
+        // [RequiresPreviewFeatures]
+        // static checked byte IAdditionOperators<byte, byte, byte>.operator +(byte left, byte right)
+        //     => checked((byte)(left + right));


What are the functions commented out? What's the plan for them?

These are dependent on one or more of the proposed language features (such as checked operators) being approved and implemented. None of these were on the table for .NET 6.

How about adding this to the top of the comment block then:

// TODO - Uncomment once checked operator support is added to C# // https://github.com/dotnet/csharplang/issues/4665

That's going to be a lot of additional commentation for likely little benefit.

For .NET 7, this will all be touched anyways to remove the [RequiresPreviewFeatures] annotations and making bits public, at which point these will either be uncommented (if they make it in) or removed (if LDM rejects the features).

Fair enough. Thanks for considering it.

My preference is to delete such code until such time that the language feature is agreed upon and exists and we decide to and are able to add such features to the interfaces. It's otherwise confusing to have such code in but commented out, especially without an explanation.

stephentoub · 2021-06-24T11:16:11Z

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

+        // IAdditionOperators
+        //
+
+        [RequiresPreviewFeatures]


Annotating the interface itself as preview isn't sufficient? We also need to annotate all explicit implementations of that interface? cc: @pgovind

Missed this notification in my email. We don't need to. We can also just suppress the analyzer here.

We don't need to. We can also just suppress the analyzer here.

Does this mean the analyzer currently requires it but you agree it shouldn't?

Does this mean the analyzer currently requires it

Yup. The analyzer does single level lookups right now and tags methods/properties etc when it find the attribute on them (or the class/interface). It works well for implicit implementations. For explicit implementations I need to think about it some more. If we had a 3rd party library author implement IAdditionOperators explicitly, I think having a diagnostic at the implementation is good. That way the library author knows that they are implementing a preview interface. I'm hoping to get dotnet/roslyn-analyzers#5155 in soon though and we can iterate on the design more concretely once we start consuming it in the runtime.

stephentoub · 2021-06-24T11:19:35Z

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

+            }
+            else if (typeof(TOther) == typeof(char))
+            {
+                return checked((byte)(char)(object)value);


Why are all of these checked?

Unchecked appears to be the CreateTruncated instead

We should revisit these names then. In general unchecked is the default for C#, and you have to opt in to checked. It's strange then for the APIs to invert that by having the "Create" name be checked and having to append a suffix on it to effectively opt-in to unchecked.

unchecked is the default for C# yes, unless you are dealing with literals or many other forms of creation.

I believe it will ultimately be a pit of failure for a user to write T x = T.Create(<TooBigLiteral>) and for it to not fail by default. This is why the compiler emits CS0221 (Constant value '###' cannot be converted to a 'T' (use 'unchecked' syntax to override) for such a scenario

It certainly threw me off, that normal Create was checked. Most important is both are available. But agree naming seems a bit off then.

I believe it will ultimately be a pit of failure for a user to write T x = T.Create() and for it to not fail by default.

I similarly think it'll be a pit of failure for a user to write T x = T.Create(variable) and have that use checked arithmetic and throw overflow exceptions and the like. We can't emulate the language's compile-time validation, but we can map to its run-time defaults.

stephentoub · 2021-06-24T11:22:18Z

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

+        //
+
+        [RequiresPreviewFeatures]
+        static byte ISpanParseable<byte>.Parse(ReadOnlySpan<char> s, IFormatProvider? provider)


I thought we'd discussed just combining ISpanParseable and IParseable. Did we decide not to?

We hadn't formalized this in API review and there were still open questions here about F# and VB to my understanding.

When are we going to decide on it then?

Sometime before this ships as stable, so possibly after .NET 6.

ViktorHofer · 2021-06-24T11:49:37Z

CC. @ViktorHofer, @ericstj for the pinning of the Roslyn version.

That change is right 👍 Thanks

stephentoub · 2021-06-24T13:45:04Z

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

+
+        [RequiresPreviewFeatures]
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        static bool INumber<char>.TryCreate<TOther>(TOther value, out char result)


I still question whether this is the right creation direction. This requires a type T to know how to create itself from another type TOther, which means T needs to know about TOther. That prevents, for example, Int64 being able to create itself from BigInteger. If it were the other way around, BigInteger could know how to create an instance of any type it can see, so it could implement the cast to Int64.

Is that direction covered elsewhere?

You result in the same fundamental issue in either direction. For an arbitrary, unknown U, you might not be able to create or convert to a given T.

There are some directions where going from U to T is easier and some where going from T to U will be easier. Right now the interfaces are only exposing "constructor" equivalents (create a T from a U, rather than convert a U to a T) because exposing both raises complications in determining the right way/order to cover both scenarios and avoid infinite recursion issues.

That prevents, for example, Int64 being able to create itself from BigInteger

This is going to be solved by IBinaryInteger.TryWriteBytes and related methods. At some point, you have to be able to fallback to something "generic enough" so you don't need to codify "every possible type".

Having something like IConvertibleTo<TOther> would help, but its also not something that the core types can reasonably implement due to additional startup cost and number of conversions between all 13 primitive types.

Hmmm, TryWriteBytes isn't in this PR, that isn't right 🤔

For an arbitrary, unknown U, you might not be able to create or convert to a given T.

No, but you have a much higher chance of doing so when the thing with the data to be converted is able to reference the thing to be created.

No, but you have a much higher chance of doing so when the thing with the data to be converted is able to reference the thing to be created.

Can you elaborate, preferably with examples, of why ConvertTo has a higher chance of success than CreateFrom? My expectation is that you ultimately need both and that they should be roughly equivalent given internal implementation details.

The biggest problem is the pit of failure for having some Create method, without DIMs, that will correctly try both (to give the highest chance of success) without risking any kind of infinite recursion (or a potentially unbounded number of interfaces to check), etc.

This all comes down to: "We need both in practice". There are flaws and limitations to just one or the other and so we ultimately need both CreateFrom and ConvertTo style APIs to ensure the relevant scenarios are covered.

@stephentoub and I did discuss this a bit last week and it will likely come down to some pattern where we tell users that you shouldn't have CreateFrom call ConvertTo or vice-versa, to avoid the issue with potential infinite recursion (imagine CreateFrom<TOther> falls back to calling value.ConvertTo<TSelf> for an unrecognized TOther, and value.ConvertTo<TOther> falls back to TOther.CreateFrom<TSelf>).

For implementors, there should likely be some recommended pattern to ensure that both functions cover all the relevant items. My initial thought is that we have:

static TSelf Create<TOther>(TOther value)

static bool TryCreate<TOther>(TOther value, out TSelf result)

static bool TryConvertFrom<TOther>(TOther value, out TSelf result)

static bool TryConvertTo<TOther>(TSelf value, out TOther result)

Where the standard pattern is something like:

public static TSelf Create<TOther>(TOther value) { if (!TryCreate(value, out TSelf result)) { throw new NotSupportedException(); } return result; } public static bool TryCreate<TOther>(TOther value, out TSelf result) { return TryConvertFrom<TOther>(value, out result) || TOther.TryConvertTo<TSelf>(value, out result); } public static bool TryConvertFrom<TOther>(TOther value, out TSelf result) { if (typeof(TOther) == typeof(...)) { // ..., any TOther where ConvertFrom is most efficient } else { return false; } } public static bool TryConvertTo<TOther>(TSelf value, out TOther result) { if (typeof(TOther) == typeof(...)) { // ..., any TOther where ConvertTo is most efficient } else { return false; } }

This would extend to each Create* pattern and so would include "exact", "saturating", "truncating", etc

Do we need both Create and TryCreate variants, or could we just have the TryCreate? Could Create be a DIM if/when we decide we need it? For that matter could TryCreate be a DIM and you only implemenet TryConvertFrom/To?

Also, you talked about the fallback of being able to convert to/from bytes. Where does that fit into this pattern?

Do we need both Create and TryCreate variants, or could we just have the TryCreate

We could probably have just TryCreate but that might make it less idiomatic to use in expressions. I don't have a strong preference here.

For that matter could TryCreate be a DIM and you only implemenet TryConvertFrom/To?

That would be ideal, but is out of scope for .NET 6. It would be addressed before we ship for real based on whether or not DIMs are in the official release

Also, you talked about the fallback of being able to convert to/from bytes. Where does that fit into this pattern?

You would handle it in one or both of the TryConvert* APIs as one of the typeof() == typeof() checks. Basically:

Handle any concrete type

Handle any abstract type via things like IBinaryInteger where you can get the raw 2s complement representation as bytes

return false

This, for value type TOther, will compile down very efficiently. For reference types there will be casts and checks but still not bad overall.

We could probably have just TryCreate but that might make it less idiomatic to use in expressions

Fair enough.

What's your plan for factoring this all in?

I'd like to get this PR merged so we have the base bits in and ready. Then to follow up with a PR that adds in TryConvert*, ideally before Preview 7, but possibly for RC1.

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

stephentoub · 2021-06-24T13:48:36Z

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

+          IMinMaxValue<DateTimeOffset>,
+          ISpanParseable<DateTimeOffset>,
+          ISubtractionOperators<DateTimeOffset, TimeSpan, DateTimeOffset>,
+          ISubtractionOperators<DateTimeOffset, DateTimeOffset, TimeSpan>


In practice, with such a specific combination of generic type arguments, how will this be used in practice? i.e. what's an example of a method that would successfully operate over this interface and not be tied directly to DateTimeOffset?

APIs like a generic Sum or Clamp can still take advantage. Other APIs may indeed not be as usable for cases like DateTimeOffset or may only be usable if you can customize more than just one input type.

I'm particularly fond of Clamp in this scenario.

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

stephentoub · 2021-06-24T13:57:04Z

Are we planning to add any APIs that use these interfaces?

tannergooding · 2021-06-29T16:47:36Z

Disable the tests on Mono AOT.

@lambdageek, I'm not sure disabling the tests is going to be feasible. This impacts 75 test assemblies and since its resulting in a crash an unknown number of tests.

lambdageek · 2021-06-29T17:38:51Z

Disable the tests on Mono AOT.

@lambdageek, I'm not sure disabling the tests is going to be feasible. This impacts 75 test assemblies and since its resulting in a crash an unknown number of tests.

Oh this is impacting other testsuites, not just the new ones in this PR :-(
Sorry I didn't appreciate that.

@vargaz can we avoid the assert in cases where the static virtual interface methods aren't actually called?

tannergooding · 2021-06-30T20:06:47Z

@lambdageek, @vargaz. Just pinging on this as we'd ideally get this in sooner to get the extra validation on any startup or size regressions and general E2E integration.

vargaz · 2021-06-30T20:40:15Z

#54981 will hopefully fix the assert.

tannergooding · 2021-06-30T21:49:59Z

#54981 will hopefully fix the assert.

Thanks! I've temporarily cherry-picked it to get some early validation and confirm that everything will pass once merged.

stephentoub · 2021-07-01T20:54:56Z

eng/Versions.props

@@ -17,6 +17,8 @@
    <DotNetFinalVersionKind Condition="'$(StabilizePackageVersion)' == 'true'">release</DotNetFinalVersionKind>
    <!-- Opt-in/out repo features -->
    <UsingToolMicrosoftNetCompilers>true</UsingToolMicrosoftNetCompilers>
+    <!-- TODO: Upgrade compiler version to enable Static Abstracts in Interfaces; remove this once the employed SDK uses a new enough version. -->
+    <MicrosoftNetCompilersToolsetVersion>4.0.0-2.21323.11</MicrosoftNetCompilersToolsetVersion>


If you're going to push any more changes, might be worth bumping this up to the latest available at the time.

stephentoub · 2021-07-01T21:00:40Z

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

+
+        [RequiresPreviewFeatures]
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        static byte INumber<byte>.CreateSaturating<TOther>(TOther value)


Is "Saturating" the right name here? Don't we use "Clamp" elsewhere for the same concept?

It depends on the API I think. We use Saturating in a few places, such as in the hardware intrinsics and its the "correct" term.

I think clamping is generally for if the user can specify the boundary values.

stephentoub · 2021-07-01T21:11:29Z

src/libraries/System.Runtime/tests/System/Int32GenericMathTests.cs

+            Assert.Equal(expected: 536854528, actual: GenericMath.Sum<int, int>(values));
+        }
+    }
+}


I'd feel a whole lot more comfortable having 15000 lines of product code merged if there were more than 110 lines of test code.

pgovind · 2021-07-01T22:30:43Z

src/libraries/System.Runtime/tests/System/GenericMathTests.cs

+            where TResult : INumber<TResult>
+        {
+            TResult sum = Sum<TSelf, TResult>(values);
+            return TResult.Create(sum) / TResult.Create(values.Count());


Just adding a note that once we start consuming the DetectPreviewFeaturesAnalyzer, we'd need to opt into preview features in this csproj.

pgovind · 2021-07-01T22:32:13Z

src/libraries/System.Runtime/tests/System/Int32GenericMathTests.cs

+
+namespace System.Tests
+{
+    public sealed class Int32GenericMathTests : GenericMathTests<int>


Is there a way to write a text template file to generate tests? This is a huge PR, it's not easy to see how much code coverage we have here :/

In answer to both yours and @stephentoub comment: #54650 (comment)

I've done a lot of testing locally, but a lot of this was "ad-hoc" and "generated" to validate various scenarios. I'm still working on porting these tests to be xUnit based and that's the main reason they aren't in this PR. I had, ideally, wanted to get this in so it makes the preview 7 cutoff and to try and get the tests up early next week.

pgovind

Library changes look good to me. We should consider having a text template file generate the tests though?

vargaz · 2021-07-01T23:07:08Z

There is a failure in the wasm aot tests, which is expected, since there is little support for generic sharing + static virtual methods right now on aot. I'd suggest disabling those tests for wasm+aot.

lambdageek

These tests should work on the mono interpreter, just no (yet) on mono AOT

src/libraries/System.Runtime/tests/System/GenericMathTests.cs

Co-authored-by: Aleksey Kliger (λgeek) <akliger@gmail.com>

jeffhandley

Looks good to me!

tannergooding · 2021-07-02T22:19:08Z

After discussing with @jeffhandley, we're going to merge this now to ensure it can flow early and isn't going into Preview 7 last minute.

The test PR will be going up early to mid next week after I finish porting them over to use xUnit.

EgorBo · 2021-07-05T16:29:35Z

Looks like this PR increased System.Private.CoreLib.dll size from 9.66 to 9.94 Mb (Windows.x64.Release)

tannergooding and others added 6 commits June 23, 2021 14:53

Pin MicrosoftNetCompilersToolsetVersion to a version that supports St…

f0c0d0d

…atic Abstracts in Interfaces

Fixed issues related to enabling generic math in a general sense (#4)

4191dc5

- Disable constraint checking during EEInit - Disable il linker running on CoreLib - Fixup generic math tests to actually build

Adding interfaces to support generic math

8ee8854

Implement generic math interfaces on core types

4d32d3f

Updating the System.Runtime ref assembly to support generic math

50ea9d7

Add a basic xunit test for generic-math

30e1f3c

dotnet-issue-labeler bot added the new-api-needs-documentation label Jun 23, 2021

tannergooding added the area-System.Numerics label Jun 23, 2021

tannergooding commented Jun 23, 2021

View reviewed changes

src/coreclr/vm/typedesc.cpp Show resolved Hide resolved

tannergooding mentioned this pull request Jun 23, 2021

Adding a rough draft of the "minimum viable product" for the .NET Libraries APIs to support generic math dotnet/designs#205

Merged

FaustVX reviewed Jun 23, 2021

View reviewed changes

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

davidwrighton mentioned this pull request Jun 24, 2021

Faster type load for scenarios made more common by generic math #54588

Merged

huoyaoyuan reviewed Jun 24, 2021

View reviewed changes

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

kzrnm mentioned this pull request Jun 24, 2021

[Proposal]: Static abstract members in interfaces dotnet/csharplang#4436

Open

4 tasks

stephentoub reviewed Jun 24, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/DateOnly.cs Outdated Show resolved Hide resolved

stephentoub reviewed Jun 24, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Double.cs Outdated Show resolved Hide resolved

lambdageek mentioned this pull request Jun 29, 2021

[mono][aot] Static virtual interface methods do not work with generic sharing #54910

Closed

Merge remote-tracking branch 'dotnet/main' into generic-math

4bf2892

tannergooding force-pushed the generic-math branch from 5dff01c to aadd7fd Compare June 30, 2021 21:49

tannergooding requested review from lambdageek, thaystg and vargaz as code owners June 30, 2021 21:49

Merge remote-tracking branch 'dotnet/main' into generic-math

5f60387

tannergooding force-pushed the generic-math branch from aadd7fd to 5f60387 Compare July 1, 2021 16:39

stephentoub reviewed Jul 1, 2021

View reviewed changes

stephentoub approved these changes Jul 1, 2021

View reviewed changes

pgovind reviewed Jul 1, 2021

View reviewed changes

pgovind approved these changes Jul 1, 2021

View reviewed changes

Skip generic math tests on Mono WASM due to dotnet#54910

a9d2daf

lambdageek reviewed Jul 2, 2021

View reviewed changes

Apply suggestions from code review

6778b79

Co-authored-by: Aleksey Kliger (λgeek) <akliger@gmail.com>

jeffhandley approved these changes Jul 2, 2021

View reviewed changes

tannergooding merged commit fe9a54d into dotnet:main Jul 2, 2021

karelz mentioned this pull request Jul 28, 2021

[WinHttpHandler] Long running test: ResponseHeadersRead_SynchronizationContextNotUsedByHandler on Win7/Win8 #54034

Closed

ghost locked as resolved and limited conversation to collaborators Aug 4, 2021

Expose and implement generic math interfaces on core types #54650

Expose and implement generic math interfaces on core types #54650

Conversation

tannergooding commented Jun 23, 2021

dotnet-issue-labeler bot commented Jun 23, 2021

dotnet-issue-labeler bot commented Jun 23, 2021

tannergooding commented Jun 23, 2021

ghost commented Jun 23, 2021

davidwrighton commented Jun 23, 2021

davidwrighton commented Jun 24, 2021

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

stephentoub Jul 1, 2021 • edited Loading

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

tannergooding Jun 24, 2021 • edited Loading

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

ViktorHofer commented Jun 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tannergooding Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

stephentoub Jun 30, 2021 • edited Loading

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

stephentoub Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub commented Jun 24, 2021

tannergooding commented Jun 29, 2021

lambdageek commented Jun 29, 2021

tannergooding commented Jun 30, 2021

vargaz commented Jun 30, 2021

tannergooding commented Jun 30, 2021

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

pgovind left a comment • edited Loading

Choose a reason for hiding this comment

vargaz commented Jul 1, 2021

lambdageek left a comment

Choose a reason for hiding this comment

jeffhandley left a comment

Choose a reason for hiding this comment

tannergooding commented Jul 2, 2021

EgorBo commented Jul 5, 2021 • edited Loading

stephentoub Jul 1, 2021 •

edited

Loading

tannergooding Jun 24, 2021 •

edited

Loading

stephentoub Jun 24, 2021 •

edited

Loading

tannergooding Jun 28, 2021 •

edited

Loading

stephentoub Jun 30, 2021 •

edited

Loading

stephentoub Jun 24, 2021 •

edited

Loading

pgovind left a comment •

edited

Loading

EgorBo commented Jul 5, 2021 •

edited

Loading