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

Code generated by Json source generator results in warnings in projects with nullable enabled #52227

Closed
pranavkm opened this issue May 4, 2021 · 3 comments · Fixed by #57178
Assignees
Milestone

Comments

@pranavkm
Copy link
Contributor

pranavkm commented May 4, 2021

I think it's an issue with the nullability annotations on the getters and setters for JsonMetadataServices.CreatePropertyInfo

These should be:

- Func<object, T>? getter,
+ Func<object, T?>? getter,

etc

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels May 4, 2021
@ghost
Copy link

ghost commented May 4, 2021

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

Issue Details

I think it's an issue with the nullability annotations on the getters and setters for JsonMetadataServices.CreatePropertyInfo

These should be:

- Func<object, T>? getter,
+ Func<object, T?>? getter,

etc

Author: pranavkm
Assignees: -
Labels:

area-System.Text.Json, untriaged

Milestone: -

@layomia layomia self-assigned this May 4, 2021
@layomia layomia removed the untriaged New issue has not been triaged by the area owner label May 4, 2021
@layomia layomia added this to the 6.0.0 milestone May 4, 2021
@Trolldemorted
Copy link

Aren't the warnings correct in this case, because System.Text.Json ignores the non-nullability?

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 10, 2021
@layomia
Copy link
Contributor

layomia commented Aug 10, 2021

Aren't the warnings correct in this case, because System.Text.Json ignores the non-nullability?

There's robust logic in STJ to handle null on serialization and deserialization, including to support features like ignoring null on serialization. Because of this we don't leak NullReferenceException and related bugs. However, it would be most correct to accept T? on deserialization and return T? on serialization, since property values could indeed be null. Unfortunately, I couldn't find a way to express this in a prototype to have the correct semantics, due error CS0453 given this code.

In the fix for the issue #57178, I left the parameter type as Func<object, T>? getter and just banged the return value of the getter call in the generated code.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 12, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Sep 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants