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

Fix nullable annotation for Validator.TryValidateValue and ValidateValue #91286

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

jeffhandley
Copy link
Member

Fixes #91162

There was an erroneous comment in the Validator TryValidateValue and ValidateValue methods indicating that the value could not be null. This comment led to incorrect nullable annotations being applied. Propagation of that surfaced in the new Options source generator.

This removes the erroneous comment, updates the annotation to be more relaxed (non-breaking), and removes the ! from all call sites in the repo. The intent is to backport this fix to .NET 8 RC2.

@ghost
Copy link

ghost commented Aug 29, 2023

Tagging subscribers to this area: @dotnet/area-system-componentmodel-dataannotations
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #91162

There was an erroneous comment in the Validator TryValidateValue and ValidateValue methods indicating that the value could not be null. This comment led to incorrect nullable annotations being applied. Propagation of that surfaced in the new Options source generator.

This removes the erroneous comment, updates the annotation to be more relaxed (non-breaking), and removes the ! from all call sites in the repo. The intent is to backport this fix to .NET 8 RC2.

Author: jeffhandley
Assignees: jeffhandley
Labels:

area-System.ComponentModel.DataAnnotations

Milestone: -

Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

LGTM.

Are we planning to backport this one to 8.0 release? It makes sense to port it. I am preparing some other changes in the source gen which we'll need to port too. just FYI for now.

@dotnet-issue-labeler

This comment was marked as resolved.

@jeffhandley jeffhandley merged commit f007d88 into dotnet:main Aug 29, 2023
@jeffhandley jeffhandley deleted the jeffhandley/tryvalidatevalue branch August 29, 2023 20:37
@jeffhandley
Copy link
Member Author

/backport to release/8.0

@github-actions
Copy link
Contributor

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6017311556

@stephentoub
Copy link
Member

stephentoub commented Aug 29, 2023

Do we have tests that validate everything compiles without warning/error with the options generator on .NET 6/7?

If it works, great, we should backport the change.

If not, I think we'll need to undo (or refine) the options generator change, so that it still uses ! when the parameters are non-nullable in the target reference assembly.

@jeffhandley
Copy link
Member Author

Thanks for calling that out @stephentoub! I chatted with @tarekgh and I will push another PR that reverts the source generator references. He's going to consider if we could/should have the source generator be aware of this annotation difference.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect nullable annotation on Validator.TryValidateValue's value?
3 participants