Remove Jetbrains nullability attributes #24418
Conversation
roji
force-pushed
the
RemoveJetbrainsNullability
branch
from
462958a
to
a2eb46b
Compare
There was a problem hiding this comment.
@AndriySvyryd here are some other cases where I wasn't too sure. Do the below need to have [DisallowNull] on them? Or allow setting null?
EFCore.Relational/Metadata/Internal/TableBase.cs:101: public virtual SortedDictionary<IEntityType, IEnumerable<IForeignKey>>? RowInternalForeignKeys { get; [param: NotNull] set; }
EFCore.Relational/Metadata/Internal/TableBase.cs:121: public virtual Dictionary<IEntityType, bool>? OptionalEntityTypes { get; [param: NotNull] set; }
EFCore/Metadata/SlimForeignKey.cs:84: private SlimNavigation? DependentToPrincipal { get; [param: NotNull] set; }
EFCore/Metadata/SlimForeignKey.cs:85: private SlimNavigation? PrincipalToDependent { get; [param: NotNull] set; }
EFCore/Metadata/SlimSkipNavigation.cs:92: public virtual SlimSkipNavigation? Inverse { get; [param: NotNull] set; }
| @@ -427,7 +427,7 @@ public virtual EntityFrameworkServicesBuilder TryAdd<TService>([NotNull] Func<IS | |||
| /// <typeparam name="TService"> The contract for the service. </typeparam> | |||
| /// <param name="implementation"> The implementation of the service. </param> | |||
| /// <returns> This builder, such that further calls can be chained. </returns> | |||
| public virtual EntityFrameworkServicesBuilder TryAdd<TService>([CanBeNull] TService implementation) | |||
| public virtual EntityFrameworkServicesBuilder TryAdd<TService>([NotNull] TService implementation) | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
However this would be a breaking change
There was a problem hiding this comment.
This is a bit of a weird case. There is only one case where null was allowed:
- The service is considered a singleton
- An instance of this service type has already been added
For this case, the null instance was simply ignored.
There was a problem hiding this comment.
However this would be a breaking change
Technically yes... though it's very hard to see how anyone could have been using the API with null in a productive way - it would only mean that we'd start throwing on the case @sharwell described above (ignore already-added singleton).
Am leaving as-is with the change.
They need [DisallowNull] |
| @@ -31,7 +30,7 @@ public class ObservableBackedBindingList<T> : SortableBindingList<T> | |||
| /// any release. You should only use it directly in your code with extreme caution and knowing that | |||
| /// doing so can result in application failures when updating to a new Entity Framework Core release. | |||
| /// </summary> | |||
| public ObservableBackedBindingList([NotNull] ICollection<T> observableCollection) | |||
| public ObservableBackedBindingList(ICollection<T> observableCollection) | |||
There was a problem hiding this comment.
❔ Did you also validate that all changes occurred in contexts where nullable reference types is enabled?
There was a problem hiding this comment.
I'm not sure what you mean here... Can you provide more detail?
There was a problem hiding this comment.
@sharwell as this is the only unresolved comment, I'm going to go ahead and merge this PR, as the conflict potential with other work is very high. But I'm happy to fix up in a later PR if we need to.
|
Thanks for the review and helpful comments @sharwell! |
roji
force-pushed
the
RemoveJetbrainsNullability
branch
from
a2eb46b
to
809fe7a
Compare
The first commit corrects a few discrepancies between Jetbrains and the Roslyn nullability and the Roslyn nullability annotations - this is worth going over. The second is mechanical and removes the Jetbrains stuff.
Just for fun, discrepancies were discovered as follows:
\[CanBeNull\][^?,)]*[,)]\[NotNull\][^,)]*\?grep -nr '\[param: CanBeNull\]' | grep -v \?.Fixes #14568