Skip to content

Fix nullable detection #12202

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

Merged
merged 2 commits into from
Jul 17, 2019
Merged

Fix nullable detection #12202

merged 2 commits into from
Jul 17, 2019

Conversation

rynowak
Copy link
Member

@rynowak rynowak commented Jul 15, 2019

Fixes: #11828 and #11813

@rynowak rynowak requested review from pranavkm and roji July 15, 2019 19:53
@Pilchie Pilchie added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jul 15, 2019
Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

LGTM, two comments:

  • Not sure if model processing speed is an issue, but you may want to look at caching context nullability at the type and module level, to avoid doing a lot of reflection over and over again. In EF Core we've seen some insane prolific models where this could be an issue.
  • I can see the comment about not handling generics and NNRT, but we may want to consider identifying pre- and post-conditions such as [MaybeNull] (fully explained here).

@rynowak
Copy link
Member Author

rynowak commented Jul 16, 2019

Thanks @roji

I'm not immediately concerned about intermediate caching, the results of this will all be cached as well.

I can see the comment about not handling generics and NNRT, but we may want to consider identifying pre- and post-conditions such as [MaybeNull]

Are these types actually defined anywhere? I keep seeing design notes, but I don't see these types in use anywhere.

@rynowak rynowak force-pushed the rynowak/nullability branch from 00b152e to 72b778a Compare July 16, 2019 20:09
@rynowak rynowak force-pushed the rynowak/nullability branch from 72b778a to 3fab972 Compare July 16, 2019 22:28
@rynowak rynowak merged commit 4ba66f0 into master Jul 17, 2019
@ghost ghost deleted the rynowak/nullability branch July 17, 2019 02:58
@roji
Copy link
Member

roji commented Jul 17, 2019

Are these types actually defined anywhere? I keep seeing design notes, but I don't see these types in use anywhere.

Take a look in corefx, they're used a lot there. BTW unlike NullableAttribute and NullableContextAttribute, these are actually defined in System.Diagnostics.CodeAnalysis (not sure why the different approach).

Note also https://github.com/dotnet/corefx/issues/36222#issuecomment-511973898 (things are going to change again a bit..).

@rynowak
Copy link
Member Author

rynowak commented Jul 17, 2019

It seems like most of the overlap here is with generic-typed fields and properties right?

@roji
Copy link
Member

roji commented Jul 18, 2019

I think so, but there are also some other cases. For example, you may want to have a property that can be set to null but will never return null (e.g. because it will return some sort of default). Exactly how we interpret that in ASP.NET/EF is also not immediately obvious.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disabled Nullability tests
4 participants