Examine JsonSerializer behavior for ref structs & pointers

[layomia]

Related - #34777.

We should detect these types and fail gracefully rather than leaking exceptions from lower level APIs like Type.MakeGenericType.

[loligans]

Any workaround in the meantime? @layomia

Nevermind, I found that I could write my own JsonConverter for the Type.

[layomia]

@loligans unfortunately there's no workaround AFAIK. We'd need reflection support for ref structs to be able to support this. Ref structs can't be boxed or be generic parameters, so they are not friendly with System.Reflection APIs.

Nevermind, I found that I could write my own JsonConverter for the Type.

How were you able to do this? A ref struct can't be the T type used in a JsonConverter<T> instance.

[loligans]

@loligans unfortunately there's no workaround AFAIK. We'd need reflection support for ref structs to be able to support this. Ref structs can't be boxed or be generic parameters, so they are not friendly with System.Reflection APIs.

Nevermind, I found that I could write my own JsonConverter for the Type.

How were you able to do this? A ref struct can't be the T type used in a JsonConverter<T> instance.

@layomia I was able to do this by using ReadOnlyMemory instead

public class MemoryJsonConverter : JsonConverter<ReadOnlyMemory<byte>>
{
    public override ReadOnlyMemory<byte> Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        return reader.GetBytesFromBase64().AsMemory();
    }

    public override void Write(Utf8JsonWriter writer, ReadOnlyMemory<byte> value, JsonSerializerOptions options)
    {
        if (writer is null) { throw new ArgumentNullException(nameof(writer)); }
        writer.WriteBase64StringValue(value.Span);
    }
}

[layomia]

This issue should also consider the scenario from #59936 which necessitated the use of [JsonIgnore]:

using System.Diagnostics;
using System.Text.Json;
using System.Text.Json.Serialization;

var sampleText = new TypeWithRefString();
var json = JsonSerializer.Serialize(sampleText); // ArgumentException Here
var deserialized = JsonSerializer.Deserialize<TypeWithRefString>(json); // And Here

internal class TypeWithRefString
{
    public ref string NameRef => ref Name;
    public string Name = "How many Richard Landers does it take to write a blog entry? Just one, you rock!";
}

[eiriktsarpalis]

Is supporting ref structs and pointers important? Fundamentally they represent non-serializable entities (what does serializing byte* mean?) and adding support for them is not cheap, adding complexity to our infrastructure due to the aforementioned restrictions using them as generic parameters.

I would recommend explicitly not supporting these types, but provide escape hatches via the JsonIgnoreAttribute like #60024 is attempting to do. We might want to fail more gracefully with a meaningful error message (rather than the current Type.MakeGenericType throwing)

[layomia]

We might want to fail more gracefully with a meaningful error message (rather than the current Type.MakeGenericType throwing)

Yes this is the intent for these types, to detect them deterministically and fail gracefully.

[krwq]

I've added a fix for issue which sounds similar to this one in contract customization PR, can we verify it works as expected now? See test I added here: 5be7092#diff-c0f5d77b22f8bd7e41958095b0e9fc829a00de0104bf283035d81e6bb3d7b30eR599

[krwq]

I'm closing this for now please create new issue with repro if you feel like we've missed anything in there

[eiriktsarpalis]

@krwq the tests you cite are specific to contract customization. We should add a test that specifically validates the scenario described in, say, #34779 (comment). I would recommend reopening the issue until that is addressed.

[eiriktsarpalis]

Let's reopen this, since we have failing tests linked to this issue:

runtime/src/libraries/System.Text.Json/tests/Common/ConstructorTests/ConstructorTests.ParameterMatching.cs

Lines 1295 to 1301 in 1a0c9ca

	[Fact]
	[ActiveIssue("https://github.com/dotnet/runtime/issues/34779")]
	public async Task BindingBetweenRefProps()
	{
	string json = @"{""NameRef"":""John""}";
	await Serializer.DeserializeWrapper<TypeWith_RefStringProp_ParamCtor>(json);
	}

[eiriktsarpalis]

The test in question is related to the issue better described in #46088. I think we should close this one since it's less concrete and address any issues related to unsupported types as they come up.