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 doesn't handle ReadOnlySpan<T> properties correctly #98590

Closed
jp2masa opened this issue Feb 17, 2024 · 5 comments · Fixed by #106083
Closed
Assignees
Labels
area-System.Text.Json bug in-pr There is an active PR which will close this issue when it is merged source-generator Indicates an issue with a source generator feature
Milestone

Comments

@jp2masa
Copy link
Member

jp2masa commented Feb 17, 2024

Description

When using the System.Text.Json source generator with ReadOnlySpan<T> properties, the generated code is invalid. Also, IgnoreReadOnlyProperties doesn't skip getter-only properties.

Reproduction Steps

    class Test(string value)
    {
        public string Value =>
            value;

        public ReadOnlySpan<char> AsSpan =>
            value;
    }

    [JsonSourceGenerationOptions(IgnoreReadOnlyProperties = true)]
    [JsonSerializable(typeof(Test))]
    internal partial class AppJsonSerializerContext : JsonSerializerContext
    {
    }

Expected behavior

No compilation error. Also, if IgnoreReadOnlyProperties is supposed to ignore getter-only properties, both properties above should be skipped.

Actual behavior

Multiple compilation errors on generated code, because ReadOnlySpan<char> was used as a generic argument.

Example:

CS0306 The type 'ReadOnlySpan' may not be used as a type argument

Relevant part of the generated code
        private static global::System.Text.Json.Serialization.Metadata.JsonPropertyInfo[] TestPropInit(global::System.Text.Json.JsonSerializerOptions options)
        {
            var properties = new global::System.Text.Json.Serialization.Metadata.JsonPropertyInfo[2];

            var info0 = new global::System.Text.Json.Serialization.Metadata.JsonPropertyInfoValues<string>
            {
                IsProperty = true,
                IsPublic = true,
                IsVirtual = false,
                DeclaringType = typeof(global::TestApp.Test),
                Converter = null,
                Getter = static obj => ((global::TestApp.Test)obj).Value,
                Setter = null,
                IgnoreCondition = null,
                HasJsonInclude = false,
                IsExtensionData = false,
                NumberHandling = null,
                PropertyName = "Value",
                JsonPropertyName = null
            };
            
            properties[0] = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreatePropertyInfo<string>(options, info0);

            var info1 = new global::System.Text.Json.Serialization.Metadata.JsonPropertyInfoValues<global::System.ReadOnlySpan<char>>
            {
                IsProperty = true,
                IsPublic = true,
                IsVirtual = false,
                DeclaringType = typeof(global::TestApp.Test),
                Converter = null,
                Getter = static obj => ((global::TestApp.Test)obj).AsSpan,
                Setter = null,
                IgnoreCondition = null,
                HasJsonInclude = false,
                IsExtensionData = false,
                NumberHandling = null,
                PropertyName = "AsSpan",
                JsonPropertyName = null
            };
            
            properties[1] = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreatePropertyInfo<global::System.ReadOnlySpan<char>>(options, info1);

            return properties;
        }

Regression?

No response

Known Workarounds

No response

Configuration

.NET 8.0.200

Also tried using the latest preview version of System.Text.Json from NuGet.

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Feb 17, 2024
@ghost
Copy link

ghost commented Feb 17, 2024

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

Description

When using the System.Text.Json source generator with ReadOnlySpan<T> properties, the generated code is invalid. Also, IgnoreReadOnlyProperties doesn't skip getter-only properties.

Reproduction Steps

    class Test(string value)
    {
        public string Value =>
            value;

        public ReadOnlySpan<char> AsSpan =>
            value;
    }

    [JsonSourceGenerationOptions(IgnoreReadOnlyProperties = true)]
    [JsonSerializable(typeof(Test))]
    internal partial class AppJsonSerializerContext : JsonSerializerContext
    {
    }

Expected behavior

No compilation error. Also, if IgnoreReadOnlyProperties is supposed to ignore getter-only properties, both properties above should be skipped.

Actual behavior

Multiple compilation errors on generated code, because ReadOnlySpan<char> was used as a generic argument.

Example:

CS0306 The type 'ReadOnlySpan' may not be used as a type argument

Relevant part of the generated code
        private static global::System.Text.Json.Serialization.Metadata.JsonPropertyInfo[] TestPropInit(global::System.Text.Json.JsonSerializerOptions options)
        {
            var properties = new global::System.Text.Json.Serialization.Metadata.JsonPropertyInfo[2];

            var info0 = new global::System.Text.Json.Serialization.Metadata.JsonPropertyInfoValues<string>
            {
                IsProperty = true,
                IsPublic = true,
                IsVirtual = false,
                DeclaringType = typeof(global::TestApp.Test),
                Converter = null,
                Getter = static obj => ((global::TestApp.Test)obj).Value,
                Setter = null,
                IgnoreCondition = null,
                HasJsonInclude = false,
                IsExtensionData = false,
                NumberHandling = null,
                PropertyName = "Value",
                JsonPropertyName = null
            };
            
            properties[0] = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreatePropertyInfo<string>(options, info0);

            var info1 = new global::System.Text.Json.Serialization.Metadata.JsonPropertyInfoValues<global::System.ReadOnlySpan<char>>
            {
                IsProperty = true,
                IsPublic = true,
                IsVirtual = false,
                DeclaringType = typeof(global::TestApp.Test),
                Converter = null,
                Getter = static obj => ((global::TestApp.Test)obj).AsSpan,
                Setter = null,
                IgnoreCondition = null,
                HasJsonInclude = false,
                IsExtensionData = false,
                NumberHandling = null,
                PropertyName = "AsSpan",
                JsonPropertyName = null
            };
            
            properties[1] = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreatePropertyInfo<global::System.ReadOnlySpan<char>>(options, info1);

            return properties;
        }

Regression?

No response

Known Workarounds

No response

Configuration

.NET 8.0.200

Also tried using the latest preview version of System.Text.Json from NuGet.

Other information

No response

Author: jp2masa
Assignees: -
Labels:

area-System.Text.Json, untriaged

Milestone: -

@elgonzo
Copy link

elgonzo commented Feb 17, 2024

Also, IgnoreReadOnlyProperties doesn't skip getter-only properties.

Not sure if this statement is meant to apply specifically to Span/ReadOnlySpan-typed properties, or if this statement was meant to apply to getter-only properties of any type. Commenting out the AsSpan property, serializing a Test instance will result in an empty json object {}. If you see a getter-only property (that is not a Span type) not being ignored, please post a code example that reproduces this behavior.

@eiriktsarpalis
Copy link
Member

IgnoreReadOnlyProperties is a run-time setting that can be overridden, as such it has no impact as to what metadata is being generated. That being said, I was surprised to discover that it's not even possible to avoid generation using a JsonIgnore attribute. We should try to fix this in the future.

In the meantime as a workaround you could either mark the property internal or convert it to a method.

@eiriktsarpalis eiriktsarpalis added bug and removed untriaged New issue has not been triaged by the area owner labels Feb 17, 2024
@eiriktsarpalis eiriktsarpalis modified the milestones: Future, 9.0.0 Feb 17, 2024
@jp2masa
Copy link
Member Author

jp2masa commented Feb 17, 2024

Unfortunately, it's not my code, but it's open source, so I can fork it. Would skipping all properties whose type IsByRef be an acceptable solution? The generated code will always be invalid, I think.

The only alternative I can think of would be implementing an API for conversion or explicit casting of types like:

[JsonSourceGenerationConvert(typeof(ReadOnlySpan<char>), typeof(string))]

In this case, the property type would be replaced with string, and conversion would be done in the generated code.

I was also a bit disappointed to find that I can't declare a member with the same name as the generated one to replace it, as the generator doesn't care if it's creating a duplicate declaration. Is this by design, or would it be acceptable to skip already declared members? This would make it much easier to workaround bugs or customize generated code.

If it helps, I can work on a PR to fix this.

@eiriktsarpalis
Copy link
Member

More likely, it would involve updating the SG so that all ref-like property types get skipped.

@eiriktsarpalis eiriktsarpalis added the source-generator Indicates an issue with a source generator feature label Jul 5, 2024
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Aug 7, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Sep 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json bug in-pr There is an active PR which will close this issue when it is merged source-generator Indicates an issue with a source generator feature
Projects
None yet
3 participants