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

Support C# 8 nullable reference types #15499

Merged
merged 2 commits into from
May 6, 2019
Merged

Conversation

roji
Copy link
Member

@roji roji commented Apr 25, 2019

This introduces two new conventions - mirroring the existing RequiredPropertyAttributeConvention and RequiredNavigationAttributeConvention - which detect C# 8 non-nullable properties and treat them in the same way as [Required]. Some remarks:

  • As things currently stand, the compiler synthesizes the NullableAttribute type used to indicate nullability into the assemblies it compiles. That means we need recognize it by name, use reflection to access its contents, and be prepared for multiple NullableAttribute types in different assemblies. I've done some caching around this to keep it efficient, things may get simplified by RTM-time.
  • The non-nullable navigation convention emits the same two log messages as the [Required] one (NonNullableReferenceOnBothNavigations, NonNullableOnDependent), but there may be a case for not doing so. Putting [Required] somewhere isn't the same as not defining a property nullable with a question mark.
  • The second commit fixes what seems to been an issue - The RequiredNavigationAttributeConvention did not ignore collection properties, so if a collection navigation with [Required] had an inverse navigation with [Required] as well, RequiredAttributeOnBothNavigations would be logged. The test RequiredAttribute_does_not_set_is_required_for_collection_navigation seems to have had the navigations switched around, so I changed that (hopefully I haven't misunderstood).
  • NonNullableNavigationConvention currently duplicates code from RequiredNavigationAttributeConvention. If this seem unreasonable I can refactor to deduplicate it somehow.

Closes #10347
Replaces #14983

@roji
Copy link
Member Author

roji commented May 5, 2019

Made the changes. @AndriySvyryd do you mind giving this another quick look, especially the changes on handling of collection navigations?

@roji roji merged commit d02f1aa into dotnet:master May 6, 2019
@roji roji deleted the ModelNullability branch May 6, 2019 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support C# 8.0 nullable reference types in model building
4 participants