TaskResultConverter causes an InvalidCastException (can't cast COM object to T)

[Sergio0694]

Describe the bug

The TaskResultConverter returns DependencyProperty.UnsetValue in the fallback path.
This causes invalid cast exceptions downstream when trying to convert the object to some type.
The converter should just return null if no value is available.

Regression

No, always been like this.

Reproducible in sample app?

• This bug can be reproduced in the sample app.

Steps to reproduce

This can be reproed in the MVVM Toolkit sample app.
Just open the "Async relay command" page and observe the crash.

Expected behavior

The converter should return null.

Screenshots

Help us help you

Yes, I'd like to be assigned to work on this item.

[michael-hawker]

@Sergio0694 this was done in #3960 to fix #3311 as the docs guidance is to return UnsetValue instead of null.

[Arlodotexe]

More detailed docs on this issue: Exceptions from converters.

@Sergio0694 Do you have an updated link to a working repro? I'd like to take a look.

[Sergio0694]

@Arlodotexe It's no longer crashing in the sample app because I used a temporary converter as a workaround, see CommunityToolkit/MVVM-Samples@0138b7a. If you remove it you can then repro the crash there as well. Imho we should just change this back to returning null. This is not the value being read from a dependency property, it's a value being converted. It doesn't make sense to me to return UnsetValue here.

[michael-hawker]

@Sergio0694 I think what the docs and we're trying to say is the binding should be this:

<Run Text="{x:Bind ViewModel.DownloadTextCommand.ExecutionTask, Converter={StaticResource TaskResultConverter}, FallbackValue='Task not set', Mode=OneWay}" />

DependencyProperty.UnsetValue is a sentinel value that has special meaning in the dependency property system, and bindings that are passed this value will use FallbackValue.

As passing UnsetValue should have the XAML engine use the FallbackValue instead. Though it's unclear if this is going to refer to old-style Binding vs. x:Bind as x:Bind only says "Specifies a value to display when the source or path cannot be resolved.".

[Sergio0694]

Wouldn't that apply to an actual dependency property though? In this case it's just a converter. It's receiving a value that has no result, so it's giving you back null, which is also consistent with what the extension does. I'm not sure I understand why it should return DependencyProperty.UnsetValue in this context 🤔

I've always treated the fallback path to be someting to use if the actual path is not valid, eg. if you have a complex path and some item along the way is null, or if you're using reflection bindings and you pass an invalid binding path.

[michael-hawker]

Hmm, was just looking at our existing tests here:

WindowsCommunityToolkit/UnitTests/UnitTests.UWP/Converters/Test_TaskResultConverter.cs

Lines 72 to 103 in b2b1c23

	[TestCategory("Converters")]
	[UITestMethod]
	public void Test_TaskResultConverter_Instance_UnsetValue()
	{
	var converter = new TaskResultConverter();
	Assert.AreEqual(DependencyProperty.UnsetValue, converter.Convert(null, null, null, null));
	Assert.AreEqual(DependencyProperty.UnsetValue, converter.Convert("Hello world", null, null, null));
	}
	[TestCategory("Converters")]
	[UITestMethod]
	public void Test_TaskResultConverter_Instance_Null()
	{
	var converter = new TaskResultConverter();
	var cts = new CancellationTokenSource();
	cts.Cancel();
	Assert.AreEqual(null, converter.Convert(Task.FromCanceled(cts.Token), null, null, null));
	Assert.AreEqual(null, converter.Convert(Task.FromException(new Exception()), null, null, null));
	Assert.AreEqual(null, converter.Convert(Task.CompletedTask, null, null, null));
	TaskCompletionSource<int> tcs1 = new TaskCompletionSource<int>();
	Assert.AreEqual(null, converter.Convert(tcs1.Task, null, null, null));
	TaskCompletionSource<string> tcs2 = new TaskCompletionSource<string>();
	Assert.AreEqual(null, converter.Convert(tcs2.Task, null, null, null));
	}

If any task is bound, it should return null if there is no result, so I'm confused now. Let me pull down the MVVM Sample app and try and get a repro there to better understand what's happening.

[michael-hawker]

I tested this out directly in the MVVM Sample App and I understand now where @Sergio0694 and I were disconnecting. The MVVM Toolkit's AsyncRelayCommand doesn't set the ExecutionTask's value until it is actually executing (i.e. the default isn't an empty Task it's null).

This means the binding in the app was pointing to a null instance and not a Task and passing this null instead of an expected Task to the converter (as it was originally written). So, on the WCT side, all of our existing tests were either already passing a Task, or not actually trying to convert the result of the converter to the expected target type (like an actual x:Bind does). When I tweaked the tests, I got the same result due to the returning of the UnsetValue. So, at least on the same page here!

Basically, regardless of if we have a Task or not the binding that the converter is attached to needs something that mimics the target type of the property being bound. I think the key missing part of our comment on the converter here:

The methods in this converter will safely return if the input task is still running, or if it has faulted or has been canceled.

This doesn't define the behavior if the input Task is missing. When we also need to return the default value, if the task isn't even set yet.