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

Implement support for unspeakable types. #83631

Merged

Conversation

eiriktsarpalis
Copy link
Member

@ghost
Copy link

ghost commented Mar 18, 2023

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

Issue Details

Fix #82457.

cc @eerhardt @brunolins16 @halter73

Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

area-System.Text.Json

Milestone: 8.0.0

@eiriktsarpalis eiriktsarpalis requested a review from tarekgh March 18, 2023 00:51
Copy link
Member

@krwq krwq left a comment

Choose a reason for hiding this comment

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

I have suggestion for slightly different way to fix this:

internal class AsyncEnumerableFixerWrapper : IJsonTypeInfoResolver
{
    private IJsonTypeInfoResolver _resolver;

    public AsyncEnumerableFixerWrapper(IJsonTypeInfoResolver resolver)
    {
        _resolver = resolver;
    }

    public JsonTypeInfo? GetTypeInfo(Type type, JsonSerializerOptions options)
    {
        JsonTypeInfo? result = _resolver.GetTypeInfo(type, options);

        if (result != null)
        {
            return result;
        }

        foreach (var @interface in type.GetInterfaces())
        {
            if (@interface.IsGenericType && @interface.GetGenericTypeDefinition() == typeof(IAsyncEnumerable<>))
            {
                return _resolver.GetTypeInfo(@interface, options);
            }
        }

        return null;
    }
}

Above will currently throw:

InvalidOperationException: The IJsonTypeInfoResolver returned an incompatible JsonTypeInfo instance of type 'System.Collections.Generic.IAsyncEnumerable`1[System.Int32]', expected type 'Program+<<<Main>$>g__Foo|0_1>d'.
    System.Text.Json.ThrowHelper.ThrowInvalidOperationException_ResolverTypeNotCompatible(Type requestedType, Type actualType)
    System.Text.Json.JsonSerializerOptions.GetTypeInfoNoCaching(Type type)
    System.Collections.Concurrent.ConcurrentDictionary<TKey, TValue>.GetOrAdd(TKey key, Func<TKey, TValue> valueFactory)
    System.Text.Json.JsonSerializerOptions.GetTypeInfoInternal(Type type, bool ensureConfigured, bool resolveIfMutable)
    System.Text.Json.JsonSerializer.GetTypeInfo(JsonSerializerOptions options, Type inputType)
    System.Text.Json.JsonSerializer.SerializeAsync(Stream utf8Json, object value, Type inputType, JsonSerializerOptions options, CancellationToken cancellationToken)
    Microsoft.AspNetCore.Http.HttpResponseJsonExtensions.WriteAsJsonAsyncSlow(Stream body, object value, Type type, JsonSerializerOptions options, CancellationToken cancellationToken)
    Microsoft.AspNetCore.Routing.EndpointMiddleware.<Invoke>g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)
    Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddlewareImpl.Invoke(HttpContext context)

I suggest we loosen the artifical check and provide that as workaround. I think current approach is too broad and may lead to potential data loss.

@eiriktsarpalis
Copy link
Member Author

eiriktsarpalis commented Mar 20, 2023

I have suggestion for slightly different way to fix this

Changing this at the resolver level would break the invariant of JsonTypeInfo.Type matching the input type. So it would immediately give you InvalidCastException once you attempt to resolve the generic JsonTypeInfo or if you try to deserialize. So this needs to be implemented as an ad hoc resolution mechanism driven by the cache that doesn't change the result when try to look up metadata for non weakly typed serialization scenaria.

@krwq
Copy link
Member

krwq commented Mar 20, 2023

@eiriktsarpalis in case of original repro why are we looking at the runtime time in the first place? Shouldn't we be directly using IAsyncEnumerable<...> overload? Original repro: #82457

In that case this problem doesn't exist and we do nothing here since we never need unspeakable type?

To me this seems like issue in ASP.NET.

@eiriktsarpalis
Copy link
Member Author

eiriktsarpalis commented Mar 20, 2023

To me this seems like issue in ASP.NET.

Maybe, but it's caused by shipped behavior and APIs that will be hard to fix. At the same time, this solves other SG issues that are not specific to ASP.NET's boxing behavior, like private interface implementation support.

Co-authored-by: Dan Moseley <danmose@microsoft.com>
@layomia
Copy link
Contributor

layomia commented Mar 20, 2023

Fix #82457

Seems like the issue shouldn't be closed given that issues raised in https://github.com/dotnet/runtime/pull/83631/files#r1142186623 aren't resolved yet.

Copy link
Member

@krwq krwq left a comment

Choose a reason for hiding this comment

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

As discussed offline. This behavior is ok for now but as follow up for 8.0 we should open the issue to consider opt-in/out switch for this behavior and consider: #83631 (comment) as example of unintended behavior

@eiriktsarpalis eiriktsarpalis merged commit dcd0506 into dotnet:main Mar 20, 2023
@eiriktsarpalis eiriktsarpalis deleted the feature/json-unspeakable-types branch March 20, 2023 20:08
@ghost ghost locked as resolved and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add source gen support for unspeakable types
6 participants