Clean up, formalize, and enhance RunState

[dodexahedron]

Update:

Current plan is to:

• Make RunState a sealed internal record type.
• Remove IDisposable from it.
• Introduce an interface for it (IRunState) that is public and replace references to the explicit RunState type with the interface.
• Make the Toplevel property get-only, to enable covariance for implementers of the interface.
• Change the set accessor for Toplevel to an explicit SetRunState method
• (Open question) Implement INotifyProprtyChanged?
  • Re-implement IDisposable to handle event subscriber cleanup.
• (Open question) Implement IObservable<T>?
  • Re-implement IDisposable to handle cleanup of subscribed IObserver<RunState> instances from the subscriber list.

---

Orignal:

TL;DR: The Terminal.Gui.RunState type should be sealed and NOT implement IDisposable.

Why?

It references but is not necessarily the owner of an IDisposable object.

It doesn't own any unmanaged resources.

It doesn't do anything in disposal anyway except throw a potentially uncatchable exception (see below).

And the protected virtual modifier on the Dispose(bool) method is unnecessary, as well, since nothing derives from it.

And GC.SuppressFinalize should only get called once.
However, beyond that, the GC.SuppressFinalize is unnecessary for this class for a couple of reasons:

• Most importantly, the class does not have a finalizer¹, so it won't even go into the finalizer queue in the first place. That's all that method call avoids. It tells the GC that this object does not need the GC to call the overridden Object.Finalize() method (which, again, doesn't exist anyway) that a finalizer becomes. The finalizer's job is to call the non-public Dispose method with false, which is an indication that it was not called by code, but by the GC trying to eliminate the object once and for all and is the entire actual purpose of that bool parameter.
• Even if the class had a finalizer, the dispose methods still don't do anything useful. However, the protected one - which throws an exception, which is a Very Bad Thing™️ to do - will be called by the GC via the finalizer (or should be, for a correctly-implemented finalizer) if Dispose wasn't called on it when it became unreachable from root. That would mean it can crash the entire program completely unexpectedly and at an unpredictable time (or upon an explicit GC.Collect() call), simply because a RunState object had no path back to root any more.

There are caveats for derived types, too, but I don't think that's relevant here.

At least in its current state, I don't think it should be IDisposable at all. It certainly doesn't actually follow the pattern. I also think it should be sealed. If someone really wants to rip it out and replace it with their own, they can type forward to do so or define their own with the same name to override it.

Footnotes

1. Finalizers look like C++ destructors and have a sorta similar purpose, just managed-style. ↩

[dodexahedron]

As far as I can tell, the entire class, at present, can reduce down to this:

/// <summary>The execution state for a <see cref="Toplevel"/> view.</summary>
public sealed record RunState (Toplevel Toplevel) : IEqualityOperators<RunState, RunState, bool>
{
    /// <summary>The <see cref="Toplevel"/> belonging to this <see cref="RunState"/>.</summary>
    public Toplevel? Toplevel { get; internal set; } = Toplevel;
}

Honestly, though? I'd go one step farther and make it generic, for even better compile-time and JIT behavior/optimization possibilities, like this:

/// <summary>The execution state for a <see cref="Toplevel"/> view.</summary>
public sealed record RunState<T> (T? Toplevel) : IEqualityOperators<RunState<T>, RunState<T>, bool> where T : Toplevel
{
    /// <summary>The <see cref="Toplevel"/> belonging to this <see cref="RunState"/>.</summary>
    public T? Toplevel { get; internal set; } = Toplevel;
}

That requires small adjustments elsewhere to add the type parameters (pretty straightforward). I've got it half done already, since I was validating it all in code as I was reporting on it anyway.

[tig]

No reason other than me being stoopid, probably.

A PR would be appreciated.

[dodexahedron]

Bah, I'm sure it probably originally made sense and then just evolved away from it haha.

And yup - PR coming soonish. I backed up a little bit to revisit, with a more critical eye, the generic idea WRT this specific class.

Generic wouldn't be necessary if I change the property in that class to be get-only, with a method to change it instead of a set accessor.

Why?

That gets what I was going for in the first place (covariance, primarily), without needing to bother with generics. I'll do a quick PoC and see how that looks before continuing down the other path. It'll likely touch a lot fewer files, too, if I do it that way.

A read-write property requires an invariant type. Get-only allows covariant returns for overriding descendants, by virtue of them just being methods returning a value.

[dodexahedron]

Related to IDisposable, though...

So, RunState is an obvious type where change notification is potentially quite useful to anything using it...

Events (such as from INotifyPropertyChanged) and the observer pattern (using System.IObserver<T>¹ and System.IObservable<T>² are both non-exclusive options for that. Each of those brings back potential need for keeping IDisposable, to aid in proper cleanup for avoiding memory leaks and difficult-to-diagnose exceptions in cases where consumers fail to properly unsubscribe before ditching a subscriber/observer object instance.

If I were to implement either or both of those mechanisms, I'd put the IDisposable back in and just "do the needful" to be sure it does what it needs to do correctly.

Any thoughts/comments/questions/concerns/gripes/jeers about that?

Footnotes

1. IObserver<T> is contravariant on T. ↩

2. IObservable<T> is covariant on T. ↩

[dodexahedron]

Closing this as done.