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

Add sourcegen support for required & init-only properties. #79828

Merged

Conversation

eiriktsarpalis
Copy link
Member

Updates the source generator to include support for required and init properties. This is done by piggybacking on the existing parameteric constructor infrastructure using property initializer syntax. For example, given the following type:

public class MyPoco
{
    public required int X { get; set; }
    public int Y { get; init; }
}

The source generator will now generate metadata treating X and Y as pseudo-constructor parameters and emit a delegate as follows:

ObjectWithParameterizedConstructorCreator = static (args) => new MyPoco() { X = (int)args[0], Y = (int)args[1] }

Fix #58770.

@ghost
Copy link

ghost commented Dec 19, 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

Updates the source generator to include support for required and init properties. This is done by piggybacking on the existing parameteric constructor infrastructure using property initializer syntax. For example, given the following type:

public class MyPoco
{
    public required int X { get; set; }
    public int Y { get; init; }
}

The source generator will now generate metadata treating X and Y as pseudo-constructor parameters and emit a delegate as follows:

ObjectWithParameterizedConstructorCreator = static (args) => new MyPoco() { X = (int)args[0], Y = (int)args[1] }

Fix #58770.

Author: eiriktsarpalis
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

@@ -243,13 +243,13 @@ public async Task TestJsonPathDoesNotFailOnMultiThreads()
await Task.WhenAll(tasks);
}

private async void TestIdTask()
private async Task TestIdTask()
Copy link
Member Author

@eiriktsarpalis eiriktsarpalis Dec 19, 2022

Choose a reason for hiding this comment

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

@eiriktsarpalis
Copy link
Member Author

eiriktsarpalis commented Dec 21, 2022

The test failures appear to be impacting release builds of wasm only. I'm able to reproduce the failures locally, and all seem related to an issue in which JsonSerializerOptions.ReferenceHandler configuration is being ignored. The error appears consistently and only occurs if the entire test suite is being run -- individual test runs are passing without issue. It also seems unrelated to the testing infrastructure changes in this PR, refactoring to a simple unit test that just calls into JsonSerializer is also failing. I'll keep investigating but at this point I'm running out of theories.

@layomia
Copy link
Contributor

layomia commented Dec 21, 2022

Sounds like the options configuration/instances might not be flowing through properly following the change to move the fast-path decision logic to the context (or just how the context/options are being instantiated). Since individual runs pass my mind also goes to threading issues with the options caching optimizations added in .NET 7. Not sure if these were considered but wanted to mention.

@eiriktsarpalis
Copy link
Member Author

Since individual runs pass my mind also goes to threading issues with the options caching optimizations added in .NET 7. Not sure if these were considered but wanted to mention.

Had the same thought, but the errors keep popping up even with global caching disabled.

Something stranger than that is happening. I've been able to isolate the test failures to using the Roslyn4.4 target when running tests: #79890

Steps to reproduce (on WSL) :

./build.sh -os Browser -configuration Release && 
./dotnet.sh build -t:Test src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/System.Text.Json.SourceGeneration.Roslyn4.4.Tests.csproj \
    -p:TargetOS=Browser -p:TargetArchitecture=wasm -p:Configuration=Release

@eiriktsarpalis
Copy link
Member Author

PR blocked until #79943 can be addressed.

@eiriktsarpalis eiriktsarpalis added the blocked Issue/PR is blocked on something - see comments label Dec 23, 2022
@eiriktsarpalis
Copy link
Member Author

Given that #79943 has been fixed, rebasing changes to see if it resolves CI issues.

@eiriktsarpalis eiriktsarpalis removed the blocked Issue/PR is blocked on something - see comments label Jan 9, 2023
@eiriktsarpalis eiriktsarpalis merged commit 1c735ed into dotnet:main Jan 9, 2023
@eiriktsarpalis eiriktsarpalis deleted the fix/src-gen-init-required branch January 9, 2023 20:04
eiriktsarpalis added a commit to eiriktsarpalis/runtime that referenced this pull request Jan 11, 2023
eiriktsarpalis added a commit that referenced this pull request Jan 12, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 10, 2023
@jeffhandley jeffhandley added the blog-candidate Completed PRs that are candidate topics for blog post coverage label Mar 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json blog-candidate Completed PRs that are candidate topics for blog post coverage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System.Text.Json source generator fails to deserialize init-only & required properties
4 participants