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.

Sign up for GitHub

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

Jump to bottom

Remove C# dynamic support from JsonNode #55430

Merged
merged 7 commits into from
Jul 13, 2021
Merged

Remove C# dynamic support from JsonNode #55430

merged 7 commits into from
Jul 13, 2021

Conversation

steveharter
Copy link
Member

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.

@steveharter steveharter self-assigned this Jul 9, 2021
@ghost
Copy link

ghost commented Jul 9, 2021

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: -

@dotnet-issue-labeler
Copy link

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.

@steveharter steveharter added this to the 6.0.0 milestone Jul 12, 2021
@runfoapp runfoapp bot mentioned this pull request Jul 13, 2021
Closed
layomia
layomia reviewed Jul 13, 2021
View reviewed changes
...stem.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/StackOfTConverter.cs Outdated
@@ -14,6 +15,7 @@ protected override void Add(in TElement value, ref ReadStack state)
((TCollection)state.Current.ReturnValue!).Push(value);
}

[DynamicDependency(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor, typeof(Stack<>))]
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member Author

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
Copy link
Member Author

Test failures unrelated:

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

@steveharter steveharter merged commit 04429ca into dotnet:main Jul 13, 2021
@steveharter steveharter deleted the RemoveDynamic branch July 13, 2021 17:57
@ericstj ericstj added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Jul 13, 2021
@ghost 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
@ghost
Copy link

ghost commented Jul 13, 2021

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

@ericstj
Copy link
Member

ericstj commented Jul 13, 2021 •
edited
Loading

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=

@runfoapp runfoapp bot mentioned this pull request Jul 13, 2021
Closed
@steveharter
Copy link
Member Author

Breaking change issue: dotnet/docs#25105

@runfoapp runfoapp bot mentioned this pull request Jul 15, 2021
Closed
@ManickaP ManickaP mentioned this pull request Jul 20, 2021
Merged
@karelz karelz mentioned this pull request Jul 28, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Aug 12, 2021
@ericstj 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub left review comments

@eerhardt eerhardt eerhardt left review comments

@layomia layomia layomia left review comments

@eiriktsarpalis eiriktsarpalis eiriktsarpalis approved these changes

@terrajobst terrajobst Awaiting requested review from terrajobst

@ericstj ericstj Awaiting requested review from ericstj

@jozkee jozkee Awaiting requested review from jozkee

Assignees

@steveharter steveharter

Labels
area-System.Text.Json breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. new-api-needs-documentation
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@steveharter @ericstj @stephentoub @eiriktsarpalis @eerhardt @layomia