Evaluate parameter expression for Enumerable.Contains calls

[bahusoid]

Fixes #2872
Fixes #2276

Alternative for #2291. This fix specifically handles only Contains call on collection parameters. It minimizes changes and I think should suffice for our needs.

Targeting it to 5.3 as "semi-broken" queries in 5.2 now throw exception (see #2872).

[bahusoid]

Now expressions evaluation are duplicated

Seems to be fixed by 15381b0

[hazzik]

Why do we need to do that recursively?

[bahusoid]

To support chain of LINQ commands. And to make sure it doesn't start with mapped association (somethin like
db.Orders.Where( x => x.OrderLines.Select(...).Where(...).Contains(...)) should not be evaluated)

[hazzik]

Imho, there is a bug in EvaluatableTreeFindingExpressionVisitor.Analyze which marks any expression containing lambda as not evaluatable. This happens because any lambda has parameters and parameters are not evaluatable. I believe, the EvaluatableTreeFindingExpressionVisitor should visit lambdas to evaluate the subtrees, but the lambdas itself should not affect the expressions containing those lambdas.

I made the tests working by copying EvaluatableTreeFindingExpressionVisitor to our codebase and modifying VisitLambda as following:

	protected override Expression VisitLambda<T>(Expression<T> expression)
	{
		if (expression == null) throw new ArgumentNullException("expression");

		var parent = _isCurrentSubtreeEvaluatable;
		_isCurrentSubtreeEvaluatable = true;

		var vistedExpression = base.VisitLambda(expression);

		_isCurrentSubtreeEvaluatable = parent;

		// Testing the parent expression is only required if all children are evaluatable
		if (_isCurrentSubtreeEvaluatable)
			_isCurrentSubtreeEvaluatable = _evaluatableExpressionFilter.IsEvaluatableLambda(expression);

		return vistedExpression;
	}

There are some side effects (like lambdas are marked as evaluatable which it should not do, but it can be fixed further down the track)

[hazzik]

In other words the @v-kabanov's solution is more correct. Although I don't like it, because I believe it can be much simpler.

[hazzik]

Here you're modifying external dependency, which you should not.

[bahusoid]

I don't get it. Why I shouldn't touch it? It's just a wrapper around set of expressions that needs evaluation. I can store it in separate field (as I did initially) - but to minimize change noise I used already present class for storing evaluatable expressions.

[hazzik]

I don't get it. Why I shouldn't touch it?

Because this is the information provided by another concerning party. We already let this logic spread across solution and so we're trying to catch bugs doing 10th bugfix release. Let's avoid doing ad-hoc patches of logic here and there.

[bahusoid]

Well.. Ok... It can be stored separately. But I still think it's more proper to store it in _partialEvaluationInfo - external or not, it stores list of expressions that needs evaluation. So it's better to fill it also with our expressions - so all other external parties can handle it appropriately as evaluatable expression.

But before making any further changes - do you agree with this fix on principle?

[hazzik]

With the proposed implementation following expression would be considered as evaluatable:

db.Orders.Where(o => ids.Where(i => i == o.OrderId).Contains(o.OrderId))

[bahusoid]

In other words the @v-kabanov's solution is more correct.

I agree. Though I don't like including another bunch of slightly modified files from Re-Linq. IMO better local small targeted fix. We can drop it if his Relinq PR is accepted re-motion/Relinq#14

There are some side effects (like lambdas are marked as evaluatable which it should not do, but it can be fixed further down the track)

Sounds like it can cause more regressions (at least more unnecessary attempts to compile) than my fix

[bahusoid]

With the proposed implementation following expression would be considered as evaluatable:

db.Orders.Where(o => ids.Where(i => i == o.OrderId).Contains(o.OrderId))

Yeah. I know.. But it's not supported anyway.

Old behavior:
v5.2: Where part is silently ignored;
v5.3: Where part is ignored and throws InvalidCastException when setting parameter (see #2872) ;

New behavior: Exception is thrown: Evaluation failure on value(NHibernate.DomainModel.Northwind.Entities.Order[]).Where(i => (i == o.OrderId)) So I think it's an improvement as it shows exact place of failure; So it's clear which part of query is not supported.

[bahusoid]

Imho, there is a bug in EvaluatableTreeFindingExpressionVisitor.Analyze which marks any expression containing lambda as not evaluatable.

https://re-motion.atlassian.net/browse/RMLNQ-96 It looks like it's known limitation since 2016.

And here is author's thoughts about @v-kabanov's fix: https://re-motion.atlassian.net/browse/RMLNQ-127?focusedCommentId=21851

[hazzik]

But before making any further changes - do you agree with this fix on principle?

After sleeping on the issue overnight, I think we should not fix it. Not at 5.3.x at least.

I would prefer a fix of RMLNQ-96 in re-linq itself.

[bahusoid]

Ok. I rebased it on top of master.
I would prefer to fix it in 5.4 one way or another. So let it hang till we decide what should we do with Re-Linq