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.

Sign up for GitHub

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

Jump to bottom

Add missing generic constraints #101504

Closed
wants to merge 2 commits into from
Closed

Add missing generic constraints #101504

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b2220ea
Fix issues with missing generic constraints
ericstj Apr 17, 2024
a184cb9
Comment out suppressions for added constraints until we have an updat…
ericstj Apr 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...es/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

public class Meter : IDisposable
{
public Counter<T> CreateCounter<T>(string name, string? unit = null, string? description = null) where T : struct { throw null; }
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Private.CoreLib/ref/System.Private.CoreLib.ManualShimTypeForwards.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public ByteEqualityComparer() { }
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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not exposed publicly in reference assembly.

{
public EnumEqualityComparer() { }
public override bool Equals([System.Diagnostics.CodeAnalysis.NotNullWhenAttribute(true)] object? obj) { throw null; }
Expand Down
6 changes: 3 additions & 3 deletions src/libraries/System.Runtime.InteropServices/ref/System.Runtime.InteropServices.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,17 +362,17 @@ public void FinalRelease() { }
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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW this would not have produced a runtime exception.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

{
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>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'.

{
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>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently fails at runtime with:

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

{
public static T ConvertToUnmanaged(System.Exception e) { throw null; }
}
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Runtime/ref/System.Runtime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2492,7 +2492,7 @@ protected Enum() { }
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; }
Copy link
Member

@jkotas jkotas Apr 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@ericstj ericstj Apr 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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')

Copy link
Member

@terrajobst terrajobst Apr 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

public static bool TryParse(System.Type enumType, System.ReadOnlySpan<char> value, bool ignoreCase, [System.Diagnostics.CodeAnalysis.NotNullWhenAttribute(true)] out object? result) { throw null; }
public static bool TryParse(System.Type enumType, System.ReadOnlySpan<char> value, [System.Diagnostics.CodeAnalysis.NotNullWhenAttribute(true)] out object? result) { throw null; }
public static bool TryParse(System.Type enumType, string? value, bool ignoreCase, [System.Diagnostics.CodeAnalysis.NotNullWhenAttribute(true)] out object? result) { throw null; }
Expand Down
79 changes: 79 additions & 0 deletions src/libraries/apicompat/ApiCompatBaseline.NetCoreAppLatestStable.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,4 +469,83 @@
<Left>net8.0/System.dll</Left>
<Right>net9.0/System.dll</Right>
</Suppression>
<!-- Uncomment when picking up the latest APICompat with https://github.com/dotnet/sdk/pull/40230
<Suppression>
<DiagnosticId>CP0021</DiagnosticId>
<Target>M:System.Enum.TryFormat``1(``0,System.Span{System.Char},System.Int32@,System.ReadOnlySpan{System.Char})``0:System.Enum</Target>
<Left>net8.0/mscorlib.dll</Left>
<Right>net9.0/mscorlib.dll</Right>
</Suppression>
<Suppression>
<DiagnosticId>CP0021</DiagnosticId>
<Target>M:System.Enum.TryFormat``1(``0,System.Span{System.Char},System.Int32@,System.ReadOnlySpan{System.Char})``0:System.Enum</Target>
<Left>net8.0/netstandard.dll</Left>
<Right>net9.0/netstandard.dll</Right>
</Suppression>
<Suppression>
<DiagnosticId>CP0021</DiagnosticId>
<Target>T:System.Diagnostics.Metrics.MeasurementCallback`1``0:struct</Target>
<Left>net8.0/System.Diagnostics.DiagnosticSource.dll</Left>
<Right>net9.0/System.Diagnostics.DiagnosticSource.dll</Right>
</Suppression>
<Suppression>
<DiagnosticId>CP0021</DiagnosticId>
<Target>M:System.Enum.TryFormat``1(``0,System.Span{System.Char},System.Int32@,System.ReadOnlySpan{System.Char})``0:System.Enum</Target>
<Left>net8.0/System.Runtime.dll</Left>
<Right>net9.0/System.Runtime.dll</Right>
</Suppression>
<Suppression>
<DiagnosticId>CP0021</DiagnosticId>
<Target>T:System.Runtime.InteropServices.Marshalling.ExceptionAsDefaultMarshaller`1``0:struct</Target>
<Left>net8.0/System.Runtime.InteropServices.dll</Left>
<Right>net9.0/System.Runtime.InteropServices.dll</Right>
</Suppression>
<Suppression>
<DiagnosticId>CP0021</DiagnosticId>
<Target>T:System.Runtime.InteropServices.Marshalling.ExceptionAsDefaultMarshaller`1``0:unmanaged</Target>
<Left>net8.0/System.Runtime.InteropServices.dll</Left>
<Right>net9.0/System.Runtime.InteropServices.dll</Right>
</Suppression>
<Suppression>
<DiagnosticId>CP0021</DiagnosticId>
<Target>T:System.Runtime.InteropServices.Marshalling.ExceptionAsHResultMarshaller`1``0:struct</Target>
<Left>net8.0/System.Runtime.InteropServices.dll</Left>
<Right>net9.0/System.Runtime.InteropServices.dll</Right>
</Suppression>
<Suppression>
<DiagnosticId>CP0021</DiagnosticId>
<Target>T:System.Runtime.InteropServices.Marshalling.ExceptionAsHResultMarshaller`1``0:System.Numerics.INumber&lt;T&gt;</Target>
<Left>net8.0/System.Runtime.InteropServices.dll</Left>
<Right>net9.0/System.Runtime.InteropServices.dll</Right>
</Suppression>
<Suppression>
<DiagnosticId>CP0021</DiagnosticId>
<Target>T:System.Runtime.InteropServices.Marshalling.ExceptionAsHResultMarshaller`1``0:unmanaged</Target>
<Left>net8.0/System.Runtime.InteropServices.dll</Left>
<Right>net9.0/System.Runtime.InteropServices.dll</Right>
</Suppression>
<Suppression>
<DiagnosticId>CP0021</DiagnosticId>
<Target>T:System.Runtime.InteropServices.Marshalling.ExceptionAsNaNMarshaller`1``0:struct</Target>
<Left>net8.0/System.Runtime.InteropServices.dll</Left>
<Right>net9.0/System.Runtime.InteropServices.dll</Right>
</Suppression>
<Suppression>
<DiagnosticId>CP0021</DiagnosticId>
<Target>T:System.Runtime.InteropServices.Marshalling.ExceptionAsNaNMarshaller`1``0:System.Numerics.IFloatingPointIeee754&lt;T&gt;</Target>
<Left>net8.0/System.Runtime.InteropServices.dll</Left>
<Right>net9.0/System.Runtime.InteropServices.dll</Right>
</Suppression>
<Suppression>
<DiagnosticId>CP0021</DiagnosticId>
<Target>T:System.Runtime.InteropServices.Marshalling.ExceptionAsNaNMarshaller`1``0:unmanaged</Target>
<Left>net8.0/System.Runtime.InteropServices.dll</Left>
<Right>net9.0/System.Runtime.InteropServices.dll</Right>
</Suppression>
<Suppression>
<DiagnosticId>CP0021</DiagnosticId>
<Target>T:System.Text.Json.Serialization.JsonNumberEnumConverter`1``0:System.Enum</Target>
<Left>net8.0/System.Text.Json.dll</Left>
<Right>net9.0/System.Text.Json.dll</Right>
</Suppression> -->
</Suppressions>
Loading
Loading