Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use System.Linq.Expressions.ExpressionVisitor where available #182

Merged

Conversation

TheConstructor
Copy link
Contributor

@TheConstructor TheConstructor commented Feb 26, 2023

Addresses #122. This also adds support for e.g. Block and Throw expressions, where they are available, making this PR supersede #178.

I tried to build upon the 'expression-visitor'-branch, but found that the visitor-method VisitMemberAccess needs to be renamed to VisitMember to be able to get a working library. I wish they had chosen VisitMemberAccess, as it better communicates what happens, but they didn't.

@TheConstructor
Copy link
Contributor Author

On first glance this seems to play nice regarding #116 and #118, too. The shipped ExpressionVisitor's VisitExtension looks like this:

protected internal virtual Expression VisitExtension(Expression node) => node.VisitChildren(this);

with e.g. QueryRootExpression just returning this, when receiving this call, but giving the extension-expression the opportunity to interact nicely with the visitor.

@StefH
Copy link
Collaborator

StefH commented Mar 3, 2023

@TheConstructor
Thank you for the PR, I'll need some time to review.

src/LinqKit.Core/Utilities/IntrospectionExtensions.cs Outdated Show resolved Hide resolved
src/LinqKit.Core/Utilities/IntrospectionExtensions.cs Outdated Show resolved Hide resolved
src/LinqKit.Core/Utilities/TaskHelper.cs Outdated Show resolved Hide resolved
@StefH StefH merged commit e978e34 into scottksmith95:master Mar 18, 2023
@TheConstructor TheConstructor deleted the feature/build-on-shipped-visitor branch March 18, 2023 17:18
@doboczyakos
Copy link
Contributor

This is a breaking change for custom expressions

@TheConstructor
Copy link
Contributor Author

@doboczyakos this was merged more than a year ago, there are quite some tests and I used the library during the full time. While it certainly changes things, I would hope, that it is rather for the better (e.g. support for all bundled expression-types).

Can you provide a minimal example, that showcases the problem you encountered? Maybe we can make a new issue of that and find a solution, that works for you, too.

@doboczyakos
Copy link
Contributor

@doboczyakos this was merged more than a year ago, there are quite some tests and I used the library during the full time. While it certainly changes things, I would hope, that it is rather for the better (e.g. support for all bundled expression-types).

Can you provide a minimal example, that showcases the problem you encountered? Maybe we can make a new issue of that and find a solution, that works for you, too.

Dear @TheConstructor ,

I deleted my comment when I found the root cause and a solution, which was the following:
Changing the ExpressionVisitor caused the change of VisitExtension from
protected virtual Expression VisitExtension(Expression extensionExpression)
{
return extensionExpression;
}
to
protected internal virtual Expression VisitChildren(ExpressionVisitor visitor)
{
if (!CanReduce) throw Error.MustBeReducible();
return visitor.Visit(ReduceAndCheck());
}

When I call Expand() it causes the calling of ReduceAndCheck() which is a breaking change and leads to exception in some cases.
I have some tricky custom Expression types. I solved the issue by overriding Accept() in my expression type the following way:
protected override Expression Accept(ExpressionVisitor visitor) => this;

@TheConstructor
Copy link
Contributor Author

Hey @doboczyakos,

Glad to hear, that you found a way to make it work for you again. I did check how ExpressionVisitor changes and pointed it out in #182 (comment) ; I also checked all the Extension-Expressions I found in EF Core, but I may have missed the cited default-Implementation in Expression. If you write your own extensions, you likely need to override it now. Sorry.

Given that the release is "out there", it may be difficult to change it's version in retrospective, but If you could provide a unit-test, we could be able to prevent similar breaking changes in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants