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

System.Text.Json source generator does not ignore delegate properties #62354

Closed
mphilipp622 opened this issue Dec 3, 2021 · 7 comments · Fixed by #63495
Closed

System.Text.Json source generator does not ignore delegate properties #62354

mphilipp622 opened this issue Dec 3, 2021 · 7 comments · Fixed by #63495
Assignees
Milestone

Comments

@mphilipp622
Copy link

mphilipp622 commented Dec 3, 2021

Description

When using the dotnet 6 System.Text.Json source generator on a class or record that contains a delegate type (Func, Action), a CS0149 Method Name Expected error is thrown from the generated g.cs file. This happens even if using the [JsonIgnore] property.

Reproduction Steps

Create a dotnet 6 project and create a new class/record with the source generator. The following code will reproduce the problem minimally:

public record TestRecord(string Test, Func<string, bool> Callback);

[JsonSerializable(typeof(TestRecord))]
public partial class TestRecordGenerationContext : JsonSerializerContext { }

Expected behavior

The expected behavior is that the g.cs files are generated without error and the [JsonIgnore] attribute is properly used for delegate types.

Actual behavior

Currently, the g.cs files are generated but throw a CS0149 Method Name Expected error.

Regression?

No response

Known Workarounds

Only workaround I've used so far is to completely remove the delegate types as member properties or member variables from the class or converting them to a method (if possible for your requirements) so the source generator ignores it.

Configuration

  • Which version of .NET is the code running on? dotnet 6.0
  • What OS and version, and what distro if applicable? Windows 10 64-bit
  • What is the architecture (x64, x86, ARM, ARM64)? x64
  • Do you know whether it is specific to that configuration? Unsure
  • If you're using Blazor, which web browser(s) do you see this issue in? I am using Blazor, but this isn't a runtime issue

Other information

For where the issue might be, perhaps the source generator does properly ignore the member for the generated serialization code, but it doesn't ignore it when traversing the member hierarchy to determine which classes need serialization code generated. Not sure.

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Dec 3, 2021
@ghost
Copy link

ghost commented Dec 3, 2021

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

Issue Details

Description

When using the dotnet 6 System.Text.Json source generator on a class or record that contains a delegate type (Func, Action), a CS0149 Method Name Expected error is thrown from the generated g.cs file. This happens even if using the [JsonIgnore] property.

Reproduction Steps

Create a dotnet 6 project and create a new class/record with the source generator. The following code will reproduce the problem minimally:

`
public record TestRecord(string Test, Func<string, bool> Callback);

[JsonSerializable(typeof(TestRecord))]
public partial class TestRecordGenerationContext : JsonSerializerContext { }
`

Expected behavior

The expected behavior is that the g.cs files are generated without error and the [JsonIgnore] attribute is properly used for delegate types.

Actual behavior

Currently, the g.cs files are generated but throw a CS0149 Method Name Expected error.

Regression?

No response

Known Workarounds

Only workaround I've used so far is to completely remove the delegate types as member properties or member variables from the class or converting them to a method (if possible for your requirements) so the source generator ignores it.

Configuration

  • Which version of .NET is the code running on? dotnet 6.0
  • What OS and version, and what distro if applicable? **Windows 10 64-bit**
    
  • What is the architecture (x64, x86, ARM, ARM64)? **x64**
    
  • Do you know whether it is specific to that configuration? **Unsure**
    
  • If you're using Blazor, which web browser(s) do you see this issue in? **I am using Blazor, but this isn't a runtime issue**
    

Other information

For where the issue might be, perhaps the source generator does properly ignore the member for the generated serialization code, but it doesn't ignore it when traversing the member hierarchy to determine which classes need serialization code generated. Not sure.

Author: mphilipp622
Assignees: -
Labels:

area-System.Text.Json, untriaged

Milestone: -

@eiriktsarpalis
Copy link
Member

Minimal repro:

public class TestClass
{
    [JsonIgnore] 
    Func<string, bool> Callback { get; set; }
}

[JsonSerializable(typeof(TestClass))]
public partial class TestRecordGenerationContext : JsonSerializerContext { }

The source generator appears to be trying to deserialize delegates using the Func(System.Object, IntPtr) constructor:

ObjectWithParameterizedConstructorCreator = static (args) => new global::System.Func<global::System.String, global::System.Boolean>((global::System.Object)args[0], (global::System.IntPtr)args[1]),

which is what produces the compiler error. Adding JsonIgnoreAttribute doesn't appear to suppress generation from the time. This suggests to me that this is a bug we might want to service in 6.

@eiriktsarpalis eiriktsarpalis added bug and removed untriaged New issue has not been triaged by the area owner labels Dec 6, 2021
@eiriktsarpalis eiriktsarpalis added this to the 6.0.x milestone Dec 6, 2021
@ericstj
Copy link
Member

ericstj commented Dec 6, 2021

This works with the runtime-serializer (no source gen), so that's a workaround. It seems that both metadata and fast-path generator produce the bad code.

Honoring JsonIgnore here seems like a good safety mechanism. Folks should have this as a tool to workaround cases where we can't handle properties. It's similar reasoning to why we took 784687f.

I'd also like us to look at why the member discovery is different between source-gen and runtime here, since even without JsonIgnore source gen tries to serialize this private property.

@fbrosseau
Copy link

Even when ignored types/properties do generate valid code, it results in very bloated deadcode that ends up in the final assembly, for instance I just encountered a JsonIgnored' X509Certificate2, which apparently can be source-generated fine, but comes with an enormous tree of deadcode for all of the various properties that that type contains. Plus the obsolete warnings due to some obsolete attributes somewhere in there. (bug #62076)

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 7, 2022
@layomia
Copy link
Contributor

layomia commented Jan 7, 2022

I opened a fix for this issue - #63495.

I'd also like us to look at why the member discovery is different between source-gen and runtime here, since even without JsonIgnore source gen tries to serialize this private property.

Source gen doesn't try to serialize the property at runtime, but includes/generates metadata for the property to be inspected by the serializer at runtime so that it can handle things like property name collisions due to runtime-specified settings such as options.PropertyNamingPolicy.

Even when ignored types/properties do generate valid code, it results in very bloated deadcode that ends up in the final assembly, for instance I just encountered a JsonIgnored' X509Certificate2, which apparently can be source-generated fine, but comes with an enormous tree of deadcode for all of the various properties that that type contains.

@fbrosseau as mentioned right above, the current strategy is to generate metadata for ignored properties and their corresponding data types since this information might be needed at runtime.

Can you create a separate issue for your scenario? In 7.0/future, we might be able to move more of the property resolution phase of the serializer to compile time and possibly generate less code for ignored members.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 7, 2022
@layomia
Copy link
Contributor

layomia commented Jan 8, 2022

Reopening to consider for 6.0 servicing - #63514.

@layomia layomia reopened this Jan 8, 2022
@layomia
Copy link
Contributor

layomia commented Jan 10, 2022

Fixed for 6.0.2 in #63514.

@layomia layomia closed this as completed Jan 10, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Feb 10, 2022
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.

5 participants