ExpressionComparer Block Support?

[a046]

What's the criteria for Expressions supported via ExpressionComparer.ExpressionEqual for node sharing?

I notice support was expanded in #239, but it does not support Block expressions. I suspect a Block is unlikely (impossible?) via the Fluent API and would probably be non trivial to compare, so are these unsupported on the left hand side of rules?

[snikolayev]

@a046 the initial set of expressions was indeed selected based on what's possible to represent as an expression tree with fluent API, given what's supported by the compiler. And, as you said, the compiler does not support block expressions. And more expressions were added based on bug reports and feature requests from the community. I don't think there is a fundamental reason preventing comparison of block expressions, just a matter of a need. And so far there was none.
There may be a compromise solution though - just treat unsupported types of expressions as not equal, hence give up node sharing, but at least not fail rule compilation. Does that sound like a reasonable path forward to you?

[a046]

Thanks for the fast reply!

The compromise sounds good, the only issue I can see is that it might hide future errors for cases that should be handled for node sharing. So if the compromise solution is done, I wonder if it's worth suppressing it for the Fluent API in some way to catch those cases?

For context, I am trying to approximate the safe navigation operation (?., not supported in Expression Trees) with Blocks that cache the intermediate nullable result and check it for null. The alternative approach I believe would be to null check individual parts of my expression before checking a null unsafe component, which is not exactly equivalent as it hits the properties multiple times and is also probably not effective for checking for node sharing either,

[snikolayev]

That's a fair point about masking all failed expression comparisons. This is one of the reasons I had it fail fast, to use as a forcing mechanism for adding support for more expressions as those surfaced through bug reports. I'm thinking perhaps adding an options/settings argument to the RuleCompiler, where by default it will continue failing fast, but will have an option to override that behavior. Another idea is to collect those failures, such that they can be logged/inspected w/o failing the whole ruleset compilation.

Regarding safe navigation - I see what you are doing; probably something like https://stackoverflow.com/questions/44681362/an-expression-tree-lambda-may-not-contain-a-null-propagating-operator
Like I said, there is really no fundamental reason to not support block expression comparison. Probably going to be a bit tricky, as it needs to detect equivalent expressions, ignoring variable names. Though, while I can commit to adding a fallback behavior fairly quickly, comparing block expressions is probably going to take some time.

Another possible idea, given the context you provided, is to catch NullReferenceException in LhsExpressionFailedEvent and just setting IsHandled to true. Then the engine will carry on as if the expression evaluated to false. This is not ideal, but if the situation is infrequent, and if short-circuiting to false is what's needed, may be acceptable.

[a046]

I'm a fan of failing fast too, so the RuleCompiler flag sounds good. I imagine failure logs will go unseen.

That StackOverflow approach technically isn't functionally equivalent as it will hit any getter code multiple times, so I went with a Block based approach instead. EF doesn't support Blocks either, so no one suggests that approach on SO. I made an extension method for it, so I can trivially convert it over to Expression.Condition as an interim workaround, and refactor it back eventually.

I agree, full BlockExpression support is definitely a very low priority and will probably have some complexities. The optional fallback behavior still might be a nice addition as it will handle this case, but also provides a fast workaround if anyone finds new unhandled cases.

[snikolayev]

I raised an issue for the fallback feature: #282

Regarding that SO, I think the third answer was talking about an expression visitor rewriting the OG expression to use block expressions.