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]

It seems does the job but I wonder how to make it more optimal. Now expressions evaluation are duplicated inside NhPartialEvaluatingExpressionVisitor.TryGetCollectionParameter and as part of common parsing inside NhPartialEvaluatingExpressionVisitor.Visit(Expression expression).

[bahusoid]

Now expressions evaluation are duplicated

Seems to be fixed by https://github.com/nhibernate/nhibernate-core/pull/2873/commits/15381b0a47c42f23de76b9296c303f5b7974d433

[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]

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 https://github.com/re-motion/Relinq/pull/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