Annotate ChangeTracking for nullability #24261

smitpatel · 2021-02-24T19:55:14Z

Part of #19007

src/EFCore/ChangeTracking/Internal/CurrentProviderValueComparer`.cs

src/EFCore/ChangeTracking/Internal/IInternalEntityEntryFactory.cs

src/EFCore/ChangeTracking/ObservableHashSet.cs

src/EFCore/ChangeTracking/Internal/StateManager.cs

src/EFCore/ChangeTracking/Internal/SimpleNullableDependentKeyValueFactory.cs

src/EFCore/ChangeTracking/ChangeTracker.cs

src/EFCore/ChangeTracking/EntityEntryGraphNode.cs

src/EFCore/ChangeTracking/Internal/SimplePrincipalKeyValueFactory.cs

src/EFCore/ChangeTracking/Internal/ArrayPropertyValues.cs

src/EFCore/ChangeTracking/Internal/EntityReferenceMap.cs

src/EFCore/ChangeTracking/PropertyEntry`.cs

src/EFCore/ChangeTracking/PropertyValues.cs

roji · 2021-02-25T07:51:15Z

src/EFCore/ChangeTracking/PropertyValues.cs

@@ -158,7 +160,7 @@ public virtual IEntityType EntityType
        /// <param name="propertyName"> The property name. </param>
        /// <param name="value"> The property value if any. </param>
        /// <returns> True if the property exists, otherwise false. </returns>
-        public virtual bool TryGetValue<TValue>([NotNull] string propertyName, out TValue value)
+        public virtual bool TryGetValue<TValue>([NotNull] string propertyName, out TValue? value)


Probably [NotNullWhen(true)]

But the value can be null even when true

It's the same logic as with GetValue above (or with ReadShadowValue), i.e. whether the user calls TryGetValue<string> or TryGetValue<string?>`. We should make a global decision on that and apply everywhere.

I think this is still left, since we made the decision to have GetValue return TValue and not TValue?.

src/EFCore/ChangeTracking/ReferenceEntry`.cs

src/EFCore/Update/IUpdateEntry.cs

Part of #19007

Updated and re-requested review.

ghost · 2021-02-25T23:45:53Z

Hello @smitpatel!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost · 2021-02-25T23:54:31Z

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:

Azure Pipelines

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 this

From the bot dev team

We'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.

roji · 2021-02-26T08:54:49Z

src/EFCore/ChangeTracking/Internal/CompositePrincipalKeyValueFactory.cs

                {
-                    return null;
+                    return default!;


Shouldn't this method return object[]? (and possibly banged at relevant call sites)? Though as long as it's correct, it's just a private function so no big deal.

small nit: I (now) usually use default! only in generic contexts. In reference type contexts like this one, I tend to use null! for more clarity.

Long chain.
So basically this implements PrincipalKeyValueFactory<object[]> hence type becomes non-nullable. The reason for T = object[] because it is used as dependents map which uses dict underneath which cannot use nullable type as key. So while it is default! it shouldn't hit the code path here.

src/EFCore/ChangeTracking/Internal/InternalEntityEntry.cs

src/EFCore/ChangeTracking/Internal/RelationshipsSnapshot.cs

roji · 2021-02-26T09:02:32Z

src/EFCore/ChangeTracking/Internal/RelationshipsSnapshot.cs

@@ -67,7 +69,7 @@ public void RemoveFromCollection(IPropertyBase propertyBase, object removedEntit
                var index = propertyBase.GetRelationshipIndex();
                if (index != -1)
                {
-                    ((HashSet<object>)_values[index])?.Remove(removedEntity);
+                    ((HashSet<object>)_values[index]!)?.Remove(removedEntity);


If you want the runtime check, remove the bang; if we know it should never be null, remove the question mark.

src/EFCore/ChangeTracking/Internal/RelationshipsSnapshot.cs

roji · 2021-02-26T09:10:23Z

src/EFCore/ChangeTracking/Internal/SidecarValues.cs

@@ -32,9 +34,9 @@ public bool TryGetValue(int index, out object value)
            }

            public T GetValue<T>(int index)
-                => IsEmpty ? default : _values.GetValue<T>(index);
+                => IsEmpty ? default! : _values.GetValue<T>(index);


This could be a case where it's more correct to return T?, since there's an additional Empty state. Is the user expected to know before calling this whether it's empty or not? If not, we probably want to force them to deal with the Empty state.

The way whole T of GetValue flows, this shouldn't be invoked with non-null type if it can be empty. It is same as we returning T from other places, if you ask for wrong nullability it will throw exception. default! throws exception for us. @ajcvickers - Thoughts?

src/EFCore/ChangeTracking/PropertyEntry`.cs

src/EFCore/ChangeTracking/EntityEntryGraphNode.cs

roji

Looks almost perfect, see comments below.

roji · 2021-02-26T09:43:42Z

May bad, forgot to remove auto-merge, sorry.

Following #24261