made ActorCell use nullable
#7475
Conversation
|
I've also cleaned up A TON of |
There was a problem hiding this comment.
API approval file - you should still look over it but there's nothing super scary here. Going to comment on the changes in the source files themselves.
|
|
||
| private ImmutableDictionary<string, FunctionRef> FunctionRefs => Volatile.Read(ref _functionRefsDoNotCallMeDirectly); | ||
| internal bool TryGetFunctionRef(string name, out FunctionRef functionRef) => | ||
| internal bool TryGetFunctionRef(string name, out FunctionRef? functionRef) => |
There was a problem hiding this comment.
All of these TryGetValue<TKey, TValue> methods have a signature of bool TryGetValue<TKey, TValue>(TKey key, out TValue? val)in the .NET standard libraries - so I've brought theTryGetFunctionRef` up to speed.
There was a problem hiding this comment.
So a really useful over-arching point for all of these code changes in the review:
We're not changing what is actually returned - we're just accurately annotating it going forward.
Please bear that in mind - only input behavior has changed (i.e. explicitly handling null inputs where we weren't before), not any output behavior. Methods that could return null before could still return null, but it's just annotated now.
| /// <param name="actor">TBD</param> | ||
| /// <returns>TBD</returns> | ||
| public ChildRestartStats InitChild(IInternalActorRef actor) | ||
| public ChildRestartStats? InitChild(IInternalActorRef actor) |
There was a problem hiding this comment.
ChildRestartStats will be null if we don't have a child by this name - should never happen but it has always been theoretically possible
| /// Tries to get the stats for the child with the specified name. This ignores children for whom only names have been reserved. | ||
| /// </summary> | ||
| private bool TryGetChildRestartStatsByName(string name, out ChildRestartStats child) | ||
| private bool TryGetChildRestartStatsByName(string name, out ChildRestartStats? child) |
There was a problem hiding this comment.
Again, the TryGetValue pattern applied here
src/core/Akka/Actor/ActorCell.cs
Outdated
| /// Current message count inside the mailbox. | ||
| /// </summary> | ||
| public int NumberOfMessages { get { return Mailbox.NumberOfMessages; } } | ||
| public int NumberOfMessages { get { return Mailbox!.NumberOfMessages; } } |
There was a problem hiding this comment.
Mailbox can't be null by the time anyone calls this method - or else there's been a catastrophic failure.
|
|
||
| SwapMailbox(mailbox); | ||
| Mailbox.SetActor(this); | ||
| Mailbox!.SetActor(this); |
There was a problem hiding this comment.
Mailbox can't be null by the time anyone calls this method - or else there's been a catastrophic failure.
|
|
||
|
|
||
| Assert.Assert(instance != null, (string)(nameof(instance) + " != null")); | ||
| return instance!; |
There was a problem hiding this comment.
The assertion on the line above prevents this from ever actually returning null.
| { | ||
| var current = Current; | ||
| return current != null ? current.Sender : ActorRefs.NoSender; | ||
| return current?.Sender ?? ActorRefs.NoSender; |
There was a problem hiding this comment.
Since Current can be null and Sender can separately be null (this has always been a possibility), we return ActorRefs.NoSender here - which, ironically, is null right now.
| /// <param name="kvp">TBD</param> | ||
| /// <param name="index">TBD</param> | ||
| protected void ChildStatsAppender(StringBuilder sb, KeyValuePair<string, IChildStats> kvp, int index) | ||
| protected static void ChildStatsAppender(StringBuilder sb, KeyValuePair<string, IChildStats> kvp, int index) |
There was a problem hiding this comment.
Not a breaking change - moving to static would only change inheritors, not callers.
There was a problem hiding this comment.
(and there could be no inheritors of this method before.)
* Add `NotNullWhen` compiler annotation attribute to all of the `bool TryX(out Y)` patterned functions * Add assertions on all shebangs to make sure we document why the nullability check was overriden
| /// <returns>TBD</returns> | ||
| public static IEnumerable<T> Concat<T>(this IEnumerable<T> enumerable, T item) | ||
| #nullable enable | ||
| public static IEnumerable<T> Concat<T>(this IEnumerable<T>? enumerable, T item) |
| /// </summary> | ||
| public static readonly IActorRef NoSender = null; | ||
| #nullable enable | ||
| public static readonly IActorRef? NoSender = null; |
Changes
This PR looks scarier than it is. I've implemented
nullabilityon theActorCell, the brains underneath each actor, in order to support some future changes we're going to make here. None of this should result in a behavior change or a breaking API change - practically speaking, given that most of these APIs are not called directly from user code.Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):