Allow to evaluate expressions on constants #14
Conversation
…essions on constants can be evaluated (in external code)
…llow to override it in the filter. Unit tests pass.
…pe test and added complementary SubQuery_InNewExpression_RetainsTypeWithEvaluatableParameterButQueryableConstant
|
Hello @v-kabanov, thank you for your contribution. I've looked it over for basic stuff first and the following topics arose: It would be helpful if you could open the corresponding issues on our issue tracker (https://re-motion.atlassian.net/projects/RMLNQ) or start a discussion on the feature requests in our mailing list (https://groups.google.com/forum/#!forum/re-motion-dev). Either way, we can discuss the ideas first before there's effort spent on a specific implementation. In the PR itself, there appears to be at least two separate concerns: one is the actual feature for evaluating expressions on constants, the other is some API changes and un-sealing. Those will need to be separated into dedicated discussions and thus PRs. As for the code, there appears to be unrelated and/or non-standard code-reformatting and parameter checks that do not follow established practices. In general regarding the non-public or sealed members: those choices have been made intentionally to allow us to follow semantic versioning principles by preventing dependencies on implementation details. Any virtual member requires careful consideration of its use as an extension point, what happens when the member is called, or if one chooses to remove a non-public virtual member. Similiarily, it is not possible to introduce a new member to an interface without causing a breaking change. While I'm okay with introducing such breaking changes with a new major version, a new major version must wait until there's enough changes or need to warrent this. It might therefor be necessary to do a non-breaking implementation first, depending on your roadmap-needs. Best regards, Michael |
…ableParameter compilation for .net 4.0 and below
…luatingExpressionVisitor constructor private again as required by maintainer
|
Hi Michael, I used to call 'check' library methods in the past too but since the code is now autogenerated (by e.g. resharper) rather than typed the reason for doing it has disappeared. But it's not a big deal, relevant change has been pushed. Regarding the class sealing and private constructors I think for the free and open source project like this one it is unnecessary. Developers inheriting/overriding it will already be resposnsible for the consequences. I'll now try to create an issue in your tracker. Regards, |
|
RMLNQ-127 created |
|
Hi Vasily, thank you for the first clean up. There's still some formatting changes left (plus unrelated config files) that will need cleanup before actual integration but that can wait until the other stuff is done. Those changes are at least non-intrusive when reviewing the PR. PartialEvaluationQueryParserIntegrationTest is missing an integration test. That's actually one of the more central test for this feature, given it deals with partial evaluation of the expression tree in the first place :) I've got an e-mail thread regarding https://re-motion.atlassian.net/browse/RMLNQ-96 that I will need to dig out and put into the jira ticket. It turns out, this is not the first time we've discussed this particular issue (if my assumption is correct about this being a duplicate ticket) and there is quite a bit of stuff around this in order to provide a complete solution. At least during the discussion. It's always possible that the solution turns out to be much simpler than expected. Best regards, Michael TODOs for the PR-review:
|
|
Hi Vasily, just wanted to give you quick heads-up: I unfortunately wasn't yet able to integrate the specifications for your PR in Jira due to time constraints on my part. I'm sorry for that. Best regards, Michael |
|
No worries Michael, thanks for the update. |
| /// If lambda is not eliminated, evaluated parameter produces an error complaining that evaluating lambda parameter | ||
| /// must return non-null expression of the same type. | ||
| /// </remarks> | ||
| private ParameterStatus CalcParameterStatus(ParameterExpression expression) |
There was a problem hiding this comment.
You can use full name without shortening 😃
CalculateParameterStatus
MichaelKetting
force-pushed
the
develop
branch
from
d3c0e62
to
c3b77d5
Compare
Lambda expressions on constants could not be evaluated because all parameters were considered non-evaluatable, which caused e.g. NHibernate issue #2276.
Relevant classes required to override this behavior were sealed and duplication of Relinq code in target project was required.
This change makes evaluatable expression finding visitor keep track of ancestors and examine lambda expression defining parameter to establish its evaluatability. New method is added to the evaluatable expression filter interface and default (base) implementation prohibits evaluation on any parameters for backward compatibility. If filter is overridden and evaluation of parameters is allowed, lambda expressions on constants (non-queryable) can be evaluated.
Also some relevant classes are un-sealed and made public.