Update source-gen APIs according to review by layomia · Pull Request #59042 · dotnet/runtime

layomia · 2021-09-13T17:02:14Z

Implements API approved in #45448 (comment). These are cosmetic changes to APIs called by generated code to configure type metadata, to make them easier to extend with new features in future versions of the generator/STJ.

Should be ported to 6.0.

ghost · 2021-09-13T17:02:21Z

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.

ghost · 2021-09-13T17:02:23Z

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

Issue Details

Implements API approved in #45448 (comment). These are cosmetic changes to APIs called by generated code to configure type metadata, to make them easier to extend with new features in future versions of the generator/STJ.

Should be ported to 6.0.

Author:	layomia
Assignees:	layomia
Labels:	`area-System.Text.Json`
Milestone:	6.0.0

layomia · 2021-09-13T21:42:53Z

I want to call out new commit 4ce31e9 ("Rename fast-path func name and add src-gen/JsonNode interop support"). It implements the proposed API for tomorrow - #45448 (comment).

steveharter

LGTM pending API approval especially around the SerializeHandler\Serialize changes.

steveharter · 2021-09-14T14:31:14Z

Possible Mono\WASM issues:

/datadisks/disk1/work/AD8D0967/p/build/wasm/WasmApp.Native.targets(293,5): error : Failed to compile /datadisks/disk1/work/AD8D0967/w/A24F0912/e/wasm_build/obj/wasm/System.Text.Json.SourceGeneration.Tests.dll.bc -> /datadisks/disk1/work/AD8D0967/w/A24F0912/e/wasm_build/obj/wasm/System.Text.Json.SourceGeneration.Tests.dll.o [/datadisks/disk1/work/AD8D0967/w/A24F0912/e/publish/ProxyProjectForAOTOnHelix.proj]

layomia · 2021-09-14T18:52:30Z

LGTM pending API approval especially around the SerializeHandler\Serialize changes.

API has been approved, so this is good to merge when green - #45448 (comment).

eerhardt · 2021-09-16T19:07:55Z

@lewing @radical @SamMonoRT - Have any of you seen the failure before? It appears to be an issue with WASM AOT.

https://helix.dot.net/api/2019-06-17/jobs/770ad140-31bb-4858-9396-d50162076efd/workitems/System.Text.Json.SourceGeneration.Roslyn3.11.Tests/console

 [39/47] xunit.runner.utility.netcoreapp10.dll.bc -> xunit.runner.utility.netcoreapp10.dll.o [took 20.326s]
/datadisks/disk1/work/9D6C08B7/p/build/wasm/WasmApp.Native.targets(293,5): error : Failed to compile /datadisks/disk1/work/9D6C08B7/w/AA0A0963/e/wasm_build/obj/wasm/System.Text.Json.SourceGeneration.Roslyn3.11.Tests.dll.bc -> /datadisks/disk1/work/9D6C08B7/w/AA0A0963/e/wasm_build/obj/wasm/System.Text.Json.SourceGeneration.Roslyn3.11.Tests.dll.o [/datadisks/disk1/work/9D6C08B7/w/AA0A0963/e/publish/ProxyProjectForAOTOnHelix.proj]
/datadisks/disk1/work/9D6C08B7/p/build/wasm/WasmApp.Native.targets(293,5): error : "/datadisks/disk1/work/9D6C08B7/p/build/emsdk/upstream/bin/clang++" -target wasm32-unknown-emscripten -DEMSCRIPTEN -fno-inline-functions -mllvm -combiner-global-alias-analysis=false -mllvm -enable-emscripten-cxx-exceptions -mllvm -enable-emscripten-sjlj -mllvm -disable-lsr -D__EMSCRIPTEN_major__=2 -D__EMSCRIPTEN_minor__=0 -D__EMSCRIPTEN_tiny__=23 -D_LIBCPP_ABI_VERSION=2 -Dunix -D__unix -D__unix__ -Werror=implicit-function-declaration -Xclang -iwithsysroot/include/SDL --sysroot=/datadisks/disk1/work/9D6C08B7/p/build/emsdk/upstream/emscripten/cache/sysroot -Xclang -iwithsysroot/include/compat -Oz -g -v -c /datadisks/disk1/work/9D6C08B7/w/AA0A0963/e/wasm_build/obj/wasm/System.Text.Json.SourceGeneration.Roslyn3.11.Tests.dll.bc -o /tmp/tmpkKeaiw.tmp [/datadisks/disk1/work/9D6C08B7/w/AA0A0963/e/publish/ProxyProjectForAOTOnHelix.proj]
/datadisks/disk1/work/9D6C08B7/p/build/wasm/WasmApp.Native.targets(293,5): error : clang version 13.0.0 (/b/s/w/ir/cache/git/chromium.googlesource.com-external-github.com-llvm-llvm--project 5852582532b3eb3ea8da51a1e272d8d017bd36c9) [/datadisks/disk1/work/9D6C08B7/w/AA0A0963/e/publish/ProxyProjectForAOTOnHelix.proj]
/datadisks/disk1/work/9D6C08B7/p/build/wasm/WasmApp.Native.targets(293,5): error : Target: wasm32-unknown-emscripten [/datadisks/disk1/work/9D6C08B7/w/AA0A0963/e/publish/ProxyProjectForAOTOnHelix.proj]
/datadisks/disk1/work/9D6C08B7/p/build/wasm/WasmApp.Native.targets(293,5): error : Thread model: posix [/datadisks/disk1/work/9D6C08B7/w/AA0A0963/e/publish/ProxyProjectForAOTOnHelix.proj]
/datadisks/disk1/work/9D6C08B7/p/build/wasm/WasmApp.Native.targets(293,5): error : InstalledDir: /datadisks/disk1/work/9D6C08B7/p/build/emsdk/upstream/bin [/datadisks/disk1/work/9D6C08B7/w/AA0A0963/e/publish/ProxyProjectForAOTOnHelix.proj]
/datadisks/disk1/work/9D6C08B7/p/build/wasm/WasmApp.Native.targets(293,5): error : (in-process) [/datadisks/disk1/work/9D6C08B7/w/AA0A0963/e/publish/ProxyProjectForAOTOnHelix.proj]
/datadisks/disk1/work/9D6C08B7/p/build/wasm/WasmApp.Native.targets(293,5): error : "/datadisks/disk1/work/9D6C08B7/p/build/emsdk/upstream/bin/clang++" -cc1 -triple wasm32-unknown-emscripten -emit-obj --mrelax-relocations -disable-free -main-file-name System.Text.Json.SourceGeneration.Roslyn3.11.Tests.dll.bc -mrelocation-model static -mframe-pointer=none -fno-rounding-math -mconstructor-aliases -target-cpu generic -fvisibility hidden -debug-info-kind=limited -dwarf-version=4 -debugger-tuning=gdb -v -fcoverage-compilation-dir=/datadisks/disk1/work/9D6C08B7/w/AA0A0963/e/publish -resource-dir /datadisks/disk1/work/9D6C08B7/p/build/emsdk/upstream/lib/clang/13.0.0 -Oz -Werror=implicit-function-declaration -fdebug-compilation-dir=/datadisks/disk1/work/9D6C08B7/w/AA0A0963/e/publish -ferror-limit 19 -fgnuc-version=4.2.1 -fno-inline-functions -vectorize-slp -iwithsysroot/include/SDL -iwithsysroot/include/compat -mllvm -combiner-global-alias-analysis=false -mllvm -enable-emscripten-cxx-exceptions -mllvm -enable-emscripten-sjlj -mllvm -disable-lsr -o /tmp/tmpkKeaiw.tmp -x ir /datadisks/disk1/work/9D6C08B7/w/AA0A0963/e/wasm_build/obj/wasm/System.Text.Json.SourceGeneration.Roslyn3.11.Tests.dll.bc [/datadisks/disk1/work/9D6C08B7/w/AA0A0963/e/publish/ProxyProjectForAOTOnHelix.proj]
/datadisks/disk1/work/9D6C08B7/p/build/wasm/WasmApp.Native.targets(293,5): error : clang -cc1 version 13.0.0 based upon LLVM 13.0.0git default target x86_64-unknown-linux-gnu [/datadisks/disk1/work/9D6C08B7/w/AA0A0963/e/publish/ProxyProjectForAOTOnHelix.proj]
/datadisks/disk1/work/9D6C08B7/p/build/wasm/WasmApp.Native.targets(293,5): error : emcc: error: '/datadisks/disk1/work/9D6C08B7/p/build/emsdk/upstream/bin/clang++ -target wasm32-unknown-emscripten -DEMSCRIPTEN -fno-inline-functions -mllvm -combiner-global-alias-analysis=false -mllvm -enable-emscripten-cxx-exceptions -mllvm -enable-emscripten-sjlj -mllvm -disable-lsr -D__EMSCRIPTEN_major__=2 -D__EMSCRIPTEN_minor__=0 -D__EMSCRIPTEN_tiny__=23 -D_LIBCPP_ABI_VERSION=2 -Dunix -D__unix -D__unix__ -Werror=implicit-function-declaration -Xclang -iwithsysroot/include/SDL --sysroot=/datadisks/disk1/work/9D6C08B7/p/build/emsdk/upstream/emscripten/cache/sysroot -Xclang -iwithsysroot/include/compat -Oz -g -v -c /datadisks/disk1/work/9D6C08B7/w/AA0A0963/e/wasm_build/obj/wasm/System.Text.Json.SourceGeneration.Roslyn3.11.Tests.dll.bc -o /tmp/tmpkKeaiw.tmp' failed (received SIGKILL (-9)) [took 433.767s] [/datadisks/disk1/work/9D6C08B7/w/AA0A0963/e/publish/ProxyProjectForAOTOnHelix.proj]
XHarness artifacts: /datadisks/disk1/work/9D6C08B7/w/AA0A0963/uploads/xharness-output

radical · 2021-09-16T19:12:33Z

Failed to compile /datadisks/disk1/work/9D6C08B7/w/AA0A0963/e/wasm_build/obj/wasm/System.Text.Json.SourceGeneration.Roslyn3.11.Tests.dll.bc -> /datadisks/disk1/work/9D6C08B7/w/AA0A0963/e/wasm_build/obj/wasm/System.Text.Json.SourceGeneration.Roslyn3.11.Tests.dll.o
... received SIGKILL (-9)

@lewing @radekdoulik Could this be due to running out of memory, or other resources?

layomia · 2021-09-16T19:42:07Z

Was chatting with @eerhardt about the AOT failure offline.. FWIW I wanted to note that the size of the test .dll in question here (System.Text.Json.SourceGeneration.Roslyn4.0.Tests) increased from 4.2 MB to 5.4 MB (+ ~28%) with these new changes. I expect the size to keep increasing as we continue sharing tests between src-gen/reflection JsonSerializer implementations.

lewing · 2021-09-16T20:34:10Z

cc @vargaz

layomia · 2021-09-16T21:37:31Z

In a6c3d7c I disabled the src-gen tests in the WASM/AOT leg to unblock this PR so we can get the changes into 6.0. I opened #59228 to track the disabled tests.

layomia · 2021-09-17T04:19:26Z

/backport to release/6.0-rc2

github-actions · 2021-09-17T04:19:39Z

Started backporting to release/6.0-rc2: https://github.com/dotnet/runtime/actions/runs/1244163258

MichalStrehovsky · 2021-09-28T03:04:39Z

I thought RC1 is our go-live release and people can start shipping on it, including e.g. publishing NuGet packages with pregenerated code in it.

Will the NuGet packages using the RC1 S.T.Json source generator be usable on the RTM runtime?

I'm looking at a MissingMethodException: Missing method 'System.Text.Json.Serialization.Metadata.JsonTypeInfo`1<!!0[]> System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreateArrayInfo(System.Text.Json.JsonSerializerOptions, System.Text.Json.Serialization.Metadata.JsonTypeInfo, System.Text.Json.Serialization.JsonNumberHandling, System.Action`2<System.Text.Json.Utf8JsonWriter, !!0[]>)' that appears to be caused by using an RC1 source generator to generate the code, but running on a post-RC1 runtime.

danmoseley · 2021-09-28T14:55:58Z

@MichalStrehovsky support for go-live releases like RC1 ends as soon as the next one goes out, RC2 in this case.
https://dotnet.microsoft.com/platform/support/policy/dotnet-core

jkotas · 2021-09-28T15:43:59Z

In the past, we have sometimes treated APIs in the go-live releases as frozen. We are clearly not doing that this time.

I have added dotnet/core#6570 (comment) to make sure we mention this in the .NET 6 RC2 announcement.

layomia added the area-System.Text.Json label Sep 13, 2021

layomia added this to the 6.0.0 milestone Sep 13, 2021

layomia requested review from eiriktsarpalis and steveharter September 13, 2021 17:02

layomia self-assigned this Sep 13, 2021

ghost added the new-api-needs-documentation label Sep 13, 2021

steveharter reviewed Sep 13, 2021
View reviewed changes

Comment threadsrc/libraries/System.Text.Json/ref/System.Text.Json.cs

steveharter reviewed Sep 13, 2021
View reviewed changes

Comment threadsrc/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs Outdated

danmoseley reviewed Sep 13, 2021
View reviewed changes

Comment threadsrc/libraries/System.Text.Json/ref/System.Text.Json.cs

steveharter reviewed Sep 14, 2021
View reviewed changes

Comment thread...em.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/NodeInteropTests.cs Outdated

steveharter reviewed Sep 14, 2021
View reviewed changes

Comment thread...braries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonMetadataServices.cs Outdated

steveharter approved these changes Sep 14, 2021
View reviewed changes

runfoappBot mentioned this pull request Sep 14, 2021
TLS1.3 networking tests are flaky on Windows 11 #58927
Closed

layomia force-pushed the Apis branch from 4ce31e9 to a1ac279Compare September 14, 2021 15:44

layomia added 3 commits September 15, 2021 15:50

Update source-gen APIs according to review
32f39f1

Rename fast-path func name and add src-gen/JsonNode interop support
77efb55

Address review feedback
71b9268

layomia force-pushed the Apis branch from 2bfa8db to 71b9268Compare September 15, 2021 22:51

layomia mentioned this pull request Sep 16, 2021
System.Text.Json.SourceGeneration.Tests are disabled in WASM #59228
Closed

Disable src-gen tests in browser target OS
a6c3d7c

layomia merged commit 3409f73 into dotnet:mainSep 17, 2021

github-actionsBot mentioned this pull request Sep 17, 2021
[release/6.0-rc2] Update source-gen APIs according to review #59243
Merged

sebastienros mentioned this pull request Sep 21, 2021
Platform benchmarks failing for .NET6.0 (System.TypeInitializationException) aspnet/Benchmarks#1699
Closed

MichalStrehovsky mentioned this pull request Sep 28, 2021
System.Text.Json using source generators produces "Method not found" dotnet/runtimelab#1498
Closed

karelz mentioned this pull request Sep 29, 2021
What's new in .NET 6 RC2 [WIP] dotnet/core#6570
Closed

eerhardt mentioned this pull request Oct 5, 2021
Update Regex source generator to use new GetBestTypeByMetadataName method #59778
Merged

ghost locked as resolved and limited conversation to collaborators Nov 3, 2021

Uh oh!

Conversation

layomia commented Sep 13, 2021

Uh oh!

ghost commented Sep 13, 2021

Uh oh!

ghost commented Sep 13, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

layomia commented Sep 13, 2021

Uh oh!

Uh oh!

Uh oh!

steveharter left a comment

Choose a reason for hiding this comment

Uh oh!

steveharter commented Sep 14, 2021

Uh oh!

layomia commented Sep 14, 2021

Uh oh!

eerhardt commented Sep 16, 2021

Uh oh!

radical commented Sep 16, 2021

Uh oh!

layomia commented Sep 16, 2021

Uh oh!

lewing commented Sep 16, 2021

Uh oh!

layomia commented Sep 16, 2021

Uh oh!

layomia commented Sep 17, 2021

Uh oh!

github-actionsBot commented Sep 17, 2021

Uh oh!

MichalStrehovsky commented Sep 28, 2021

Uh oh!

danmoseley commented Sep 28, 2021

Uh oh!

jkotas commented Sep 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants