Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a failing test for conditional operator nullable conversions #1746

Merged
merged 2 commits into from
Dec 1, 2023

Conversation

tomasherceg
Copy link
Member

I have added a test case BindingCompiler_ImplicitConversion_ConditionalNullableAndNonNullable for conditional operators that cannot compile:

boolValue ? nullableDouble : double

It seems to me that the VisitConditionalExpression can do both implicit conversions (from nullable double to double and vice versa) and cannot decide on the direction.

Shall we be even able to do an implicit conversion from nullable to non-nullable?
I am not sure if I can fix it in the logic of implicit conversion, or if it is a special case here.

tomasherceg and others added 2 commits November 28, 2023 15:22
…ullableAndNonNullable` for ternary operator nullable conversions
We allow non-standard T? -> T implicit conversion, which leads to
ambiguity when T : T? are in conditional expression.
Special case for this has been added.
@exyi
Copy link
Member

exyi commented Nov 29, 2023

I fixed it by adding a special case. I didn't know we allow the "from nullable" implicit conversion, but we can't disallow this in a patch release, so I added a special case to the VisitConditionalExpression function.

Also, I had to adjust the testcase for B ? null : DoubleProp. This is not allowed by standard C#. It (unfortunately) works in DotVVM, but returns object, not double?. Again, we can't disallow it in a patch version. Changing the return type to double? is also a bit risky (and not trivial), but doable. We can either forbid this in v5, or make it return double? in v4.3 or v5, whichever comes sooner.

@exyi exyi marked this pull request as ready for review December 1, 2023 13:41
@tomasherceg tomasherceg merged commit 3817d9f into main Dec 1, 2023
12 of 15 checks passed
@tomasherceg tomasherceg deleted the fix/ternary-operator-conversion branch December 1, 2023 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants