Nullability Problems with Bind() calls

[Noggog]

Describe the bug

Existing instructions on how to set up XAML code behind bindings have a few usability issues due to the new nullability compiler features being added.

Steps To Reproduce

Let's take a 101 binding example inspired from the RxUI docs (https://www.reactiveui.net/docs/handbook/data-binding/windows-presentation-foundation):

// Setup run button
this.OneWayBind(this.ViewModel, vm => vm.RunAllCommand, view => view.RunButton.Command)
    .DisposeWith(disposable);

This should do the very simple job of binding a button's command to a viewmodel's command property.

Two problems come up with nullability.

1. OneWayBind's viewModel parameter is not null, while ReactiveUserControl's ViewModel member is. This means the user has to add the ! operator to get things to pass in, which is undesirable:

// Setup run button
this.OneWayBind(this.ViewModel!, vm => vm.RunAllCommand, view => view.RunButton.Command)
    .DisposeWith(disposable);

It's my understanding that ! should be avoided whenever possible, as it's bad coding habit that starts reintroducing routes for null ref errors into your code again.

2. OneWayBind returns a IReactiveBinding object that is nullable. This doesn't hook into DisposeWith cleanly, as it expects a non null item. So now the user has to add ? checks after every bind call, which is not too clean:

// Setup run button
this.OneWayBind(this.ViewModel!, vm => vm.RunAllCommand, view => view.RunButton.Command)
    ?.DisposeWith(disposable);

Since bindings like this are written -all- the time when developing a GUI, I think it would be good to set up the structure to not require any ! usage, for sure, and then potentially try to avoid the need for elvis operators, if possible.

Expected behavior

I would expect this code without any extra operators to function:

// Setup run button
this.OneWayBind(this.ViewModel, vm => vm.RunAllCommand, view => view.RunButton.Command)
    .DisposeWith(disposable);

For me, my suggested solutions would be to make the viewModel parameter on OneWayBind nullable, so that it can take ReactiveControl's ViewModel property directly and cleanly. Since the bind definitions return potentially null bindings already, just have a null viewmodel short circuit and return a null binding, perhaps? Seems like the cleanest route.

public IReactiveBinding<TView, TViewModel, TVProp> OneWayBind<TViewModel, TView, TVMProp, TVProp>(
        TViewModel? viewModel,
        TView view,
   ...

Second, since the binding calls are returning potentially null objects, it might be nice to upgrade DisposeWith to take nullable items, to remove the need for the ? call between the binding and the dispose every time.

[return: NotNullIfNotNull("item")]
public static T? DisposeWith<T>(this T? item, CompositeDisposable compositeDisposable)
    where T : class, IDisposable
{
    if (compositeDisposable == null)
    {
        throw new ArgumentNullException(nameof(compositeDisposable));
    }
    
    if (item == null) return item;

    compositeDisposable.Add(item);
    return item;
}

Perhaps the new class constraint isn't desirable and can be avoided, but this general idea is what I'm thinking.

[glennawatson]

! Is fine in this scenario since we aren't using the expression for member access. It's just to get around the compiler error.

[glennawatson]

You will often use ! In Rx coding.

Eg

observable.Where(x => x != null).Subscribe(x => x!.DoStuff())

[Noggog]

Yeah, I ran into that problem as well. I've been able to get around it in my codebase with this extension:

public static IObservable<T> NotNull<T>(this IObservable<T?> source)
    where T : class
{
    return source
        .Where(u => u != null)
        .Select(u => u!);
}

Would make your code look like

observable.NotNull().Subscribe(x => x.DoStuff());

[Noggog]

That sort of goes to my point that if we know the ! operator is okay, we should preferably internalize it so the users don't have to do it. Like in my extension method, I am safely making use of !, so that 100s of other use cases don't have to. I think we can do the same for the binding setups in RxUI

[glennawatson]

Yeah. I've avoided that one just due to additional invocation with the Select.

[Noggog]

True, there's a little bite on the optimization. I think it could be added to the Rx codebase as a single operator, though. A more official WhereNotNull that does it without chaining two calls together.

[glennawatson]

I'll play around with the nullables view model binding and see if it makes sense at least.

[Noggog]

Popped open an issue in the official Rx repo just to see what they think. dotnet/reactive#1262

I'll be toying around with the bindings more myself to get some more insight, too.

[glennawatson]

Just remember we need to support interfaced types so I think we are stuck with this

[Noggog]

#2468 Should solve the Bind problems

There still remains the problem that WhenAnyValue expressions all require a ! to get into the ViewModel.

I wonder if we could add some extension methods to ReactiveUserControl:

public IObservable<TResult> WhenVmValue<TViewModel, TResult>(this ReactiveUserControl<TViewModel> control, Expression<Func<TViewModel, TResult>> property1)
{
    if (control.ViewModel == null) 
    {
         return Observable.Empty<TResult>();
         // Or throw?
         // Again, the idea would be to take the burden off the user of trying to figure out what to do when the view model
         // is null, and internalizing that problem here and solving it once internally.
    }
    return control.ViewModel.WhenAnyValue(property1);
}

Then the end users could do this again:

this.WhenActivated(disposable =>
{
    this.WhenVmValue(x => x.RunAllCommand)
        .BindTo(this, view => view.RunButton.Command)
        .DisposeWith(disposable);
}

---

I wish it was as simple as encouraging users to do a simple if check:

this.WhenActivated(disposable =>
{
    if (this.ViewModel == null) return;
    this.WhenAnyValue(x => x.ViewModel.RunAllCommand)
        .BindTo(this, view => view.RunButton.Command)
        .DisposeWith(disposable);
}

But the expressions seem to remain unhappy. Presumably because an expression can theoretically be "applied" later on at a time when ViewModel is null again.

[glennawatson]

Hmm might be worth opening a new issue with the WhenAnyValue ones and keeping this one about the Bind() based operators.

[Noggog]

can do