Annotate metadata for nullability, leaving only builders and conventions#23381
Annotate metadata for nullability, leaving only builders and conventions#2338111 commits merged intomainfrom
Conversation
| private Navigation AddNavigation(MemberIdentity navigationMember, ForeignKey foreignKey, bool pointsToPrincipal) | ||
| { | ||
| var name = navigationMember.Name; | ||
| var name = navigationMember.Name!; |
There was a problem hiding this comment.
I think so... This private AddNavigation gets called from two sites, where a non-None MemberIdentity is constructed... Do you see a problem?
We may want to think about the design of MemberIdentity to make it friendlier...
There was a problem hiding this comment.
Depends on caller. But if we are using just name (rather than memberInfo from MemberIdentity then string parameter could have just worked too.
There was a problem hiding this comment.
MemberIdentity.Name should be non-nullable
There was a problem hiding this comment.
At least theoretically there's the None value of MemberIdentity. Should Name be non-nullable and just assert for that case?
There was a problem hiding this comment.
- Add
IsNoneto replaceMemberIdentity.Name == nullchecks
And either:
- Make
MemberIdentity.Namenon-nullable and throw if it's called onNone
Or:
- Annotate
IsNoneand call it before callingMemberIdentity.Name
I think the first option requires less code changes
There was a problem hiding this comment.
OK, am doing the first option.
There was a problem hiding this comment.
@AndriySvyryd I took a look at this as the last nullability leftover.
First, there seem to be a lot of cases (or enough) where MemberIdentity.Name is accessed and null really is expected. If we make Name throw for null, that means that all those call sites have to be changed to identity.IsNone() ? null : identity.Name, which needlessly complicates the code (this is option 1 above). This needs to be weighed against the gain in removing bangs in sites where we know the identity isn't empty (and don't even need/want to check) - I think it may not be worth it.
We could do option 2 and annotated IsNone while leaving Name nullable, but there seems to be little value in doing that - callers can just as well compare Name to null, instead of checking IsNone.
So I think the best thing may be to leave the code as-is - but let me know what you prefer and I'll do it.
Not totally related, but depending on the above we can also obsolete IsNone, and either introduce HasValue (property instead of method like IsNone, and also positive instead of negative), or better yet, do nothing at all - Name can just be checked directly.
There was a problem hiding this comment.
Sure, removing IsNone is fine, even if it means leaving the bangs in
| { | ||
| private static readonly MethodInfo _loadMethod = typeof(ILazyLoader).GetMethod(nameof(ILazyLoader.Load)); | ||
| private static readonly MethodInfo _loadAsyncMethod = typeof(ILazyLoader).GetMethod(nameof(ILazyLoader.LoadAsync)); | ||
| private static readonly MethodInfo _loadMethod = typeof(ILazyLoader).GetMethod(nameof(ILazyLoader.Load))!; |
There was a problem hiding this comment.
So... are we removing GetRequiredMethod in the end?
There was a problem hiding this comment.
I can neither confirm nor deny.
There was a problem hiding this comment.
So many words to just write NCND
| ? foreignKey.PrincipalEntityType | ||
| : foreignKey.PrincipalKey.DeclaringEntityType; | ||
| var principalTable = StoreObjectIdentifier.Create(principalType, StoreObjectType.Table); | ||
| var principalTable = StoreObjectIdentifier.Create(principalType, StoreObjectType.Table)!; |
There was a problem hiding this comment.
Bug alert: this and below should use storeObject.StoreObjectType instead of StoreObjectType.Table. Also if principalTable or duplicatePrincipalTable is null this should throw (or return false), basically the same as when principalColumns is null
There was a problem hiding this comment.
FWIW the two current call sites for this seem to always pass Table, so this shouldn't cause any trouble currently.
Fixed the above, hopefully as you wanted - if not let me know and I'll correct in a future PR.
There was a problem hiding this comment.
It would be better to move the
principalTable is null
|| duplicatePrincipalTable is nullcondition to the next if
There was a problem hiding this comment.
No problem, I will do that in the next PR.
| return dbFunction.Schema is null | ||
| ? _sqlExpressionFactory.Function( | ||
| dbFunction.Name, | ||
| arguments, | ||
| dbFunction.IsNullable, | ||
| argumentsPropagateNullability, | ||
| method.ReturnType.UnwrapNullableType()) | ||
| : _sqlExpressionFactory.Function( | ||
| dbFunction.Schema, | ||
| dbFunction.Name, | ||
| arguments, | ||
| dbFunction.IsNullable, | ||
| argumentsPropagateNullability, | ||
| method.ReturnType.UnwrapNullableType()); |
There was a problem hiding this comment.
We don't need 2 different calls. 2nd Function allows nullable schema as parameter. Please revert this change.
There was a problem hiding this comment.
It is not annotated in this way 
The implementation also has a Check.NotNull on the schema... Should this be changed?
There was a problem hiding this comment.
@smitpatel am merging as-is for now, we can revisit this afterwards and improve.
|
Hello @roji! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:
These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check. Give feedback on thisFrom the bot dev teamWe've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments. Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin. |
ghost
merged commit
| var skipNavigation = memberIdentity.MemberInfo != null | ||
| ? entityType.FindSkipNavigation(memberIdentity.MemberInfo) | ||
| : entityType.FindSkipNavigation(memberIdentity.Name); | ||
| : memberIdentity.Name is not null |
There was a problem hiding this comment.
Not to worry, I will revert this in the next PR.
3 participants
Here's another bucket of nullability. After this, all of metadata is annotated (core, relational, providers) except for builders, conventions and some remaining internal stuff (which I will do later). I've left some nullability warnings where I think a closer look is warranted.
Feel free to push commits directly to this branch, or I can do corrections if you tell me to.