Windowscommunitytoolkit: RelayCommand<T> throws NullReferenceException when T is a ValueType and CanExecute is used

[sonnemaf]

Describe the bug

If you open a View (Page or Window) and you have DataBound the Command of a Button to a RelayCommand<T> where T is a ValueType and the command has a CanExecute action you get a NullReferenceException. Tested it in a UWP and in a WPF app. Both have this problem.

Without the CanExecute it works ok. If T is a reference type it also works ok.

Steps to Reproduce

Take this xaml

<Page
    x:Class="App19.MainPage"
    xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
    xmlns:local="using:App19"
    xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
    xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
    mc:Ignorable="d"
    Background="{ThemeResource ApplicationPageBackgroundThemeBrush}">

    <Grid RowSpacing="8" Padding="8">
        <Grid.RowDefinitions>
            <RowDefinition Height="Auto" />
            <RowDefinition Height="1*" />
        </Grid.RowDefinitions>
        <Button Content="Test"
                Command="{x:Bind TestCommand, Mode=OneTime}"
                CommandParameter="{x:Bind listViewDemo.SelectedIndex, Mode=OneWay}"/>
        <ListView x:Name="listViewDemo">
            <ListViewItem Content="A" />
            <ListViewItem Content="B" />
            <ListViewItem Content="C" />
        </ListView>
    </Grid>
</Page>

And this codebehind

using Microsoft.Toolkit.Mvvm.Input;
using Windows.UI.Popups;
using Windows.UI.Xaml.Controls;

// The Blank Page item template is documented at https://go.microsoft.com/fwlink/?LinkId=402352&clcid=0x409

namespace App19 {
    /// <summary>
    /// An empty page that can be used on its own or navigated to within a Frame.
    /// </summary>
    public sealed partial class MainPage : Page {

        private RelayCommand<int> TestCommand { get; }

        public MainPage() {
            this.InitializeComponent();
            TestCommand = new RelayCommand<int>(OnTest, index => index > 0);
        }

        private void OnTest(int index) {
            _ = new MessageDialog($"Test: {index}").ShowAsync();
        }
    }
}

Start the app using F5. You get the Exception. You don't get the Page/Window it happens from the constructor of the page/window.

This also happens in WPF apps, not only UWP.

Expected behavior

No Exception, just a working app.

Environment

NuGet Package(s): 
Microsoft.Toolkit.Mvvm
Package Version(s): 
7.0.0.0-preview4

Windows 10 Build Number:
- [ ] Fall Creators Update (16299)
- [ ] April 2018 Update (17134)
- [ ] October 2018 Update (17763)
- [ ] May 2019 Update (18362)
- [X] May 2020 Update (19041)
- [ ] Insider Build (build number: )

App min and target version:
- [ ] Fall Creators Update (16299)
- [ ] April 2018 Update (17134)
- [ ] October 2018 Update (17763)
- [ ] May 2019 Update (18362)
- [ ] May 2020 Update (19041)
- [ ] Insider Build (xxxxx)

Device form factor:
- [X] Desktop
- [ ] Xbox
- [ ] Surface Hub
- [ ] IoT

Visual Studio 
- [ ] 2017 (version: )
- [X] 2019 (version: ) 
- [X] 2019 Preview (version: )

Additional context

Add any other context about the problem here.

[msftbot[bot]]

Hello sonnemaf, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

[Sergio0694]

Thanks for the report Fons! This is something I was looking into and that had been reported by some devs in the Discord server as well. The issue is that the behavior is not really well defined in general when you're binding to a value type and the value is null. There are two possible solutions to this to avoid the null reference exception:

• Just return false if T is a value type and the input is null
• Map null to default(T) when T is a value type, and invoke CanExecute(default(T)) instead

cc. @FrayxRulez since you mentioned this on Discord as well.
cc. @michael-hawker for general feedback

I guess we should just decide on which behavior would be preferred in these cases 🙂

[sonnemaf]

I would just return false. This is the easiest thing to explain (document). Can live with default(T) too.

[michael-hawker]

I'm a bit confused, what's null here? Isn't SelectedIndex equal to -1 by default?

In the example I'm assuming if RelayCommand<int?> were used instead, it'd work fine?

I'd say returning false on null probably would lead to fewer side-effects? Imagine if the logic in the scenario about had index => index >= 0 (basically any item is selected). If SelectedIndex worked by returning null (instead of -1 when unselected) then if you mapped default(T) there'd be no way to distinguish those two states vs. just returning false for the dev.

So, I agree with @sonnemaf that returning false is probably best.

[sonnemaf]

@michael-hawker I can understand your confusion. This is just something which is badly designed in WPF and UWP. The CanExecute is called twice from the Page/Window constructor. The first time with null the second time with the defined CommandParameter which is a valid int value (the databound SelectedIndex which by default is -1).

The first one will cause the app to crash. I don't think we can influence how WPF or UWP works so we have to fix it ourselves. It doesn't really matter which solution you pick it will be overrriden by the second call immediately anyway.

[Sergio0694]

Yeah that is indeed pretty annoying, my guess is that ICommand was not really ever meant to be used with value types. With reference types of course that behavior would just not be noticed for the most part, as the CanExecute method already expects inputs to sometimes be null, so it would handle those values just fine. I think I agree with you that just returning false would make sense, especially if the alternative is to just... Throw an exception 😄

Fixed in 3f965b280c322c29ee12ddbb8eb91fbe54c0b11f.

Side note, wonder if we should open an issue for WinUI 3 to fix this quirk? 🤔

[sonnemaf]

@Sergio0694 great solution. This also works correct for a null for a Nullable<T> valuetype.

[michael-hawker]

Side note, wonder if we should open an issue for WinUI 3 to fix this quirk? 🤔

@Sergio0694 probably not a bad idea for posterity at least, though I'd tell you to open an identical one on the WPF repo as well. 🙂