Remove C# dynamic support from JsonNode by steveharter · Pull Request #55430 · dotnet/runtime

steveharter · 2021-07-09T20:00:04Z

See discussion at #53195

We can always add it back in v.next, or add a "new (more modern) dynamic" if\when that comes out.

ghost · 2021-07-09T20:00:08Z

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

Issue Details

See discussion at #53195

We can always add it back in v.next, or add a "new (more modern) dynamic" if\when that comes out.

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

ghost · 2021-07-09T20:00:09Z

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.

src/libraries/System.Text.Json/src/System.Text.Json.csproj

src/libraries/System.Text.Json/ref/System.Text.Json.cs

src/libraries/System.Text.Json/tests/System.Text.Json.Tests/JsonNode/JsonArrayTests.cs

src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonNode.Dynamic.cs

layomia · 2021-07-13T13:35:27Z

...stem.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/StackOfTConverter.cs

            ((TCollection)state.Current.ReturnValue!).Push(value);
        }

+        [DynamicDependency(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor, typeof(Stack<>))]


I'm not sure we should take this approach. In the PR to support all the collection types in source-gen mode, #55566, these converters are being used by the source generator. That PR modifies them such that they perform no reflection/rooting (in source-gen mode), e.g. this diff.

I believe the plan is to replace the trimming tests that use the pre-existing serializer overloads with new ones that use the source generator instead - #53437. Given this, could we disable the failing Stack test in this PR and replace it after #55566 goes in? cc @eerhardt

Agreed. I'd rather we just delete the trimming test until we have source gen support.

Do we know why this change is causing the test to fail though?

steveharter · 2021-07-13T14:47:40Z

Test failure appears infrastructure-related:

2021-07-12 21:45:20,080: INFO: 4546600384: run(141): main: Main thread starting 10 workers
2021-07-12 21:45:25,241: WARNING: 4546600384: run(169): main: Unhandled exception trying to report to ADO: no element found: line 33, column 3
2021-07-12 21:45:25,263: WARNING: 4546600384: run(170): main: We'll attempt to count the XUnit results and if XML is present and no failures, return 0
Searching '/tmp/helix/working/A2B508F9/w/B04809E0/e/..' for log files
Found log '/tmp/helix/working/A2B508F9/w/B04809E0/e/../console.b67234d6.log'
Uri 'https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-55430-merge-18140ce380bd48d098/System.Runtime.InteropServices.Tests/console.b67234d6.log?sv=2019-07-07&se=2021-08-02T01%3A18%3A16Z&sr=c&sp=rl&sig=AEuDxHzBeH%2Bdg1mQ0137%2FiE1DPsgVpAbFF7TM3JZl40%3D'
Generated log list: console.b67234d6.log:
  https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-55430-merge-18140ce380bd48d098/System.Runtime.InteropServices.Tests/console.b67234d6.log?sv=2019-07-07&se=2021-08-02T01%3A18%3A16Z&sr=c&sp=rl&sig=AEuDxHzBeH%2Bdg1mQ0137%2FiE1DPsgVpAbFF7TM3JZl40%3D

Searching '/private/tmp/helix/working/A2B508F9/w/B04809E0/e' for test results files
Found results file /private/tmp/helix/working/A2B508F9/w/B04809E0/e/testResults.xml with format xunit
Reporting has failed.  Running mitigation for https://github.com/dotnet/arcade/issues/7371
Searching '/private/tmp/helix/working/A2B508F9/w/B04809E0/e' for test results files
Found results file /private/tmp/helix/working/A2B508F9/w/B04809E0/e/testResults.xml 
Searching '/tmp/helix/working/A2B508F9/w/B04809E0/uploads' for test results files
Reporter script has failed, but XUnit test results show no failures.

steveharter · 2021-07-13T17:57:19Z

Test failures unrelated:

System.Net.Http.WinHttpHandler.Functional.Tests 
Test System.Net.Quic.Tests.MsQuicTests.ByteMixingOrNativeAVE_MinimalFailingTest has failed
System.Net.Requests.Tests

ghost · 2021-07-13T17:58:43Z

Tagging @dotnet/compat for awareness of the breaking change.

ericstj · 2021-07-13T17:59:53Z

Marking this as breaking since it's a break from previous preview (which is acceptable, but should be messaged). Please fill out the template here: https://github.com/dotnet/docs/issues/new?assignees=&labels=&template=dotnet-breaking-change.md&title=

steveharter · 2021-07-13T18:55:20Z

Breaking change issue: dotnet/docs#25105

steveharter added 2 commits July 9, 2021 14:08

Remove support for C# dynamic from JsonNode

523e132

Change converter to be based on JsonNode instead of System.Object

07e7985

steveharter added the area-System.Text.Json label Jul 9, 2021

steveharter requested review from ericstj, layomia and terrajobst July 9, 2021 20:00

steveharter self-assigned this Jul 9, 2021

steveharter requested review from eiriktsarpalis and jozkee as code owners July 9, 2021 20:00

ghost added the new-api-needs-documentation label Jul 9, 2021

stephentoub reviewed Jul 9, 2021

View reviewed changes

src/libraries/System.Text.Json/src/System.Text.Json.csproj Show resolved Hide resolved

stephentoub reviewed Jul 9, 2021

View reviewed changes

src/libraries/System.Text.Json/ref/System.Text.Json.cs Show resolved Hide resolved

Remove System.Linq.Expressions assembly references

dbed112

eiriktsarpalis reviewed Jul 12, 2021

View reviewed changes

src/libraries/System.Text.Json/tests/System.Text.Json.Tests/JsonNode/JsonArrayTests.cs Show resolved Hide resolved

eiriktsarpalis approved these changes Jul 12, 2021

View reviewed changes

steveharter added this to the 6.0.0 milestone Jul 12, 2021

eerhardt reviewed Jul 12, 2021

View reviewed changes

src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonNode.Dynamic.cs Show resolved Hide resolved

steveharter added 3 commits July 12, 2021 15:02

Attempt to fix trim test errors

1d778b2

Attempt to fix trim test errors

d31cc03

Remove linker file

c92af76

runfoapp bot mentioned this pull request Jul 13, 2021

system.linq.expressions.tests work item #55546

Closed

layomia reviewed Jul 13, 2021

View reviewed changes

Disable failing test

9c0e474

steveharter merged commit 04429ca into dotnet:main Jul 13, 2021

steveharter deleted the RemoveDynamic branch July 13, 2021 17:57

ericstj added the breaking-change Issue or PR that represents a breaking API or functional change over a previous release. label Jul 13, 2021

ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jul 13, 2021

runfoapp bot mentioned this pull request Jul 13, 2021

system.net.quic.tests.msquictests.bytemixingornativeave_minimalfailingtest times out on Fedora #55588

Closed

runfoapp bot mentioned this pull request Jul 15, 2021

System.Net.Requests.Tests failing with a crash #55714

Closed

ManickaP mentioned this pull request Jul 20, 2021

[QUIC] Remove AppContext switch from S.N.Quic #56027

Merged

karelz mentioned this pull request Jul 28, 2021

[WinHttpHandler] Long running test: ResponseHeadersRead_SynchronizationContextNotUsedByHandler on Win7/Win8 #54034

Closed

ghost locked as resolved and limited conversation to collaborators Aug 12, 2021

ericstj removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove C# dynamic support from JsonNode#55430

Remove C# dynamic support from JsonNode#55430
steveharter merged 7 commits intodotnet:mainfrom
steveharter:RemoveDynamic

steveharter commented Jul 9, 2021

Uh oh!

ghost commented Jul 9, 2021

Uh oh!

ghost commented Jul 9, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

layomia Jul 13, 2021

Uh oh!

eerhardt Jul 13, 2021

Uh oh!

steveharter commented Jul 13, 2021

Uh oh!

steveharter commented Jul 13, 2021

Uh oh!

ghost commented Jul 13, 2021

Uh oh!

ericstj commented Jul 13, 2021 •

edited

Loading

Uh oh!

steveharter commented Jul 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

steveharter commented Jul 9, 2021

Uh oh!

ghost commented Jul 9, 2021

Uh oh!

ghost commented Jul 9, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

layomia Jul 13, 2021

Choose a reason for hiding this comment

Uh oh!

eerhardt Jul 13, 2021

Choose a reason for hiding this comment

Uh oh!

steveharter commented Jul 13, 2021

Uh oh!

steveharter commented Jul 13, 2021

Uh oh!

ghost commented Jul 13, 2021

Uh oh!

ericstj commented Jul 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steveharter commented Jul 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ericstj commented Jul 13, 2021 •

edited

Loading