Maui icon indicating copy to clipboard operation
Maui copied to clipboard

[BUG] IsListNullOrEmptyConverter (plus others) do not work with nullable target types

Open GalaxiaGuy opened this issue 2 years ago • 8 comments

Is there an existing issue for this?

  • [X] I have searched the existing issues

Did you read the "Reporting a bug" section on Contributing file?

  • [X] I have read the "Reporting a bug" section on Contributing file: https://github.com/CommunityToolkit/Maui/blob/main/CONTRIBUTING.md#reporting-a-bug

Current Behavior

When using IsListNullOrEmptyConverter to bind to a bool? value (for example AutomationProperties.IsInAccessibleTree) an InvalidCastException is thrown.

Expected Behavior

The converter works.

Steps To Reproduce

Use code like the following in XAML

AutomationProperties.IsInAccessibleTree="{Binding Items, Converter={StaticResource IsListNullOrEmptyConverter}}"

Or the following C#

var converter = new IsListNullOrEmptyConverter() as IValueConverter;
converter.Convert(new List<string>(), typeof(bool?), null, CultureInfo.CurrentCulture);

The linked repo contains XAML that triggers straight away. You will need to break on exceptions to tell that it has happened.

Additionally, there is a button to trigger it manually.

Link to public reproduction project repository

https://github.com/GalaxiaGuy/NullableConverterBug

Environment

- .NET MAUI CommunityToolkit: 7.0.1
- OS: macOS
- .NET MAUI: 8.0.3

Anything else?

No response

GalaxiaGuy avatar Feb 13 '24 12:02 GalaxiaGuy

Have you tried disabling exceptions for converters:

https://learn.microsoft.com/en-gb/dotnet/communitytoolkit/maui/get-started?tabs=CommunityToolkitMaui#using-the-nuget-packages

bijington avatar Feb 13 '24 19:02 bijington

Hi,

I don't think that would make a difference since the exception is already caught when used in XAML. I previously thought it was preventing the converter from correctly executing with the binding, but looking at it now it does seem fine.

I would guess that is expected behavior? Calling the converter manually still throws, but that seems uncommon for someone to do.

In my case it is also annoying since I'm trying to upgrade a Forms app to MAUI and breaking on exceptions especially to track down binding issues is useful. That exception is getting thrown a lot for us so some check that doesn't rely on exception handling would be preferred.

For reference, the exact line that trigger the invalid cast is on line 24 here:

https://github.com/CommunityToolkit/Maui/blob/666f0e97ced203c8db310b1cb772c9dcb6f37f7c/src/CommunityToolkit.Maui/Extensions/ValueConverterExtension.shared.cs#L23-L24

From what I can tell, it seems T is bool and targetType is bool?, so I think the failure is trying to assign to the instanceOfT variable.

GalaxiaGuy avatar Feb 14 '24 09:02 GalaxiaGuy

I thought there was a PR that fixed this issue! I need to dig around and work out what it was

bijington avatar Feb 14 '24 12:02 bijington

This was the PR that fixed this issue https://github.com/CommunityToolkit/Maui/pull/1307 the code that you've linked to does appear to be quite different so perhaps we have regressed somehow.

I'll try and take a look at some point.

bijington avatar Feb 14 '24 12:02 bijington

@GalaxiaGuy I am only testing in VS Code and I am not seeing the exception being thrown from XAML. I have tested your code by adding a try catch around the manual triggering code and with debugging it I don't see any exceptions.

bijington avatar Feb 25 '24 14:02 bijington

@bijington Apologies, it seems I don't see an exposed exception after all, it was just been caught internally. I originally thought the correct result from the converter was not getting setting in that case but I think something else was causing me problems as the same time.

I don't know if limiting internal exceptions is a specific goal, but I honestly have to run with exception breakpoints enabled quite frequently, especially to track down XAML issues. I think swapping the these two conditions would have the same result without throwing an exception internally:

			&& !IsValidTargetType<TTarget>(targetType) // Ensure targetType be converted to TTarget? Eg `Convert.ChangeType()` returns a non-null value
			&& !(shouldAllowNullableValueTypes && typeof(TTarget).IsValueType && IsValidNullableValueType(targetType))) // Is TTarget a Value Type and targetType a Nullable Value Type? Eg TTarget is bool and targetType is bool?

If I confirmed that and were to submit a PR to that effect would it likely to be accepted?

GalaxiaGuy avatar Feb 26 '24 08:02 GalaxiaGuy

It isn't catching an exception internally. I have run your same code in our sample application and stuck a breakpoint in the exception handling as well as when converting and no exceptions are thrown. Maybe I am missing something?

But to answer your question, if there is a bug then we would love to accept a PR to resolve it :)

bijington avatar Feb 26 '24 19:02 bijington

This is the (caught) exception I see in Rider:

Screenshot 2024-02-27 at 09 25 33

While I getting that, I confirmed it is definitely caught and the end value is converted correctly, so I don't think it's worth anyone else looking at.

GalaxiaGuy avatar Feb 27 '24 09:02 GalaxiaGuy

@GalaxiaGuy apologies for not getting back to this sooner, I have managed to reproduce the issue and can confirm that your proposed solution does in fact solve the issue. Would you still be willing to submit the PR? I would be more than happy for you to take the credit for the work. If not I am happy to submit it instead.

bijington avatar Jun 09 '24 18:06 bijington

Actually I take it back, while the proposed change does prevent the exception from being thrown the current sample app now crashes with the following:

<Label
    Text="{Binding StringItemSource, Converter={StaticResource IsListNullOrEmptyConverter}}"
    FontAttributes="Bold"
    TextColor="Red" />

I think I have a solution which I will submit via PR shortly

bijington avatar Jun 11 '24 20:06 bijington