Better strategy to split code between Expression and ExpressionInfo

[dadhi]

Current approach is mix and max with a lot of copy-paste. Hopefully, we can devise a strategy:

1 Split the code completely, may be even move to separate file and package.

Pros:

• Simple to achieve
• More performant cause requires less run-time switches
• Flexible in regards of adding new features into ExprInfo
• Less noise for those who don't need ExprInfo

Cons:

• Changes and Fixes should be copied between two
• Two files/packages to support

2 Abstract over both and have single source code for both

Pros:

• One compact code-base, no need to copy changes and fixes
• One package to support

Cons:

• Harder to-do and may impact performance due switch logic
• Likely, less flexible when adding the new features or changing ExprInfo alone

[dadhi]

3 Convert Expression to ExpressionInfo and handle the latter

I may use the approach from sum-types to make ExpressionInfo cases a structs under the ExpressionInfo class, implementing the same interface. That's will ensure the performance of conversion from Expression to EI struct.

Given that C# 7 enables in (by ref) semantics for struct parameters, the result IE can be passed to TryEmit.. methods in efficient manner.

This + migrating to C# 7+ may be a goal for v2.0

[jogibear9988]

The current strategy makes the Code much more complex. wouldn't it be possible to Generate Code only for normal "Expressions" and create a second .cs file via T4 for ExpressionInfo?
So we put "using static System.Linq.Expressions.Expression;" and change this via the T4 Template for ExpressionInfo?
The current approach makes the Code very complicated where it not needs to be.

[dadhi]

Ok may be we are at the point to split.

I did not consider T4 but did consider a split.

Bonus points except code simplification are:

• Not performance loss due casting and matching Expr vs ExpressionInfo, and no need in proxy types.
• The full parity beteen both will be automatic.

Technically, how the template should look like?

<# T4 directives ... #>
using static FastExpressionCompiler.ExpressionInfo;
<#@ include File="$(SolitionDir)\Shared\FastExpressionCompiler.cs" #>

[jogibear9988]

I think you should have at least 2 source files.

One with the complete parsing code and "using static System.Linq.Expressions.Expression;" at the top.

One file with the complete ExpressionInfo type definitions.

And a T4 Template wich replaces the "using static System.Linq.Expressions.Expression;" at the top.

[dadhi]

Putting using static aside there other things to consider.

I want and do in DryIoc use both Ecpr and ExprInfo in the same class. That means I need to prevent conflicts via either names (Info suffix) or via different namespaces.

There are method that accept Expr and ExprInfo as parameters. I would like to remove the midfke ground object parameters. In this case using static won't help, cause parameter names should be fully stated.

Then we may remove Info suffix, but need to use a new namespace, to enable non-conflicting usage.

[dadhi]

I think we may start with moving ExpressionInfo to separate file.

Then remove all the noize from FEC related to ExprInfo.

At the end we may just copy FEC.cs to new project with ExprInfo.cs and make it work, preserving minimal changes. Better 1, or 2 line changes.

Then move all tests for ExprInfo to separate project.

If everything compiles - works, it will be a very good step.

Then the new FECWithExprInfo package.

We may even dismiss or postpone T4.

[jogibear9988]

when will you start doing this?

[dadhi]

Depends. I may start after merging your requests, maybe later Today or Tomorrow.
I but I don't think I'll finish in one swoop. Maybe a couple of days.

[jogibear9988]

If I should help anything, tell me.

[dadhi]

The work is going on in dev branch

[jogibear9988]

As you now already have breaking changes, maybe it's a good idea to remove obsolete API's ?

[dadhi]

Good idea!

[jogibear9988]

maybe we should also modify the tests that most of them work with Expression and LightExpression?

[jogibear9988]

maybe we could also do this with conditional compilation?

[dadhi]

Already doing so. At the end I will link all the Unit and Issue tests (which is make sense) to the respective LE tests.

You may check AssignTests and BockTests already for how I am doing so.

[jogibear9988]

split is finished?

[dadhi]

The main part is finished.
What remains is the incremental addition of Tests (Unit and Issues) for FEC.LE, and adding whatever is missed in FEC.LE.Expression.cs

The tests may be added one-by-one without affecting other fixes. Therefore moving to master.

[dadhi]

@jogibear9988 , @dzmitry-lahoda ,

If you have possibility to help with adding the rest of the tests for LE.Expression,
I would appreciate your help.

Just put comment here about what tests you are migrating.

Check my commits above for guideline.

[dzmitry-lahoda]

I may cover ref related tests within several weeks if not yet. Other vice I may do math operators tests.

[jogibear9988]

I've converted all test where possible (GitHub issues not yet)

But the ones wich include a Expression created from c# compiler are not converted! I also think we should test c# compiler generated expressions, so we should not change this tests. If we need them also for LightExpression we need to create new ones.

[jogibear9988]

I'm working on all the GH issues tests. think 20min

[jogibear9988]

problem:

this line:

  expression = field == null ? Property(expression, property) : Field(expression, field);

does not work with light expression, because Property && Field return different types! what should I do?

[jogibear9988]

I now changed the return type to MemberExpression

[dadhi]

I now changed the return type to MemberExpression

I think this is fine, not sure why it was two expressions without common parent.
Usually, I am decompiling original System...Expression.cs and see what's there.

But the ones wich include a Expression created from c# compiler are not converted! I also think we should test c# compiler generated expressions, so we should not change this tests.

Yep, that's why I've already #ifed them out. We may consider to do tests on respective manually built exprs, but not now.

[jogibear9988]

They do have a common parent: MemberExpression

But the Return Type of the Static method of LightExpression class was to specific. It does only return a MemberExpression on the MS Code. And so the type of the variable is to specific, and C# could not compile!

[jogibear9988]

finished?

[dadhi]

I will uncomment benchmarks. Will check if any problems in using FEС and FEC.LE together. Will fix and close.