Add missing generic constraints #101504

ericstj · 2024-04-24T17:24:07Z

In dotnet/sdk#40230 I added generic constraint checking to APICompat.

This fixes the places in runtime that were missing those. cc @jkotas @terrajobst

Note the suppressions in comparison to last release are commented out until we have an APICompat that validates the new rule in runtime.

…ed APICompat

dotnet-issue-labeler · 2024-04-24T17:24:12Z

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, 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-policy-service · 2024-04-24T17:25:48Z

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

tannergooding · 2024-04-24T17:35:01Z

...aries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs

@@ -402,7 +402,7 @@ public abstract class Instrument<T> : Instrument where T : struct
        public ReadOnlySpan<System.Collections.Generic.KeyValuePair<string, object?>> Tags { get { throw null; } }
        public T Value { get { throw null; } }
    }
-    public delegate void MeasurementCallback<T>(Instrument instrument, T measurement, ReadOnlySpan<System.Collections.Generic.KeyValuePair<string, object?>> tags, object? state);
+    public delegate void MeasurementCallback<T>(Instrument instrument, T measurement, ReadOnlySpan<System.Collections.Generic.KeyValuePair<string, object?>> tags, object? state) where T : struct;


Are all of these internal/non-public types?

Adding a constraint is a binary breaking, so just wanting to double check. -- notably, changing struct to unmanaged is binary compatible, but may not be source compatible

It looks to me like they're all impacting the ref assembly, so users calling these methods from unconstrained or less constrained APIs will break on roll forward.

These are all public types. I've marked this change breaking. The constraints were already present in the runtime so I believe folks would have already observed runtime errors if they were violating the constraint.

System.TypeLoadException: GenericArguments[0], 'System.String', on 'System.Diagnostics.Metrics.MeasurementCallback`1[T]' violates the constraint of type parameter 'T'. at Program.<Main>$(String[] args) at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor) at System.Reflection.MethodBaseInvoker.InvokeDirectByRefWithFewArgs(Object obj, Span`1 copyOfArgs, BindingFlags invokeAttr)

If we don't want to add the source breaking for any of these we should remove the runtime constraint.

The constraints were already present in the runtime so I believe folks would have already observed runtime errors if they were violating the constraint.

👍. I was definitely more concerned about binary breaks, I think source breaks are fine particularly given that a TypeLoadException was already occurring.

jkotas · 2024-04-24T18:07:58Z

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

@@ -2492,7 +2492,7 @@ public abstract partial class Enum : System.ValueType, System.IComparable, Syste
        public string ToString([System.Diagnostics.CodeAnalysis.StringSyntaxAttribute("EnumFormat")] string? format) { throw null; }
        [System.ObsoleteAttribute("The provider argument is not used. Use ToString(String) instead.")]
        public string ToString([System.Diagnostics.CodeAnalysis.StringSyntaxAttribute("EnumFormat")] string? format, System.IFormatProvider? provider) { throw null; }
-        public static bool TryFormat<TEnum>(TEnum value, System.Span<char> destination, out int charsWritten, [System.Diagnostics.CodeAnalysis.StringSyntaxAttribute("EnumFormat")] System.ReadOnlySpan<char> format = default(System.ReadOnlySpan<char>)) where TEnum : struct { throw null; }
+        public static bool TryFormat<TEnum>(TEnum value, System.Span<char> destination, out int charsWritten, [System.Diagnostics.CodeAnalysis.StringSyntaxAttribute("EnumFormat")] System.ReadOnlySpan<char> format = default(System.ReadOnlySpan<char>)) where TEnum : struct, System.Enum { throw null; }


I think this should be removed in the implementation instead. Notice that TryParse APIs on this type do not have the Enum constraint.

@stephentoub what do you think? You added the API with the constraint in 62f3eb2

As with above this already fails at runtime already with:

System.Security.VerificationException: Method System.Enum.TryFormat: type argument 'System.DateTime' violates the constraint of type parameter 'TEnum'.

The unconstrained generic methods (like Parse and TryParse) fail with:

System.ArgumentException: Type provided must be an Enum. (Parameter 'TEnum')

We approved it with the constraint but I believe this was because it was proposed like that and it made sense to us. Not having the constraint seems fine.

Ok, I'll remove the constraints from both ref and source and ensure that this API checks the T and throws an argument exception then (unless @stephentoub mentions otherwise).

ericstj · 2024-04-24T18:29:47Z

src/libraries/System.Private.CoreLib/ref/System.Private.CoreLib.ManualShimTypeForwards.cs

@@ -65,7 +65,7 @@ public sealed partial class ByteEqualityComparer : System.Collections.Generic.Eq
        public override int GetHashCode() { throw null; }
        public override int GetHashCode(byte b) { throw null; }
    }
-    public sealed partial class EnumEqualityComparer<T> : System.Collections.Generic.EqualityComparer<T>, System.Runtime.Serialization.ISerializable where T : struct
+    public sealed partial class EnumEqualityComparer<T> : System.Collections.Generic.EqualityComparer<T>, System.Runtime.Serialization.ISerializable where T : struct, System.Enum


This is not exposed publicly in reference assembly.

ericstj · 2024-04-24T18:34:49Z

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

@@ -362,17 +362,17 @@ public sealed partial class ComObject : System.Runtime.InteropServices.IDynamicI
        System.Runtime.InteropServices.Marshalling.VirtualMethodTableInfo System.Runtime.InteropServices.Marshalling.IUnmanagedVirtualMethodTableProvider.GetVirtualMethodTableInfoForKey(System.Type type) { throw null; }
    }
    [System.Runtime.InteropServices.Marshalling.CustomMarshallerAttribute(typeof(System.Exception), System.Runtime.InteropServices.Marshalling.MarshalMode.UnmanagedToManagedOut, typeof(System.Runtime.InteropServices.Marshalling.ExceptionAsDefaultMarshaller<>))]
-    public static partial class ExceptionAsDefaultMarshaller<T> where T : struct
+    public static partial class ExceptionAsDefaultMarshaller<T> where T : unmanaged


FWIW this would not have produced a runtime exception.

@jkoritzinsky let me know that for these we want the constraints here.

Any type that doesn't follow the : unmanaged constraints will have unexpected runtime behaviors

ericstj · 2024-04-24T18:35:31Z

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

    {
        public static T ConvertToUnmanaged(System.Exception e) { throw null; }
    }
    [System.Runtime.InteropServices.Marshalling.CustomMarshallerAttribute(typeof(System.Exception), System.Runtime.InteropServices.Marshalling.MarshalMode.UnmanagedToManagedOut, typeof(System.Runtime.InteropServices.Marshalling.ExceptionAsHResultMarshaller<>))]
-    public static partial class ExceptionAsHResultMarshaller<T> where T : struct
+    public static partial class ExceptionAsHResultMarshaller<T> where T : unmanaged, System.Numerics.INumber<T>


Currently fails at runtime with

System.TypeLoadException: GenericArguments[0], 'MyStruct', on 'System.Runtime.InteropServices.Marshalling.ExceptionAsHResultMarshaller`1[T]' violates the constraint of type parameter 'T'.

ericstj · 2024-04-24T18:36:12Z

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

    {
        public static T ConvertToUnmanaged(System.Exception e) { throw null; }
    }
    [System.Runtime.InteropServices.Marshalling.CustomMarshallerAttribute(typeof(System.Exception), System.Runtime.InteropServices.Marshalling.MarshalMode.UnmanagedToManagedOut, typeof(System.Runtime.InteropServices.Marshalling.ExceptionAsNaNMarshaller<>))]
-    public static partial class ExceptionAsNaNMarshaller<T> where T : struct
+    public static partial class ExceptionAsNaNMarshaller<T> where T : unmanaged, System.Numerics.IFloatingPointIeee754<T>


Currently fails at runtime with:

System.TypeLoadException: GenericArguments[0], 'MyStruct', on 'System.Runtime.InteropServices.Marshalling.ExceptionAsNaNMarshaller`1[T]'

ViktorHofer · 2024-04-24T20:01:08Z

You might want to add the APICompat update from #101378 into this PR.

ericstj · 2024-04-24T21:58:29Z

You might want to add the APICompat update from #101378 into this PR.

Oh interesting. I wasn't aware that had already flowed. Maybe I'll just collapse this into the darc PR to reduce the churn, once I get consensus on the plan.

ericstj · 2024-04-25T01:14:34Z

Closing this in favor of fixing the ingestion PR where these errors are already popping. That lets me also checking the suppressions which I had to comment out here.

ericstj added 2 commits April 16, 2024 20:12

Fix issues with missing generic constraints

b2220ea

Comment out suppressions for added constraints until we have an updat…

a184cb9

…ed APICompat

ericstj requested a review from ViktorHofer April 24, 2024 17:24

dotnet-issue-labeler bot added area-Infrastructure-libraries new-api-needs-documentation labels Apr 24, 2024

dotnet-policy-service bot assigned ericstj Apr 24, 2024

ericstj mentioned this pull request Apr 24, 2024

Stage 2 Builds failing during runtime build dotnet/source-build#4355

Closed

ericstj added breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. and removed new-api-needs-documentation labels Apr 24, 2024

dotnet-policy-service bot added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Apr 24, 2024

tannergooding reviewed Apr 24, 2024

View reviewed changes

jkotas reviewed Apr 24, 2024

View reviewed changes

ericstj commented Apr 24, 2024

View reviewed changes

ViktorHofer mentioned this pull request Apr 24, 2024

[main] Update dependencies from dotnet/cecil, dotnet/hotreload-utils, dotnet/icu, dotnet/runtime, dotnet/runtime-assets, dotnet/sdk, dotnet/xharness #101378

Merged

ericstj closed this Apr 25, 2024

This was referenced Apr 25, 2024

Long Running Test: System.Text.Json.Tests.Utf8JsonWriterTests.WriteNumbers #101522

Closed

Dead lettering tests #101524

Closed

[WASI][AOT] LLVM ERROR: out of memory #101533

Closed

github-actions bot locked and limited conversation to collaborators May 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing generic constraints #101504

Add missing generic constraints #101504

ericstj commented Apr 24, 2024

dotnet-issue-labeler bot commented Apr 24, 2024

dotnet-policy-service bot commented Apr 24, 2024

tannergooding Apr 24, 2024

tannergooding Apr 24, 2024

ericstj Apr 24, 2024

tannergooding Apr 24, 2024

jkotas Apr 24, 2024 •

edited

Loading

ericstj Apr 24, 2024

ericstj Apr 24, 2024 •

edited

Loading

terrajobst Apr 24, 2024 •

edited

Loading

ericstj Apr 24, 2024

ericstj Apr 24, 2024

ericstj Apr 24, 2024

ericstj Apr 24, 2024

ericstj Apr 24, 2024

ericstj Apr 24, 2024

ViktorHofer commented Apr 24, 2024

ericstj commented Apr 24, 2024

ericstj commented Apr 25, 2024

Add missing generic constraints #101504

Add missing generic constraints #101504

Conversation

ericstj commented Apr 24, 2024

dotnet-issue-labeler bot commented Apr 24, 2024

dotnet-policy-service bot commented Apr 24, 2024

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

jkotas Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericstj Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

terrajobst Apr 24, 2024 • 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 Apr 24, 2024

ericstj commented Apr 24, 2024

ericstj commented Apr 25, 2024

jkotas Apr 24, 2024 •

edited

Loading

ericstj Apr 24, 2024 •

edited

Loading

terrajobst Apr 24, 2024 •

edited

Loading