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

Get System.Text.Json.SourceGeneration tests running with NativeAot #73431

Closed
Tracked by #79122
MichalStrehovsky opened this issue Aug 5, 2022 · 5 comments · Fixed by #86975
Closed
Tracked by #79122

Get System.Text.Json.SourceGeneration tests running with NativeAot #73431

MichalStrehovsky opened this issue Aug 5, 2022 · 5 comments · Fixed by #86975
Assignees
Labels
area-System.Text.Json blocking-release Priority:1 Work that is critical for the release, but we could probably ship without test-enhancement Improvements of test source code
Milestone

Comments

@MichalStrehovsky
Copy link
Member

It would be good to go over these failures to see if #73124 is really the only AOT issue.

Full run log is here:
jsonfail.txt

The main buckets:

  • xUnit doing its thing. If a Fact/Theory is a generic method, xUnit will MakeGeneric it at runtime. This tends to fail. Can be worked around with RD.XML. Not a customer issue.
  • Enum converter. I believe this is [API Proposal]: A pattern for AOT-friendly source-generated JsonConverterFactory-based converters. #73124
    [FAIL] System.Text.Json.SourceGeneration.Tests.JsonSerializerContextTests.SupportsGenericParameterWithCustomConverterFactory
    System.Reflection.MissingMetadataException : 'System.Text.Json.Serialization.Converters.EnumConverter<System.Text.Json.SourceGeneration.Tests.JsonSerializerContextTests.TestEnum>' is missing metadata. For more information, please visit http://go.microsoft.com/fwlink/?LinkID=392859
      at System.Reflection.Runtime.TypeInfos.RuntimeTypeInfo.get_TypeHandle() + 0x8e
      at System.Reflection.Runtime.MethodInfos.RuntimePlainConstructorInfo`1.Invoke(BindingFlags, Binder, Object[], CultureInfo) + 0x2f
       at System.ActivatorImplementation.CreateInstance(Type, BindingFlags, Binder, Object[], CultureInfo, Object[]) + 0x256
       at System.Text.Json.Serialization.Converters.EnumConverterFactory.Create(Type, EnumConverterOptions, JsonNamingPolicy, JsonSerializerOptions) + 0x8c
       at System.Text.Json.SourceGeneration.Tests.JsonSerializerContextTests.GenericParameterWithCustomConverterFactoryContext.GetConverterFromFactory(JsonSerializerOptions, Type, JsonConverterFactory) + 0x27
       at System.Text.Json.SourceGeneration.Tests.JsonSerializerContextTests.GenericParameterWithCustomConverterFactoryContext.Create_TestEnum(JsonSerializerOptions) + 0x81
       at System.Text.Json.SourceGeneration.Tests.JsonSerializerContextTests.GenericParameterWithCustomConverterFactoryContext.ListTestEnumSerializeHandler(Utf8JsonWriter, List`1) + 0x8f
       at System.Text.Json.JsonSerializer.WriteCore[TValue](Utf8JsonWriter, TValue&, JsonTypeInfo`1) + 0x6e
       at System.Text.Json.JsonSerializer.WriteString[TValue](TValue&, JsonTypeInfo`1) + 0xe0
       at System.Text.Json.JsonSerializer.Serialize[TValue](TValue, JsonTypeInfo`1) + 0x3c
    
  • Not sure what this is but looks different:
    [FAIL] System.Text.Json.SourceGeneration.Tests.SerializationContextTests.RoundtripWithCustomConverterProperty_Struct
    System.Reflection.MissingRuntimeArtifactException : MakeGenericMethod() cannot create this generic method instantiation because no code was generated for it: 
    'System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver.CreateReflectionJsonTypeInfo<System.Text.Json.SourceGeneration.Tests.StructWithCustomConverterProperty>(System.Text.Json.JsonSerializerOptions)'.
    at System.Reflection.Runtime.MethodInfos.RuntimeNamedMethodInfo`1.GetUncachedMethodInvoker(RuntimeTypeInfo[], MemberInfo) + 0x2f
    at System.Reflection.Runtime.MethodInfos.RuntimeNamedMethodInfo`1.MakeGenericMethod(Type[]) + 0x201
    at System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver.CreateJsonTypeInfo(Type, JsonSerializerOptions) + 0x8e
    at System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver.GetTypeInfo(Type, JsonSerializerOptions) + 0x47
    at System.Text.Json.JsonSerializerOptions.GetTypeInfoNoCaching(Type) + 0x2a
    at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey, Func`2) + 0x85
    at System.Text.Json.JsonSerializerOptions.GetTypeInfoInternal(Type, Boolean, Boolean) + 0x35
    at System.Text.Json.JsonSerializer.GetTypeInfo(JsonSerializerOptions, Type) + 0x71
    at System.Text.Json.SourceGeneration.Tests.RealWorldContextTests.RoundtripWithCustomConverterProperty_Struct() + 0x10d
    
  • This also looks different:
    [FAIL] System.Text.Json.SourceGeneration.Tests.SerializationContextTests.RoundtripWithCustomConverterProperty_Class
    System.Reflection.MissingRuntimeArtifactException : MakeGenericMethod() cannot create this generic method instantiation because no code was generated for it: 'System.Text.Json.Serialization.Metadata.JsonTypeInfo.CreateJsonPropertyInfo<System.Int32>(System.Text.Json.Serialization.Metadata.JsonTypeInfo,System.Text.Json.JsonSerializerOptions)'.
    at System.Reflection.Runtime.MethodInfos.RuntimeNamedMethodInfo`1.GetUncachedMethodInvoker(RuntimeTypeInfo[], MemberInfo) + 0x2f
    at System.Reflection.Runtime.MethodInfos.RuntimeNamedMethodInfo`1.MakeGenericMethod(Type[]) + 0x201
    at System.Text.Json.Serialization.Metadata.JsonTypeInfo.CreatePropertyUsingReflection(Type) + 0xdf
    at System.Text.Json.Serialization.Metadata.ReflectionJsonTypeInfo`1.CreateProperty(Type, MemberInfo, JsonSerializerOptions, Boolean) + 0xa4
    at System.Text.Json.Serialization.Metadata.ReflectionJsonTypeInfo`1.AddPropertiesAndParametersUsingReflection() + 0x1fa
    at System.Text.Json.Serialization.Metadata.ReflectionJsonTypeInfo`1..ctor(JsonConverter, JsonSerializerOptions) + 0x8c
    at System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver.CreateReflectionJsonTypeInfo[T](JsonSerializerOptions) + 0x48
    
    

I haven't really gone through everything.

Going to block this all on this bug. There's little point testing S.T.Json with NativeAOT if it doesn't work.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Aug 5, 2022
@ghost
Copy link

ghost commented Aug 5, 2022

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

It would be good to go over these failures to see if #73124 is really the only AOT issue.

Full run log is here:
jsonfail.txt

The main buckets:

  • xUnit doing its thing. If a Fact/Theory is a generic method, xUnit will MakeGeneric it at runtime. This tends to fail. Can be worked around with RD.XML. Not a customer issue.
  • Enum converter. I believe this is [API Proposal]: A pattern for AOT-friendly source-generated JsonConverterFactory-based converters. #73124
    [FAIL] System.Text.Json.SourceGeneration.Tests.JsonSerializerContextTests.SupportsGenericParameterWithCustomConverterFactory
    System.Reflection.MissingMetadataException : 'System.Text.Json.Serialization.Converters.EnumConverter<System.Text.Json.SourceGeneration.Tests.JsonSerializerContextTests.TestEnum>' is missing metadata. For more information, please visit http://go.microsoft.com/fwlink/?LinkID=392859
      at System.Reflection.Runtime.TypeInfos.RuntimeTypeInfo.get_TypeHandle() + 0x8e
      at System.Reflection.Runtime.MethodInfos.RuntimePlainConstructorInfo`1.Invoke(BindingFlags, Binder, Object[], CultureInfo) + 0x2f
       at System.ActivatorImplementation.CreateInstance(Type, BindingFlags, Binder, Object[], CultureInfo, Object[]) + 0x256
       at System.Text.Json.Serialization.Converters.EnumConverterFactory.Create(Type, EnumConverterOptions, JsonNamingPolicy, JsonSerializerOptions) + 0x8c
       at System.Text.Json.SourceGeneration.Tests.JsonSerializerContextTests.GenericParameterWithCustomConverterFactoryContext.GetConverterFromFactory(JsonSerializerOptions, Type, JsonConverterFactory) + 0x27
       at System.Text.Json.SourceGeneration.Tests.JsonSerializerContextTests.GenericParameterWithCustomConverterFactoryContext.Create_TestEnum(JsonSerializerOptions) + 0x81
       at System.Text.Json.SourceGeneration.Tests.JsonSerializerContextTests.GenericParameterWithCustomConverterFactoryContext.ListTestEnumSerializeHandler(Utf8JsonWriter, List`1) + 0x8f
       at System.Text.Json.JsonSerializer.WriteCore[TValue](Utf8JsonWriter, TValue&, JsonTypeInfo`1) + 0x6e
       at System.Text.Json.JsonSerializer.WriteString[TValue](TValue&, JsonTypeInfo`1) + 0xe0
       at System.Text.Json.JsonSerializer.Serialize[TValue](TValue, JsonTypeInfo`1) + 0x3c
    
  • Not sure what this is but looks different:
    [FAIL] System.Text.Json.SourceGeneration.Tests.SerializationContextTests.RoundtripWithCustomConverterProperty_Struct
    System.Reflection.MissingRuntimeArtifactException : MakeGenericMethod() cannot create this generic method instantiation because no code was generated for it: 
    'System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver.CreateReflectionJsonTypeInfo<System.Text.Json.SourceGeneration.Tests.StructWithCustomConverterProperty>(System.Text.Json.JsonSerializerOptions)'.
    at System.Reflection.Runtime.MethodInfos.RuntimeNamedMethodInfo`1.GetUncachedMethodInvoker(RuntimeTypeInfo[], MemberInfo) + 0x2f
    at System.Reflection.Runtime.MethodInfos.RuntimeNamedMethodInfo`1.MakeGenericMethod(Type[]) + 0x201
    at System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver.CreateJsonTypeInfo(Type, JsonSerializerOptions) + 0x8e
    at System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver.GetTypeInfo(Type, JsonSerializerOptions) + 0x47
    at System.Text.Json.JsonSerializerOptions.GetTypeInfoNoCaching(Type) + 0x2a
    at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey, Func`2) + 0x85
    at System.Text.Json.JsonSerializerOptions.GetTypeInfoInternal(Type, Boolean, Boolean) + 0x35
    at System.Text.Json.JsonSerializer.GetTypeInfo(JsonSerializerOptions, Type) + 0x71
    at System.Text.Json.SourceGeneration.Tests.RealWorldContextTests.RoundtripWithCustomConverterProperty_Struct() + 0x10d
    
  • This also looks different:
    [FAIL] System.Text.Json.SourceGeneration.Tests.SerializationContextTests.RoundtripWithCustomConverterProperty_Class
    System.Reflection.MissingRuntimeArtifactException : MakeGenericMethod() cannot create this generic method instantiation because no code was generated for it: 'System.Text.Json.Serialization.Metadata.JsonTypeInfo.CreateJsonPropertyInfo<System.Int32>(System.Text.Json.Serialization.Metadata.JsonTypeInfo,System.Text.Json.JsonSerializerOptions)'.
    at System.Reflection.Runtime.MethodInfos.RuntimeNamedMethodInfo`1.GetUncachedMethodInvoker(RuntimeTypeInfo[], MemberInfo) + 0x2f
    at System.Reflection.Runtime.MethodInfos.RuntimeNamedMethodInfo`1.MakeGenericMethod(Type[]) + 0x201
    at System.Text.Json.Serialization.Metadata.JsonTypeInfo.CreatePropertyUsingReflection(Type) + 0xdf
    at System.Text.Json.Serialization.Metadata.ReflectionJsonTypeInfo`1.CreateProperty(Type, MemberInfo, JsonSerializerOptions, Boolean) + 0xa4
    at System.Text.Json.Serialization.Metadata.ReflectionJsonTypeInfo`1.AddPropertiesAndParametersUsingReflection() + 0x1fa
    at System.Text.Json.Serialization.Metadata.ReflectionJsonTypeInfo`1..ctor(JsonConverter, JsonSerializerOptions) + 0x8c
    at System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver.CreateReflectionJsonTypeInfo[T](JsonSerializerOptions) + 0x48
    
    

I haven't really gone through everything.

Going to block this all on this bug. There's little point testing S.T.Json with NativeAOT if it doesn't work.

Author: MichalStrehovsky
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Aug 5, 2022

A common theme in these stacktraces is the presence of Verify() methods:

public void Verify()
{
int count = 0;
foreach (object data in MyData)
{
if (data is JsonElement element)
{
SimpleTestClass obj = JsonSerializer.Deserialize<SimpleTestClass>(element.GetRawText());
obj.Verify();
}
else
{
((SimpleTestClass)data).Verify();
}
count++;
}
Assert.Equal(2, count);
}
}

TL;DR sourcegen uses reflection serialization as the baseline for validating serialization results (FWIW the same holds for the stacktraces you're highlighting). We should try to remove that dependency but it is going to be a fairly involved exercise. This is made harder by the fact that a lot of these types are shared between sourcegen and reflection test suites. Ideally we should get to a point where it is possible to enable RequiresDynamicCode warnings in the sourcegen test projects.

cc @krwq @layomia

@eiriktsarpalis eiriktsarpalis added test-enhancement Improvements of test source code and removed untriaged New issue has not been triaged by the area owner labels Aug 5, 2022
@eiriktsarpalis eiriktsarpalis added this to the 7.0.0 milestone Aug 5, 2022
@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Aug 5, 2022

For the Verify methods specifically, serialization is a quick-and-dirty method to obtain structural equality. We should probably do the legwork and implement proper structural equality comparison that doesn't depend on reflection not supported by AOT (i.e. write boilerplate that compares fields one-by-one).

@jeffhandley jeffhandley modified the milestones: 7.0.0, Future Aug 9, 2022
@eiriktsarpalis eiriktsarpalis modified the milestones: Future, 8.0.0 Sep 2, 2022
@eiriktsarpalis eiriktsarpalis added Priority:1 Work that is critical for the release, but we could probably ship without Priority:2 Work that is important, but not critical for the release and removed Priority:1 Work that is critical for the release, but we could probably ship without labels Sep 27, 2022
@layomia layomia added Priority:1 Work that is critical for the release, but we could probably ship without and removed Priority:2 Work that is important, but not critical for the release labels Dec 13, 2022
@layomia
Copy link
Contributor

layomia commented Dec 13, 2022

Triage / action items:

  • Do infrastructure work to run tests in NativeAOT in CI; leaning on prior art from existing AOT testing (Cost: S)
    • Produce a list of issues that need to be fixed
  • Re-triage this list and decide what needs to be fixed in 8.0 (Cost: TBD)

@MichalStrehovsky
Copy link
Member Author

If we had the S.T.Json tests running with NativeAOT in the CI, we'd probably be able to see the issue described in #84922 (comment) before it's reported by customers. The warning would fail the test build due to WarningsAsErrors.

@eiriktsarpalis eiriktsarpalis self-assigned this May 31, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 31, 2023
eiriktsarpalis added a commit to eiriktsarpalis/runtime that referenced this issue May 31, 2023
eiriktsarpalis added a commit that referenced this issue Jun 2, 2023
* Enable Native AOT testing in System.Text.Json. Fix #73431.

* Enabled AOT testing for generic theories/MakeGenericMethod tests. Address misc feedback.

* Remove hardcoded JsonSerializer calls from test suite.
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 2, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json blocking-release Priority:1 Work that is critical for the release, but we could probably ship without test-enhancement Improvements of test source code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants