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

[release/5.0] Set default value for value-type ctor params properly (#43831) #45457

Merged
merged 1 commit into from
Dec 8, 2020

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Dec 2, 2020

Ports #43831 to 5.0
Fixes #43757 in 5.0

Description

Fixes bug where a NullReferenceException is thrown when (de)serializing a type whose constructor has default is set as optional argument default for a value-typed ctor parameter. For example, this type:

public record PositionalRecord(Guid st = default);

Customer Impact

Avoids a NullReferenceException in a potentially common scenario.

Regression

Not a regression since the deserialization-with-paramterized-ctor feature is new in 5.0.

Testing

Unit tests have been added to verify the expected behavior.

Risk

Little to none. This is a very small and scoped change to address this issue only.

* Set default value for value-type ctor params properly

* Address review feedback
Copy link
Member

@steveharter steveharter left a comment

Choose a reason for hiding this comment

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

Looks like a direct port minus adding extension method Type.IsNullableOfT

@layomia layomia added the Servicing-consider Issue for next servicing release review label Dec 7, 2020
@layomia layomia requested a review from ericstj December 7, 2020 22:36
@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Dec 8, 2020
@layomia layomia merged commit 6b6ed31 into dotnet:release/5.0 Dec 8, 2020
@layomia layomia deleted the CtorParamDefault_5.0 branch December 8, 2020 18:36
@HeshamMeneisi
Copy link

When is this fix going to be released?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants