VB calls Convert method in an expresion tree which caused an exception in EF core

[VBAndCs]

I have this lampda in C#:
b => b.Items
which yields this expression:
b => b.Items

I converted the code to VB.NET, so I have this lambda:
Function(basket) basket.Items
which yields this expression:
basket => Convert(basket.Items, Object)

The Expression type is:
Expression<Func<T, object>> in C#
'Expression(Of Func(Of T, Object))` in VB.NET

The C# expression works fine, but the VB expression causes an exception:

InvalidOperationException: Lambda expression used inside Include is not valid.Microsoft.EntityFrameworkCore.Query.Internal.NavigationExpandingExpressionVisitor.ProcessInclude(NavigationExpansionExpression source, Expression expression, bool thenInclude)

I asked about this in EF core repo, and @smitpatel answered me:

Include lambda does not allow convert node around it. Especially for collection navigation. As a work-around you should be able to use string based include.

So, I used the code:
AddInclude(NameOf(Basket.Items))
Instead of AddInclude(Function(b) b.Items)
And it workes.
My question now: how can we solve this vb expression issue?

To reproduce the err:
Change this line to:
AddInclude(Function(b) b.Items)
This will cause the runetime exception mentioned above.

[CyrusNajmabadi]

Is this a change in behavior for the VB compiler @VBAndCs

[zspitz]

@CyrusNajmabadi Yes. You can see it here as well.

Whenever the expression type doesn't precisely match the expected type (as in this case -- the expected type is Object while the expression type is some kind of IEnumerable), the VB compiler wraps the inner expression in a Convert node.

[CyrusNajmabadi]

is this a change in behavior for the VB compiler @VBAndCs

@CyrusNajmabadi Yes. You can see it here as well.

@zspitz What did VB use to emit here?

[zspitz]

@CyrusNajmabadi AFAICT the behavior hasn't changed; not sure what you mean.

I asked @AnthonyDGreen about the reason behind the difference, but we never reached a final conclusion.

[jnm2]

Repro:

C# emits entity => entity.NavigationProperty

VB emits entity => Convert(entity.NavigationProperty, Object)

[zspitz]

@VBAndCs Could you include the link where @smitpatel gave this response? Ignoring this Convert node seems like a trivial change.

[smitpatel]

Using code samples from @jnm2
If variable is defined in VB as Dim expression As Expression(Of Func(Of Entity, List(Of ChildEntity))) = Function(entity) entity.NavigationProperty then it generates same tree as C#
Notice the type change from Expression(Of Func(Of Entity, object)) to Expression(Of Func(Of Entity, List(Of ChildEntity)))

I believe if the type is used same in C#, C# would also add the convert node. I see that C# did not add convert node.

[smitpatel]

@zspitz - dotnet/efcore#20390 (comment)

[zspitz]

@CyrusNajmabadi Sorry, I see now. No, it's not a change in the VB compiler; it's a difference between the VB compiler and the C# compiler.

@smitpatel But what is the delegate signature? If Function<T, object> that would require an explicit cast to workaround this.

[jnm2]

There are two approaches I've taken: Use object as the func return type and ignore the cast when processing the expression, or add a generic TProperty parameter to use instead of object.

[smitpatel]

The API is using TProperty in generic argument rather than object. Hence we currently don't ignore converts.

[jnm2]

Oh, this is the problematic code, has nothing to do with EF:

https://github.com/VBAndCs/eShopOnWeb_VB.NET/blob/04e5c5bcd496fdf660ab8be5fa4ea74fe7b705fa/ApplicationCore.vb/Specifications/BaseSpecification.vb#L23

        Protected Overridable Sub AddInclude(ByVal includeExpression As Expression(Of Func(Of T, System.Object)))
            Includes.Add(includeExpression)
        End Sub

@VBAndCs You need to use a generic parameter instead of object:

        Protected Overridable Sub AddInclude(Of TProperty)(ByVal includeExpression As Expression(Of Func(Of T, TProperty)))
            Includes.Add(includeExpression)
        End Sub

[VBAndCs]

This code is converted from a C# project powered by MS.
I tried to get rid of the Object, but this need massive changes in a chain of classes and interfaces.
`

[CyrusNajmabadi]

Closing out. This appears to be an issue with user code calling the wrong overload.

[VBAndCs]

@CyrusNajmabadi
I made it clear in the issue that the problem is the difference between C# and VB. This same code runs in C# look here:
https://github.com/dotnet-architecture/eShopOnWeb/blob/master/src/ApplicationCore/Specifications/BasketWithItemsSpecification.cs#15

What I am here to make VB produce the right Expression. Converting to an object is useless since it is a widening operation. It is totally unnecessary, and C# does it right.

[VBAndCs]

@tverweij
Please optimize this in mercury.

[zspitz]

@VBAndCs

I tried to get rid of the Object, but this need massive changes in a chain of classes and interfaces.

You could create an expression tree visitor which would rewrite the expression tree without the conversion. I'm not sure if you have to preserve the type (Expression(Of Func(Of T, Object))) or not.

FWIW, I agree with you; I've seen this issue come up enough times with libraries / code that was only tested against C# expression trees. Nevertheless:

We will look into fixing it in EF Core source.

for the purpose of using EF, this might be enough.

[jnm2]

Instead of having an Includes collection, you could pull the include string yourself in a few lines of code and add the string to IncludeStrings.

A second option is to replace the Includes As List(Of Expression(Of Func(Of T, Object))) collection with AddIncludes As Action(Of IQueryable(Of T)):

        Public Property AddIncludes As Action(Of IQueryable(Of T))

        Protected Overridable Sub AddInclude(Of TProperty)(ByVal includeExpression As Expression(Of Func(Of T, TProperty)))
            AddHandler AddIncludes, Function(query) query.Include(includeExpression)
        End Sub

Instead of looping over Includes in the code that builds the query and calling query.Include there, you just invoke AddIncludes if not null:

specification.AddIncludes?.Invoke(query)

[CyrusNajmabadi]

What I am here to make VB produce the right Expression.

VB appears to be abiding by its specification and has not changed behavior here. I can't see any path forward where this behavior changes. There is a clear option currently, update your VB code so no conversion is happening.

This same code runs in C# look here:

It's not the same code. VB and C# are different languages. THey don't have the same semantics. You can't just copy C# into VB and expect the semantics to stay preserved. The onus is on you when you convert to make the correct code changes to do that.

It is a non goal of Roslyn or of either language that you be able to simply copy/paste and have semantic equivalence.

[jnm2]

@CyrusNajmabadi But on the other side, I believe C# has changed its behavior here because this used to be a thing we had to deal with in C# too. Why couldn't VB follow suit if the community wanted to impl?

[CyrusNajmabadi]

Current plan for VB is to prioritize extremely highly its stability. So breaking changes are even less possible now than they might have been in the past (where they were still extremely unlikely).

[zspitz]

@CyrusNajmabadi

VB and C# are different languages. THey don't have the same semantics. You can't just copy C# into VB and expect the semantics to stay preserved.

Is this difference in expression tree generation somehow related to the semantics of VB vs C#? If so, could you elaborate on why VB semantics require wrapping the inner expression with a Convert node while C# semantics don't require this? Neither language requires writing an explicit conversion, but both are actually carrying out an implicit one.

(I'm interested in hearing something along the lines of "why in C# the use of == to compare two strings generates a BinaryExpression whose NodeType is Equal, while in VB the corresponding string comparison generates a method call to Microsoft.VisualBasic.CompilerServices.Operators.CompareString? because VB has different text comparison modes while C# does not.")

[CyrusNajmabadi]

If so, could you elaborate on why VB semantics require wrapping the inner expression with a Convert node

I don't understand the user of the term 'required' in this context. The requirement, for me, would be in not changing behavior in an observable fashion here in VB for reasons of stability. There may be code that expects this shape of tree and which will fail if this changes.

[CyrusNajmabadi]

In terms of what's going on, i imagine it be because under the covers vb actually does this conversion in its bound node tree, and expression trees are effectively generated by dumping that bound tree out.

[zspitz]

@CyrusNajmabadi

In terms of what's going on, i imagine it be because under the covers vb actually does this conversion in its bound node tree, and expression trees are effectively generated by dumping that bound tree out.

And C# doesn't do this? Or does the same thing happen in C# except the C# compiler optimizes away the conversion node?

[CyrusNajmabadi]

And C# doesn't do this?

I haven't verified, but based on what others have said here, apparently not.

[jnm2]

I'm certain C# did this in the not-too-distant past because I had to deal with this same thing and VB wasn't involved.

[zspitz]

@jnm2 It's really just to satisfy my curiosity -- as @CyrusNajmabadi says, it's probably not something that can be fixed now -- but can you see any reason why the fix wasn't implemented at the same time for VB?

[jnm2]

I'm wrong. I just checked Roslyn 1.0.0 as well as C:\Windows\Microsoft.NET\Framework64\v3.5\csc.exe and both have the same behavior. What I was remembering must have been C# adding Convert if the property type is a value type.

[VBAndCs]

This VB behavior caused a similar issue in Moc devlooped/moq#1067 (comment)
It will be wise to fix the source, not to waste other frameworks teams times.

[CyrusNajmabadi]

@VBAndCs Changing VB's behavior here is not likely to be possible due to the potential breaks that would happen.

[zspitz]

I would add that there are more than enough semantic differences between the expression trees generated by C# and those generated by VB, that library authors must be made aware that code which parses expression trees in one may not work in the other.

[VBAndCs]

@CyrusNajmabadi
VB can add a new method (or an overload), that can generate the expected expression tree, and make an option so we can choose to use this new method to have the desired tree.

[zspitz]

Or you could write your own method that takes an existing expression tree and returns a new expression tree with the conversions removed.

[CyrusNajmabadi]

so we can choose to use this new method to have the desired tree.

I don't really know what your desired tree is. It would make a lot more sense if this was provided in your own library code imo.

That, or take the library that is consuming these expression trees and update them to handle these trees.

[VBAndCs]

@CyrusNajmabadi
It is not only one library. It is all libraries (by .NET or a third party) because they all are designed and tested on C# only. So, VB must use the C# expression tree as a standard. The expression tree is not a syntax language to say it is a key feature of VB that developers accustomed to., It is a generated skip that has a design that turned out to be troublesome. I will not change myself and my code to cherish such a design. My compiler should serve me not the opposite. This is why compilers invented in first place, and how we used to have VB for decades.
It is sad to see a small firm of just 10 developers working on 6 programming languages, succeed to rebuild VB, add many missing features to, and taking it cross platform and support mobile and wasm, all in just few months, while we wasted a decade years waiting for minor fixes and only got the language frozen. Anthony already ststed the same fact: Only 5 developers were more than enough to save the lang, and Mercury proven that 10 programmers were enough to put it on the top (regardless Mercury will be well marketed).

[CyrusNajmabadi]

because they all are designed and tested on C# only.

i imagine this isn't the case, otherwise VB wouldn't have ever been able to use any of these libraries since we released Linq in 2005.

My compiler should serve me not the opposite.

The compiler serves millions of users. We have to consider hte needs of all of them, and breaking back compat is a serious deal. Given the small amount of feedback here, i think it would not be a good idea to change the behavior here. This can easily utterly break VB for many.

[zspitz]

@CyrusNajmabadi

i imagine this isn't the case, otherwise VB wouldn't have ever been able to use any of these libraries since we released Linq in 2005.

Or the library authors simply didn't check against VB, and library consumers writing in VB found creative ways of working around such issues in order to get on with the task at hand. Case in point: I filed this issue on the simple-odata-client library over a year ago, with both a workaround and a suggested fix. The author gave some vague reassurance to consider the problem, but that appears to have been the end of it. I'm not blaming the author, but I suggest that since the workarounds are usually rather straightforward, and C# devs are usually the more vociferous, there is no great pressure for library authors to fix such things.

Even a simple list of the differences in how C# and VB generate expression trees -- both differences which come from differing semantics (such as string equality) and those which are simply differences in design choice -- would be an invaluable resource for library authors.

This can easily utterly break VB for many.

Perhaps it's my narrow focus, but the specific difference (in generated expression trees) doesn't seem to have anything to do with the semantics of either language; the C# development team went right, and the VB dev team went left. I would say that removing the conversion node from generated expression trees would more closely match the semantics of VB, as you don't usually have to write in a conversion when passing a narrow type into a wider type variable/argument.

[VBAndCs]

Given the small amount of feedback here

This is because MS forced VB developers to use C# with new technologies, so, nothing to report! We began to report such issues when we tried to take VB to these areas. So, our reports is of superior importance as we try to open the door for millions of developers to use VB with these technologies. So, there is defiantly an error in prioritizing such issues in areas related to ASP.NET Core, Blazor, and Entity Framework Core. If you are OK with killing VB, so, go ahead and continue this attitude. Otherwise, you must assign a dedicated team to fix such issues, before Mercury attract hundreds of thousands of VB programmers in the next few years, where they can migrate their apps easily, but after use new features there, it will be impossible to bring them back again.
Betting on that most developers will migrate to C#, is a risk that should be recalculated after Mercury emerged.

[CyrusNajmabadi]

Perhaps it's my narrow focus, but the specific difference (in generated expression trees) doesn't seem to have anything to do with the semantics of either language;

The difference can be something that companies may be depending on. Changing this can break apps on upgrade.

[CyrusNajmabadi]

This is because MS forced VB developers to use C# with new technologies, so, nothing to report!

Outside of ref-structs (which were just in the last couple of years), there was full access to VB. So people can, and did continue to use VB.

So, our reports is of superior importance as we try to open the door for millions of developers to use VB with these technologies.

I don't think it would be fair to audience that report and weigh in on issues more often to deprioritize them against issues that that have less feedback. At that point, it's unclear why we would even solicit feedback on issues at all.

If you are OK with killing VB, so, go ahead and continue this attitude.

I don't see how prioritizing not breakign VB will kill VB. I think if we haphazardly make changes that may break VB would be potentially disastrous for the language.

Otherwise, you must assign a dedicated team to fix such issues,

Roslyn is the dedicated team to working on VB. As i've mentioned in the past, we put in a lot of effort on this. For example, just this last week i expanded on a feature greatly and i ensured that it works for VB as well. This is also true when we take in community PRs. We try to have that almost always be the case (and we will help out with this ourselves if community members don't necessarily want to do this themselves).

At this point, this part of the conversation is over. It is offtopic to the core issue of this particular expression-tree representation. If you want to discuss the future of VB please take it to gitter.im or one of our discord channels.

[Happypig375]

@CyrusNajmabadi When will be the next VB language design meeting?