Unify Expr and SimpleExpr, add ExprTrait?

[Expurple]

Currently, a lot of "operator" methods are duplicated across Expr, SimpleExpr and sea_orm::ColumnTrait, but the list of methods is slightly different for each. The type conversions are also messy and non-obvious. I complained about this in #770 today. Writing generic code on top is especially difficult.

I'd really like to:

• Get rid of Expr / SimpleExpr divide
• Move their methods into a trait (ExprTrait ???) and reuse it for SeaORM columns (ColumnTrait: ExprTrait ???)

I understand that this is a big change that won't happen soon (especially with SeaORM 1.0 on the horizon). But still, let's discuss this possibility

---

EDIT: Some other types that would benefit from implementing ExprTrait:

• sea_query::FunctionCall (see Struggling to express `LOWER(expr) IS IN values` (+ many deep issues with expressions) #770)
• sea_query::Value

---

UPDATE 2024-07-06: I'd like ExprTrait binary operator methods to accept an Into<SimpleExpr> parameter (like in Expr::eq), rather than Into<Value> (like in ColumnTrait::eq). Currently, when I want to compare a SeaORM column with an expr which isn't a column or a value (e.g. Expr::cust), I have to wrap the column in Expr::col like this: Expr::col(my_column).eq(Expr::cust(...)). It's verbose and very unintuitive.

And to add another example of the current everyday pain without ExprTrait. When I needed to express Expr::cust(...).lte(rust_value), I couldn't do that in a straightforward way. Expr::cust returns a SimpleExpr, which doesn't have an lte method. It only has eq and ne. There's Expr::lte, but it requires the left side to be an Expr. And I couldn't find a way to convert from SimpleExpr to Expr. Eventually, I came up with this workaround: Expr::cust(...).binary(BinOper::SmallerThanOrEqual, rust_value). Now, as I was typing this and double-checking everything, I finally foud Expr::expr which allows me to write Expr::expr(Expr::cust(...)).lte(rust_value). But it's still not the prettiest solution and definitely not intuitive/discoverable. Btw, I really don't understand why you couldn't just impl From<SimpleExpr> for Expr instead of defining this random inherent method which I couldn't even find among 58 other inherent methods. These type conversions and unintuitive APIs really drive me mad from time to time, and this is what sparked the idea of ExprTrait.

Sorry for this little rant, I still appreciate the work done on SeaORM. And I still hope to do an initial PR with ExprTrait when I have time/courage for larger open source PRs that aren't adding features required by the business.

---

UPDATE 2024-07-07: here's the first PR with ExprTrait: #791 🚀

I've listed some of my further plans there.

Based on that work, now I'm pretty sure that Expr indeed shouldn't be a type. It seems like just a namespace for static SimpleExpr constructors! I'll see what I can do while minimizing breakage. I hope to turn one into a type alias of another. This should be possible, because there are no conflicting methods and all Expr fields are private.

[tyt2y3]

I think having an ExprTrait that unifies the API is a pretty good idea. But I don't think it's possible to merge Expr with SimpleExpr. To keep things backwards compatible, we can use the inherent macro.

To begin, can you open a PR and add more doc test examples?

[Expurple]

But I don't think it's possible to merge Expr with SimpleExpr.

Can you share the details? From my point of view as a user, all expressions are usable in all expression contexts and there should be just one type. One of those two is probably an implementation detail. But I may be wrong.

To keep things backwards compatible, we can use the inherent macro.

I like this approach, although it won't work for ColumnTrait methods which accept &self (Expr methods accept self). I think, I'd still use inherent for Expr, then implement ColumnTrait: ExprTrait using something like self.into_expr().trait_method(...) and then switch inherent ColumnTrait methods to use something like self.clone().trait_method(...)

To begin, can you open a PR and add more doc test examples?

Just to clarify: we're good with ExprTrait name? It doesn't need to be changed to something like ExpressionTrait, ExpressionLike, Expression, etc?

[tyt2y3]

I would have preferred the abbreviated version. It's common in Rust. Because Expr is not part of the syntax tree, it's just a builder for SimpleExpr.

[Expurple]

Ok, I'll postpone the Expr/SimpleExpr thing for now and make ExprTrait PRs

[Expurple]

@tyt2y3 I opened #791, please have a look