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

Enable property visibility tests in source-gen mode #54526

Merged
merged 5 commits into from
Jul 13, 2021

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Jun 21, 2021

Currently contains changes from #54527, but will be cleaned up after that PR goes in.

@layomia layomia added this to the 6.0.0 milestone Jun 21, 2021
@layomia layomia self-assigned this Jun 21, 2021
@ghost
Copy link

ghost commented Jun 21, 2021

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details
Author: layomia
Assignees: layomia
Labels:

area-System.Text.Json

Milestone: 6.0.0

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@@ -610,13 +609,16 @@ private string GenerateForObject(TypeGenerationSpec typeMetadata)
sb.Append($@"
{PropVarName}[{i}] = {JsonMetadataServicesTypeRef}.CreatePropertyInfo<{memberTypeCompilableName}>(
options,
isProperty: {memberMetadata.IsProperty.ToString().ToLowerInvariant()},
isProperty: {ToCSharpKeyword(memberMetadata.IsProperty)},
Copy link
Member

@steveharter steveharter Jul 8, 2021

Choose a reason for hiding this comment

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

This is now at 14 parameters. I wonder if we should just create the JsonPropertyInfo in one call (with minimal parameters) and then set the individual values.

Another option is to pass a struct by ref. This could be made binary forward-compatible (e.g. we only add fields) but would need to be discussed\designed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree with the feedback that this is getting too long. I like the struct-by-ref idea and will bring it up in design review.


if (!jsonPropertyInfo.SrcGen_IsPublic)
{
if (hasJsonInclude)
Copy link
Member

@steveharter steveharter Jul 8, 2021

Choose a reason for hiding this comment

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

Could this be detected during source generation instead of at runtime? (and avoid passing this info over). Perhaps a compile error would result (trying to access to private member)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this a good idea, but makes testing a bit too difficult to tackle in this PR at this point. Switching from runtime exceptions to compile-time warnings means that test projects won't compile & we'd have to test using a different approach. Will tackle in a follow-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm I think we might have to keep this functionality as-is, given that some of the things that cause property-name conflicts (e.g. naming policy) are specified at runtime only. I'll make sure.

@layomia layomia force-pushed the SrcGenPropertyVisibility branch 4 times, most recently from 8f5785b to 91c74f0 Compare July 12, 2021 16:44
@layomia layomia marked this pull request as ready for review July 12, 2021 17:01
Copy link
Member

@steveharter steveharter left a comment

Choose a reason for hiding this comment

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

LGTM with later feedback after this PR to review the public API for source gen for alternate strategies.

@ghost
Copy link

ghost commented Jul 12, 2021

Hello @layomia!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@layomia layomia merged commit 7b19cce into dotnet:main Jul 13, 2021
@layomia
Copy link
Contributor Author

layomia commented Jul 13, 2021

FYI @romfir, thanks for logging #54385. It has been fixed in this PR, and you can try the fix using the latest build of System.Text.Json - https://dev.azure.com/dnceng/public/_packaging?_a=package&feed=dotnet6&package=System.Text.Json&protocolType=NuGet&view=versions. Please let us know if you have further issues using the source generator.

@romfir
Copy link

romfir commented Jul 13, 2021

@layomia thanks for the info, in the console app it now works, but json serializer stopped working at all with Blazor
(System.Text.Json 6.0.0-preview.7.21363.1, dotnet 6.0.100-preview.5.21302.13), it now throws:

Microsoft.AspNetCore.Components.WebAssembly.Rendering.WebAssemblyRenderer[100]
      Unhandled exception rendering component: Method `System.String.CopyTo(System.Span`1<char>)' is inaccessible from method `System.Text.Json.JsonCamelCaseNamingPolicy+<>c.<ConvertName>b__0_0(System.Span`1<char>,string)'
System.MethodAccessException: Method `System.String.CopyTo(System.Span`1<char>)' is inaccessible from method `System.Text.Json.JsonCamelCaseNamingPolicy+<>c.<ConvertName>b__0_0(System.Span`1<char>,string)'
   at System.Text.Json.JsonCamelCaseNamingPolicy.<>c.<ConvertName>b__0_0(Span`1 chars, String name) in System.Text.Json.dll:token 0x6000140+0x0
   at System.String.Create[String](Int32 length, String state, SpanAction`2 action) in System.Private.CoreLib.dll:token 0x60004fc+0x2d
   at System.Text.Json.JsonCamelCaseNamingPolicy.ConvertName(String name) in System.Text.Json.dll:token 0x600013b+0x18
   at System.Text.Json.Serialization.Metadata.JsonPropertyInfo.DeterminePropertyName() in System.Text.Json.dll:token 0x6000832+0x4c
   at System.Text.Json.Serialization.Metadata.JsonPropertyInfo.GetPolicies(Nullable`1 ignoreCondition, Nullable`1 declaringTypeNumberHandling) in System.Text.Json.dll:token 0x6000831+0x17
   at System.Text.Json.Serialization.Metadata.JsonPropertyInfo`1[[System.String, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].Initialize(Type parentClassType, Type declaredPropertyType, Type runtimePropertyType, ConverterStrategy runtimeClassType, MemberInfo memberInfo, Boolean isVirtual, JsonConverter converter, Nullable`1 ignoreCondition, Nullable`1 parentTypeNumberHandling, JsonSerializerOptions options) in System.Text.Json.dll:token 0x600087b+0x15e
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.CreateProperty(Type declaredPropertyType, Type runtimePropertyType, MemberInfo memberInfo, Type parentClassType, Boolean isVirtual, JsonConverter converter, JsonSerializerOptions options, Nullable`1 parentTypeNumberHandling, Nullable`1 ignoreCondition) in System.Text.Json.dll:token 0x60008b2+0x8
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.AddProperty(MemberInfo memberInfo, Type memberType, Type parentClassType, Boolean isVirtual, Nullable`1 parentTypeNumberHandling, JsonSerializerOptions options) in System.Text.Json.dll:token 0x60008b1+0x52
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.CacheMember(Type declaringType, Type memberType, MemberInfo memberInfo, Boolean isVirtual, Nullable`1 typeNumberHandling, Dictionary`2& ignoredMembers) in System.Text.Json.dll:token 0x60008a4+0x34
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo..ctor(Type type, JsonConverter converter, Type runtimeType, JsonSerializerOptions options) in System.Text.Json.dll:token 0x60008a3+0x116
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo..ctor(Type type, JsonSerializerOptions options) in System.Text.Json.dll:token 0x60008a2+0x0
   at System.Text.Json.JsonSerializerOptions.<RootBuiltInConvertersAndTypeInfoCreator>g__CreateJsonTypeInfo|107_0(Type type, JsonSerializerOptions options) in System.Text.Json.dll:token 0x6000436+0x0
   at System.Text.Json.JsonSerializerOptions.GetClassFromContextOrCreate(Type type) in System.Text.Json.dll:token 0x600042c+0x29
   at System.Text.Json.JsonSerializerOptions.GetOrAddClass(Type type) in System.Text.Json.dll:token 0x600042b+0x17
   at System.Text.Json.WriteStackFrame.InitializeReEntry(Type type, JsonSerializerOptions options) in System.Text.Json.dll:token 0x6000462+0x1a
   at System.Text.Json.Serialization.JsonConverter`1[[System.Object, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].TryWrite(Utf8JsonWriter writer, Object& value, JsonSerializerOptions options, WriteStack& state) in System.Text.Json.dll:token 0x60007ab+0xe3
   at System.Text.Json.Serialization.Converters.ArrayConverter`2[[System.Object[], System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.Object, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].OnWriteResume(Utf8JsonWriter writer, Object[] value, JsonSerializerOptions options, WriteStack& state) in System.Text.Json.dll:token 0x6000917+0x61
   at System.Text.Json.Serialization.Converters.IEnumerableDefaultConverter`2[[System.Object[], System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.Object, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].OnTryWrite(Utf8JsonWriter writer, Object[] value, JsonSerializerOptions options, WriteStack& state) in System.Text.Json.dll:token 0x6000962+0x86
   at System.Text.Json.Serialization.JsonConverter`1[[System.Object[], System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].TryWrite(Utf8JsonWriter writer, Object[]& value, JsonSerializerOptions options, WriteStack& state) in System.Text.Json.dll:token 0x60007ab+0x1f4
   at System.Text.Json.Serialization.JsonConverter`1[[System.Object[], System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].WriteCore(Utf8JsonWriter writer, Object[]& value, JsonSerializerOptions options, WriteStack& state) in System.Text.Json.dll:token 0x6000796+0x0
   at System.Text.Json.JsonSerializer.WriteCore[Object[]](JsonConverter jsonConverter, Utf8JsonWriter writer, Object[]& value, JsonSerializerOptions options, WriteStack& state) in System.Text.Json.dll:token 0x60003c4+0xa
   at System.Text.Json.JsonSerializer.WriteUsingMetadata[Object[]](Utf8JsonWriter writer, Object[]& value, JsonTypeInfo jsonTypeInfo) in System.Text.Json.dll:token 0x60003c5+0x62
   at System.Text.Json.JsonSerializer.WriteUsingMetadata[Object[]](Object[]& value, JsonTypeInfo jsonTypeInfo) in System.Text.Json.dll:token 0x60003d9+0x2e
   at System.Text.Json.JsonSerializer.Write[Object[]](Object[]& value, Type runtimeType, JsonSerializerOptions options) in System.Text.Json.dll:token 0x60003d8+0x8
   at System.Text.Json.JsonSerializer.Serialize[Object[]](Object[] value, JsonSerializerOptions options) in System.Text.Json.dll:token 0x60003d4+0x0
   at Microsoft.JSInterop.JSRuntime.InvokeAsync[Object](Int64 targetInstanceId, String identifier, CancellationToken cancellationToken, Object[] args) in Microsoft.JSInterop.dll:token 0x6000039+0x10a
   at Microsoft.JSInterop.JSRuntime.<InvokeAsync>d__15`1[[System.Object, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].MoveNext() in Microsoft.JSInterop.dll:token 0x600008f+0xe5
   at Microsoft.JSInterop.JSRuntimeExtensions.InvokeVoidAsync(IJSRuntime jsRuntime, String identifier, Object[] args) in Microsoft.JSInterop.dll:token 0x6000042+0x82
   at Microsoft.AspNetCore.Components.WebAssembly.Authentication.RemoteAuthenticationService`3.<EnsureAuthService>d__27[[Microsoft.AspNetCore.Components.WebAssembly.Authentication.RemoteAuthenticationState, Microsoft.AspNetCore.Components.WebAssembly.Authentication, Version=6.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60],[Microsoft.AspNetCore.Components.WebAssembly.Authentication.RemoteUserAccount, Microsoft.AspNetCore.Components.WebAssembly.Authentication, Version=6.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60],[Microsoft.AspNetCore.Components.WebAssembly.Authentication.OidcProviderOptions, Microsoft.AspNetCore.Components.WebAssembly.Authentication, Version=6.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60]].MoveNext() in Microsoft.AspNetCore.Components.WebAssembly.Authentication.dll:token 0x6000122+0x93
   at Microsoft.AspNetCore.Components.WebAssembly.Authentication.RemoteAuthenticationService`3.<GetAuthenticatedUser>d__26[[Microsoft.AspNetCore.Components.WebAssembly.Authentication.RemoteAuthenticationState, Microsoft.AspNetCore.Components.WebAssembly.Authentication, Version=6.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60],[Microsoft.AspNetCore.Components.WebAssembly.Authentication.RemoteUserAccount, Microsoft.AspNetCore.Components.WebAssembly.Authentication, Version=6.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60],[Microsoft.AspNetCore.Components.WebAssembly.Authentication.OidcProviderOptions, Microsoft.AspNetCore.Components.WebAssembly.Authentication, Version=6.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60]].MoveNext() in Microsoft.AspNetCore.Components.WebAssembly.Authentication.dll:token 0x6000120+0x7b
   at Microsoft.AspNetCore.Components.WebAssembly.Authentication.RemoteAuthenticationService`3.<GetUser>d__25[[Microsoft.AspNetCore.Components.WebAssembly.Authentication.RemoteAuthenticationState, Microsoft.AspNetCore.Components.WebAssembly.Authentication, Version=6.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60],[Microsoft.AspNetCore.Components.WebAssembly.Authentication.RemoteUserAccount, Microsoft.AspNetCore.Components.WebAssembly.Authentication, Version=6.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60],[Microsoft.AspNetCore.Components.WebAssembly.Authentication.OidcProviderOptions, Microsoft.AspNetCore.Components.WebAssembly.Authentication, Version=6.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60]].MoveNext() in Microsoft.AspNetCore.Components.WebAssembly.Authentication.dll:token 0x600011e+0xa5
   at Microsoft.AspNetCore.Components.WebAssembly.Authentication.RemoteAuthenticationService`3.<GetAuthenticationStateAsync>d__17[[Microsoft.AspNetCore.Components.WebAssembly.Authentication.RemoteAuthenticationState, Microsoft.AspNetCore.Components.WebAssembly.Authentication, Version=6.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60],[Microsoft.AspNetCore.Components.WebAssembly.Authentication.RemoteUserAccount, Microsoft.AspNetCore.Components.WebAssembly.Authentication, Version=6.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60],[Microsoft.AspNetCore.Components.WebAssembly.Authentication.OidcProviderOptions, Microsoft.AspNetCore.Components.WebAssembly.Authentication, Version=6.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60]].MoveNext() in Microsoft.AspNetCore.Components.WebAssembly.Authentication.dll:token 0x6000110+0x63
   at Microsoft.AspNetCore.Components.Authorization.AuthorizeViewCore.OnParametersSetAsync() in Microsoft.AspNetCore.Components.Authorization.dll:token 0x6000043+0xc2
   at Microsoft.AspNetCore.Components.ComponentBase.CallStateHasChangedOnAsyncCompletion(Task task) in Microsoft.AspNetCore.Components.dll:token 0x60000a2+0x7e
   at Microsoft.AspNetCore.Components.ComponentBase.RunInitAndSetParametersAsync() in Microsoft.AspNetCore.Components.dll:token 0x60000a0+0x113

@layomia
Copy link
Contributor Author

layomia commented Jul 13, 2021

@romfir thanks for the update. Can you please file a new issue for this? Any repro information, e.g. a simplified .csproj that demonstrates the issue, would be very helpful.

@romfir
Copy link

romfir commented Jul 13, 2021

I've created #55568

@ghost ghost locked as resolved and limited conversation to collaborators Aug 12, 2021
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.

System.Text.Json source generator JsonIgnoreCondition missing namespace
3 participants