Annotate Storage for nullability

[roji]

Since @AndriySvyryd and @smitpatel seemed to enjoy reviewing previous nullability PRs so much, I thought I'd give @ajcvickers the same joy - there's a commit in here just for type mapping (including converters) which can be reviewed separately.

Includes includes some leftovers from #23381 and #23376

/cc @smitpatel for some minor query things

[roji]

/cc @smitpatel

[smitpatel]

Some mappings are required. Though it does not have to map to a bool on server, can be mapped using a value converter too, we don't care.
We should bang this and perhaps add a validation code somewhere in type mapper
@ajcvickers ?

[smitpatel]

Discussed with Arthur about this. This mapping is crucial for query to work even for simple predicate kind of scenario (since they are of bool type). By default providers don't really need bool in database. They just need some support for bool even through value conversion. Also built-in we provide bool to string & bool to int conversion in the box. So it should just work in 99.99% cases. It may cause a failure for an ORM written by amoeba targeting some database but if neither of that possible than it would be probably write only database.

[roji]

@ajcvickers discussed this situation offline, am leaving the code as is and adding bangs, until we revisit as part of #13850

[smitpatel]

Wouldn't null! throw runtime error?

[smitpatel]

And if that code path is not being triggered then is the null check here necessary?

[roji]

/cc @bricelam on the one hand, we annotated SpatialConverter as [CanBeNull] in the ctor, but on the other we required it here (Npgsql, which doesn't require a converter, passes NullValueConverter currently). This PR changes to really not require a converter, or we could annotate SpatialConverter as required if we want.

[smitpatel]

Also see #21558

[smitpatel]

Remove these 2 too.
TypeMapping is never null after translation.
ColumnExpression always originates from a table which has alias.

[roji]

TypeMapping is never null after translation.

That's the point behind this assertion: unlike with TableExpression.Alias which really is never null (so it can be annotated as such and no assertion needed), SqlParameterExpression.TypeMapping is null in some cases.

But as you seem to really hate assertions I'll remove and bang instead.

BTW if we want to, we can consider making SqlExpression.TypeMapping non-nullable, and provide a separate nullable property for the type mapping to be used only during translation. This would allow us to remove the bangs when accessing TypeMapping from everywhere.

[smitpatel]

My issue is not with assertions in general. My issue is assertion in the wrong places just because it suits NRT. If the TypeMapping is null after translation in error then there are plenty of visitors which go through it and assuming it is going to be there. SqlGenerator is very much last phase of query pipeline. If you want to assert that TypeMapping is non-null after translation then this is certainly not the place to assert it.
So please don't put assert in wrong places in query.

[smitpatel]

cc: @AndriySvyryd

[roji]

I don't have super strong feelings here... But given that TypeMapping is nullable (because used during translation), any visitor after translation accessing it is assuming it will be non-null, which is fine. And I tend to codify/document coding assumptions via assertions - it's logically the exact same thing as a bang, just more explicit. Assertions also seem particularly well-suited to me when they codify assumptions on method parameters (exactly like Check.NotNull), and they concentrate the assumptions at the beginning rather than having bangs interspersed later in the method code.

[smitpatel]

Codify assumptions but not in wrong place.

[smitpatel]

I am still unsure what is happening with value conversions

• We are saying that null is handled already when calling into expression then why do we need additional null check?
• Is null! is obviously wrong? How is it different than calling null.Method()? So what additional value the null check provided.

While the places has been marked with #13850

• That issue enables that null value can be converted to non-null in database, basically skipping our first check that null value converts to null value.
• But that non-null converted value would need to come from user, no? By default we can just convert null to null in our built-in value conversions.
• Even if we decide to change our default conversions to identify more types, we cannot change the existing ones fully at all without causing a data break, since by default earlier we stored null values in database and now we would be storing altogether a different value.

[roji]

@smitpatel re value conversions, I briefly discussed with @ajcvickers offline and we just decided to defer this for now until #13850 is tackled - so I did the minimum to suppress warnings etc., without worrying right now about what #13850 will do.

We are saying that null is handled already when calling into expression then why do we need additional null check?

It does seem that currently we have double null-checking: once in the core value conversion infrastructure (e.g. SanitizeConverter), and once in each built-in value converter implementation we have.

Is null! is obviously wrong? How is it different than calling null.Method()? So what additional value the null check provided.

null! isn't wrong in the same sense as null.Method(), since the latter always fails with an NRE, while the former simply preserves our current runtime behavior, exactly as it is (that was the idea behind what I currently did). But down the road we should annotate better depending on what happens with #13850.

[roji]

@AndriySvyryd @smitpatel coming back to this after a while... I squashed and rebased over latest main, and pulled out the MemberIdentity stuff (fixup to #23381) which requires a bit more looking into (noted in #19007).