Final wave of #nullable in Rx.

[bartdesmet]

The only files left with #nullable disable are:

• Map.cs, Lookup.cs, Grouping.cs due to their dependence on Dictionary<TKey, TValue> which we use with a non-null TKey.
  • These are due for substitution with a more efficient implementation (cf. System.Linq) anyway.
  • Alternatively, we can absorb the null case check here, and clean up the use sites a bit.
• Disposable.Utils.cs is a potpourri forest of ref methods which all would need ref IDisposable? parameters because of mixed use cases, e.g. the original public disposables which support null in various places (and have been annotated as such), as well as direct uses in operators where these helpers are even used in constructors, thus breaking definite assignment analysis to prove that fields are non-null after the constructor exits (i.e. because of ref rather than out with a non-null parameter).
  • This whole set of utilities should be revisited, ideally re-encapsulating the trivial use cases of SingleAssignment, MultipleAssignment, and Serial variants of disposables, for example in a struct-based helper. That'd also make the usage more clear, rather than have an operator implementation sprinkled with Disposable.XYZ methods. Right now, simply looking at the fields of an operator does not suffice to figure out the state transitions of disposables it contains. Furthermore, many use cases of these helpers ignore the return value.

[bartdesmet]

@danielcweber Is a revisit of Disposable.Utils.cs something you'd be interested in looking into? I do very much like the reduction of allocations this has brought (due to inlining of the disposable logic where possible), but dislike the amount of references to effectively global functions this has introduced throughout the code base.

I think there's a good middle ground by having struct-based implementations of the various disposables, which encapsulate the IDisposable? field, and use the helpers (or even better, encapsulate, them entirely in there). There would be no additional allocation (by virtue of being a struct), the disposable field would be strongly typed and thus self-descriptive, and the use of helpers throughout would be reduced (also cases where we call Try* and ignore the result can be optimized). A possible sketch:

// This can be inlined in operators and encapsulates the common use case (as it did originally in Rx).
internal struct SerialDisposableValue
{
  private IDisposable? _value;

  // Maybe a constructor if initial assignment is common.

  // Maybe an additional void Initialize(IDisposable?) for the initial assignment
  // iff it's decoupled from the instantiation of the struct value and iff it's clearly
  // more performant than an assignment to Disposable.

  public bool IsDisposed => Disposables.Disposable.GetIsDisposed(ref _current);

  public IDisposable? Disposable
  {
    get => Disposables.Disposable.GetValue(ref _current);

    // Can be optimized, we discard the result of TrySetSerial anyway.
    set => Disposables.Disposable.TrySetSerial(ref _current, value);
  }

  // Possibly another form of assignment given that we're commonly using TrySetSerial
  // and discard its return value. (4/31 use sites do use the value right now)
  // bool TryAssign(IDisposable value) => ...

  public void Dispose() => Disposables.Disposable.Dispose(ref _current);
}

public sealed class SerialDisposable : IDisposable
{
  private SerialDisposableValue _value;

  public bool IsDisposed => _value.IsDisposed;

  public IDisposable Disposable
  {
    get => _value.Disposable;
    set => _value.Disposable = value;
  }

  public void Dispose() => _value.Dispose();
}

Similar for SingleAssignment and MultipleAssignment. There are likely a few "mixed cases" where a single IDisposable field is used with a mix of helper methods, but from quick inspection, most of these are due to the initial assignment using [Try]SetSingle, which can be supported either in a constructor on the value type, or some Initialize method. My guess is that there are only a handful of cases where there's legitimate mixed use of serial/single/multiple operations on a single disposable instance, and this change would make them also stand out more clearly (as atypical patterns that warrant extra scrutinty when making changes).

Current stats:

• GetValue - 1 reference
• GetValueOrDefault - 3 references
• SetSingle - 48 references
• TrySetSingle - 22 references
• TrySetMultiple - 16 references
• TrySetSerial - 31 references
• GetIsDisposed - 18 references
• Dispose - 111 references

With the proper (re)encapsulation, I'd think all of these would drop to single-digit counts, or vanish entirely behind the struct wrapper.

[danielcweber]

@bartdesmet This mention went by completely unnoticed by me, sorry. I may have a look.

The reason I revisited this PR is to have a look about the current state of nullability on the public API surface. More specifically, I ran into a bug in my code because e.g. FirstOrDefault does not expose nullability on the result type. What's its current state?

[bartdesmet]

@danielcweber The nullable annotations should be in a good shape now and be consistent with System.Linq. For example:

[return: MaybeNull]
public static TSource SingleOrDefault<TSource>(this IObservable<TSource> source);

and (see here):

[return: MaybeNull]
public static TSource FirstOrDefault<TSource>(this IEnumerable<TSource> source)

A relevant PR on the dotnet/runtime side is dotnet/runtime#179.

[bartdesmet]

Also relevant: dotnet/runtime#28086

[danielcweber]

Sorry, meant to say FirstOrDefaultAsync, and the signature should be

IObservable<TSource?> FirstOrDefaultAsync<TSource>(IObservable<TSource> source)

shouldn't it ?

[bartdesmet]

You're right. In fact, DefaultIfEmpty should also be addressed. Do you want to do a PR for these? I'm happy to tackle it as well. Just let me know.

The work has to be done in IObservable<T>, IAsyncEnumerable<T>, and the IQbservable<T> spaces.

[danielcweber]

Can do.

[danielcweber]

For the IQbservable stuff, the HomoIcon tool produces a lot of changes. I guess it hasn't been used for a long time but the generated files have been edited by hand. I could do so as well for the FirstOrDefaultAsync and DefaultIfEmpty methods or leave automatic generation to you.

[danielcweber]

Would love to do the work for Ix.Async but I can't build anything. I keep getting NETSDK1004 no matter how much I restore on any sdk (3.1, 5-preview)...

[bartdesmet]

For the IQbservable stuff, the HomoIcon tool produces a lot of changes. I guess it hasn't been used for a long time but the generated files have been edited by hand. I could do so as well for the FirstOrDefaultAsync and DefaultIfEmpty methods or leave automatic generation to you.

I'm fine with doing the hand-edits for now. I'm not sure when HomoIcon regressed; it may be a candidate for replacement (maybe using Roslyn source generators though it has its own challenges) and extension (by adding support to carry over nullable annotations).

There is a test case as well that checks that the operator surface for both IObservable<T> and IQbservable<T> is symmetric. It may be worth extending that test with checks for the nullable attributes to ensure symmetry at that level (besides the set of operators being symmetric). That can be done in a separate PR and is definitely easier than HomoIcon itself.

Would love to do the work for Ix.Async but I can't build anything.

Haven't seen that yet. Maybe @clairernovotny has some thoughts on what may be going on.

[danielcweber]

Got it running. It requires a separate restore on the *.Ref-projects (in subfolder refs).