Cleanup and refactor extension expressions

[austindrenski]

We have several custom expression types that use the ExpressionType.Extension. These can be visited by overriding VisitExtension(...) rather than explicitly dispatching from the Expression.Accept(...) method.

This is partially cosmetic, but it also clarifies where we are (and are not) perturbing the normal behavior of a generic ExpressionVisitor pattern. Further, this also reduces the public API surface of the various expression visitors by moving all of the custom visits from public to protected.

Repurposing this PR to add doc comments and cleanup some inconsistencies in the extension expressions.

Moved from #541.

[roji]

I don't have time to write a full review at the moment, but this goes against the pattern used by EF Core expressions, so regardless of what we think about advantages/disadvantages I'm pretty reluctant to accept it. I'm also not sure it's an improvement to have NpgsqlSqlTranslatingExpressionVisitor aware of all our expression types in a new VisitExtension() override.

I'd recommend thinking a bit more about what exact value this brings to the table, which justifies departing from the "standard" expression pattern in EF Core itself.

[austindrenski]

this goes against the pattern used by EF Core expressions, so regardless of what we think about advantages/disadvantages I'm pretty reluctant to accept it.

Oh yikes. I didn't realize it was following a common pattern....but looking at it again, it clearly is. The value for this PR was purely to unify patterns to what I'm used to seeing in non-EF Expression work (where visits are commonly protected).

I've force-pushed over the previous commit to revert the pattern modifications, while keeping the doc comments/refactors.

The big motivation behind this PR is to unify some of the style and layout in our expression and visitor classes. #541 is getting complex, so my hope is that some style cohesion will make it easier to navigate. (More concerned with consistency than changing the pattern.)

I'm also not sure it's an improvement to have NpgsqlSqlTranslatingExpressionVisitor aware of all our expression types in a new VisitExtension() override.

I guess I was weighing one class knowing of many as being "better" than many knowing of one. But that's entirely opinion and moot given the above.

I'd recommend thinking a bit more about what exact value this brings to the table, which justifies departing from the "standard" expression pattern in EF Core itself.

I'm going to repurpose this PR to add the doc comments and consistency-refactors, without the changes to the visitor pattern.

[roji]

OK, sounds good. Yeah, as a general rule try to look at what's happening in EF Core itself before changing our own classes - we should try to stick to whatever it is they do as long as it makes sense.

Ping me when you want a review on this (or if it's trivial stuff feel free to merge as usual).

[austindrenski]

@roji All set for another look.

[austindrenski]

@roji I'm about finished with #541, but I need to merge this before #541 can be reviewed.

At this point, I think this is almost entirely cosmetic. I'm going to go over the diff once more in the morning (UTC−05:00), and then merge it if everything checks out.

If you want me to hold off on the merge so you can give it a more thorough look, just let me know.