Skip to content

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

Merged
layomia merged 1 commit intodotnet:release/5.0from
layomia:CtorParamDefault_5.0
Dec 8, 2020
Merged

[release/5.0] Set default value for value-type ctor params properly (#43831)#45457
layomia merged 1 commit intodotnet:release/5.0from
layomia:CtorParamDefault_5.0

Conversation

@layomia
Copy link
Copy Markdown
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
Copy Markdown
Contributor

@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
Copy Markdown

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