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.

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

Missing Enum constraint on JsonNumberEnumConverter<TEnum> in reference assembly #99878

Closed
KalleOlaviNiemitalo opened this issue Mar 17, 2024 · 10 comments · Fixed by #99952
Closed
Assignees
Milestone

Comments

@KalleOlaviNiemitalo
Copy link

Description

In the source code, System.Text.Json.Serialization.JsonNumberEnumConverter<TEnum> has a type constraint where TEnum : struct, Enum. In the reference assembly however, the constraint is just where TEnum : struct and omits Enum. Thus, if TEnum is a non-enum value type, the compiler doesn't notice any problem but it fails at run time.

public sealed class JsonNumberEnumConverter<TEnum> : JsonConverterFactory
where TEnum : struct, Enum

public sealed partial class JsonNumberEnumConverter<TEnum> : System.Text.Json.Serialization.JsonConverterFactory where TEnum : struct

Reproduction Steps

NonEnum.csproj:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net8.0</TargetFramework>
    <Nullable>enable</Nullable>
  </PropertyGroup>

</Project>

Program.cs:

_ = new System.Text.Json.Serialization.JsonNumberEnumConverter<int>();

dotnet run

Expected behavior

The compiler should report an error:

Program.cs(1,64): error CS0315: The type 'int' cannot be used as type parameter 'TEnum' in the generic type or method 'JsonNumberEnumConverter<TEnum>'. There is no boxing conversion from 'int' to 'System.Enum'.

The build failed. Fix the build errors and run again.

Actual behavior

The build succeeds without any warnings, but the program fails at run time:

Unhandled exception. System.TypeLoadException: GenericArguments[0], 'System.Int32', on 'System.Text.Json.Serialization.JsonNumberEnumConverter`1[TEnum]' violates the constraint of type parameter 'TEnum'.
at Program.<Main>$(String[] args)

Regression?

Not a regression. Both the src and ref changes came from #88984, so they have never been consistent in any release.

Known Workarounds

No response

Configuration

.NET SDK 8.0.202
.NET 8.0.3
Windows 10 22H2 x64

Other information

This bug might be considered part of #26187.

The System.Enum type constraint is missing from the documentation page https://learn.microsoft.com/en-us/dotnet/api/system.text.json.serialization.jsonnumberenumconverter-1?view=net-8.0 as well.

The System.Text.Json 8.0.3 NuGet package does not carry any reference assemblies. Thus, if you reference that and target .NET Framework, then you get the expected error CS0315 at compile time.

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Mar 17, 2024
@ShreyasJejurkar
Copy link
Contributor

Strange, why APICompact didn't catch this. Looks like bug or missing usecase to check in it.

cc @ViktorHofer

@KalleOlaviNiemitalo
Copy link
Author

If I understand correctly, ApiCompat would have to compare ITypeParameterSymbol.ConstraintTypes. However, git grep -e ConstraintTypes v8.0.200 in https://github.com/dotnet/sdk/ doesn't find anything.

@eiriktsarpalis eiriktsarpalis self-assigned this Mar 19, 2024
@eiriktsarpalis eiriktsarpalis added this to the 8.0.x milestone Mar 19, 2024
@eiriktsarpalis eiriktsarpalis added bug and removed untriaged New issue has not been triaged by the area owner labels Mar 19, 2024
@eiriktsarpalis
Copy link
Member

@KalleOlaviNiemitalo what branch is this? I see the constraint is present in both main:

public partial class JsonStringEnumConverter<TEnum> : System.Text.Json.Serialization.JsonConverterFactory where TEnum : struct, System.Enum

and release/8.0:

public partial class JsonStringEnumConverter<TEnum> : System.Text.Json.Serialization.JsonConverterFactory where TEnum : struct, System.Enum

I also can't reproduce the crash you're seeing:

Screenshot 2024-03-19 at 10 33 47

@eiriktsarpalis eiriktsarpalis added untriaged New issue has not been triaged by the area owner needs-author-action An issue or pull request that requires more info or actions from the author. and removed bug labels Mar 19, 2024
@eiriktsarpalis eiriktsarpalis removed this from the 8.0.x milestone Mar 19, 2024
@dotnet-policy-service dotnet-policy-service bot removed the untriaged New issue has not been triaged by the area owner label Mar 19, 2024
@KalleOlaviNiemitalo
Copy link
Author

@eiriktsarpalis, you're looking at JsonStringEnumConverter<TEnum>, but this issue is for JsonNumberEnumConverter<TEnum>.

@dotnet-policy-service dotnet-policy-service bot added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Mar 19, 2024
@eiriktsarpalis
Copy link
Member

D'oh! Didn't even notice it was for a different type. Thanks.

@eiriktsarpalis eiriktsarpalis added bug and removed needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration labels Mar 19, 2024
@eiriktsarpalis eiriktsarpalis added this to the 8.0.x milestone Mar 19, 2024
@eiriktsarpalis
Copy link
Member

FYI @carlossanlop @ViktorHofer @ericstj this will require a .NET 8 backport that updates the reference source.

@KalleOlaviNiemitalo
Copy link
Author

KalleOlaviNiemitalo commented Mar 19, 2024

Does this truly deserve a backport? It seems unlikely that the bug would be hurting any production applications.

I originally noticed the missing constraint in the documentation and was going to file an issue there, but filed this one here instead.

@ericstj
Copy link
Member

ericstj commented Mar 19, 2024

Not sure we need the backport here. Please file an issue in dotnet/sdk that APICompat didn't catch this.

@KalleOlaviNiemitalo
Copy link
Author

@ShreyasJejurkar, do you intend to file the APICompat issue? I'm not really familiar with the tool.

@KalleOlaviNiemitalo
Copy link
Author

@eiriktsarpalis, should this issue be moved to the 9.0.0 milestone because it won't be serviced in 8.0.x?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants