Catch exception in TryConvertTo

[jeffl8n]

Catch when the converter throws an exception and return false and set the out value to default.

Addresses #33587

[SteveSandersonMS]

Thanks for contributing this, @jeffl8n!

Rather than adding a try/catch that affects all bind converters, would it be possible to do this inside the GUID converter only? This is because:

(1) try/catch has a perf cost on WebAssembly. It's a very small cost, but nonzero, so we try to minimize it in code paths that might run at a high frequency.
(2) Suppressing other exceptions might lose valuable information. I know that things like int.TryParse should generally not throw, and that's all the more reason to raise a serious alert if there is some case where it does unexpectedly throw.

[jeffl8n]

Thanks for the review @SteveSandersonMS.

Should it be a new type specific handler for GUID, or should we wrap the generic type converter with the try/catch?

aspnetcore/src/Components/Components/src/BindConverter.cs

Line 1542 in a6e00f6

var converted = typeConverter.ConvertFrom(context: null, culture ?? CultureInfo.CurrentCulture, obj);

This would add the try/catch for any of the type converter not already explicitly handled in BindConverter.cs.

[SteveSandersonMS]

I was expecting it to be a GUID-specific parser, i.e., a new case in this long list here:

aspnetcore/src/Components/Components/src/BindConverter.cs

Lines 1427 to 1508 in a6e00f6

	if (typeof(T) == typeof(string))
	{
	parser = ConvertToString;
	}
	else if (typeof(T) == typeof(bool))
	{
	parser = ConvertToBool;
	}
	else if (typeof(T) == typeof(bool?))
	{
	parser = ConvertToNullableBool;
	}
	else if (typeof(T) == typeof(int))
	{
	parser = ConvertToInt;
	}
	else if (typeof(T) == typeof(int?))
	{
	parser = ConvertToNullableInt;
	}
	else if (typeof(T) == typeof(long))
	{
	parser = ConvertToLong;
	}
	else if (typeof(T) == typeof(long?))
	{
	parser = ConvertToNullableLong;
	}
	else if (typeof(T) == typeof(short))
	{
	parser = ConvertToShort;
	}
	else if (typeof(T) == typeof(short?))
	{
	parser = ConvertToNullableShort;
	}
	else if (typeof(T) == typeof(float))
	{
	parser = ConvertToFloat;
	}
	else if (typeof(T) == typeof(float?))
	{
	parser = ConvertToNullableFloat;
	}
	else if (typeof(T) == typeof(double))
	{
	parser = ConvertToDoubleDelegate;
	}
	else if (typeof(T) == typeof(double?))
	{
	parser = ConvertToNullableDoubleDelegate;
	}
	else if (typeof(T) == typeof(decimal))
	{
	parser = ConvertToDecimal;
	}
	else if (typeof(T) == typeof(decimal?))
	{
	parser = ConvertToNullableDecimal;
	}
	else if (typeof(T) == typeof(DateTime))
	{
	parser = ConvertToDateTime;
	}
	else if (typeof(T) == typeof(DateTime?))
	{
	parser = ConvertToNullableDateTime;
	}
	else if (typeof(T) == typeof(DateTimeOffset))
	{
	parser = ConvertToDateTime;
	}
	else if (typeof(T) == typeof(DateTimeOffset?))
	{
	parser = ConvertToNullableDateTime;
	}
	else if (typeof(T).IsEnum)
	{
	// We have to deal invoke this dynamically to work around the type constraint on Enum.TryParse.
	var method = _convertToEnum ??= typeof(BindConverter).GetMethod(nameof(ConvertToEnum), BindingFlags.NonPublic | BindingFlags.Static)!;
	parser = method.MakeGenericMethod(typeof(T)).CreateDelegate(typeof(BindParser<T>), target: null);
	}

[SteveSandersonMS]

I must be missing something here. How come you don't need a try/catch any more? I thought that adding the try/catch was the main reason for the change.

Does structuring the code this way somehow mean there isn't an unhandled exception any more? If you could clarify that would be great!

[campersau]

Yes, because before the GuidConverter would call the ctor which throws on invalid values. #33587 (comment)
You can see it in the stack trace from here #33587 (comment)

---

Should there be another case for Guid??

[jeffl8n]

That's correct. Calling Guid.TryParse sets the ThrowStyle to None - https://github.com/dotnet/runtime/blob/5221db92ed579f8ffa56f1d8e84f07ba6d896077/src/libraries/System.Private.CoreLib/src/System/Guid.cs#L242
But calling the Guid ctor sets the ThrowStyle to All - https://github.com/dotnet/runtime/blob/5221db92ed579f8ffa56f1d8e84f07ba6d896077/src/libraries/System.Private.CoreLib/src/System/Guid.cs#L210

Should there be another case for Guid??

Do you mean like the check for null/empty string? The Guid class performs several checks for length and formatting, so that current check could be removed as well, but I left it since it would prevent having to call TryParse for the simplest invalid case.

[campersau]

I meant for the typeof(Guid?) type, all the others do both checks like typeof(DateTimeOffset) and typeof(DateTimeOffset?):

aspnetcore/src/Components/Components/src/BindConverter.cs

Lines 1495 to 1502 in a6e00f6

	else if (typeof(T) == typeof(DateTimeOffset))
	{
	parser = ConvertToDateTime;
	}
	else if (typeof(T) == typeof(DateTimeOffset?))
	{
	parser = ConvertToNullableDateTime;
	}

[jeffl8n]

You're right. I've just added another case for Nullable Guid.

[SteveSandersonMS]

A quick note on this: on investigation I realise this doesn't impact using @bind with a GUID property/field at all, since the existing parsing logic inside EventCallbackFactoryBinderExtensions.CreateBinderCore already handles any exception from the typeconverter and rejects the invalid value cleanly as we'd want.

This change only affects someone calling BindConverter.TryConvertTo<Guid>(...) directly from their own code, which is legal, but not a mainstream scenario.

Since we're late in 6.0 now we're trying not to take inessential changes, so I'll mark this for 7.0. We can merge it into main. Note that the logic inside ConvertToNullableGuidCore in this PR seems incorrect, as it's treating null/blank inputs as parsing failures when for nullable outputs the other conversion logic treats them as successfully parsed as null. Also it's missing unit tests for the nullable cases.

[jeffl8n]

Thanks @SteveSandersonMS. I added the missing tests for the Guid? type and have corrected the parsing success to true in the case of null/empty string trying to be parsed.

[SteveSandersonMS]

@jeffl8n Now that 7.0 work is underway, we're ready to proceed with this. However the underlying code has changed too much to merge this directly, since we started using file-scoped namespaces and the default indentation level has changed. I also wanted to tweak the test cases a bit.

So, I've filed a continuation at #39649 and will close this one in favour of that. The new PR still credits you as the commit author :)