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

[WIP] Basic support for non-nullable references #14983

Closed
wants to merge 2 commits into from

Conversation

roji
Copy link
Member

@roji roji commented Mar 11, 2019

This WIP PR adds first support for taking the new C# 8.0 non-nullable references into account in model building. This adds convention logic that interprets non-nullable references in the same way as [Required] - for both regular and navigation properties.

Some notes:

  • No scaffolding work done yet, we still have to decide what our scaffolding strategy is for this.
  • The ConfigurationSource for non-nullable references is currently Convention, as non-nullability reference nullability seems weaker than [Required] or the fluent API, and should be overridden by them (there's a test for that).
  • For now I integrated the nullability support into the existing RequiredPropertyAttributeConvention and RequiredNavigationAttributeConvention (which I renamed to RequiredPropertyConvention and RequiredNavigationConvention respectively). I'm not sure how fine-grained we like our conventions to be, in theory it may be worth separating them in case somebody doesn't want the new nullability support. Also, we can consider removing NavigationAttributeNavigationConvention as it's no longer used by us - although users may still use it for their own custom attributes?
  • Some tests are missing (for non-nullable navigation properties) as well as log message changes.
  • Weirdly, the tests pass from the cmdline but Reference_nullability_sets_is_nullable_correctly(NullAwareNonNullable) fails in VS2019 - this is a result of seeing a [Nullable] version from before NullableAttribute constructor parameter should be stored in a field roslyn#30143 (VS is somehow stuck behind, not using the latest Roslyn or something)? @bricelam we briefly discussed this, if you feel like taking a quick look to make sure I'm not crazy....

@vslee
Copy link

vslee commented Apr 5, 2019

Since the Milestone of the Roslyn PR you referenced is 16.1.P1, it will show up in Visual Studio 2019 Update 1 Preview 1

@roji
Copy link
Member Author

roji commented Apr 5, 2019

@vslee thanks for the comment. There's still a bit of work before this can get merged, but hopefully that will happen quite soon.

@roji
Copy link
Member Author

roji commented Apr 25, 2019

Superceded by #15499

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.

2 participants