refactor (Expr): add generics to make Expr properties type safe (POC proposal)

[OutSquareCapital]

Abstract

After +10 typing PR's, I realize now that the best ROI going forward would be to improve the typing of the Expr class in itself as much as possible, rather than handling each call sites like I did in this PR.
This is WIP, but I already put this here so we can discuss if that solution is a good one, or if another one would be preferrable.

1) Motivation

Currently, having Expr::{this, expression} completely untyped with Any as return types has multiple problems:

• A poor developper experience for chained .this.expression.this calls, with zero autocompletion
• The need to spam # type: ignore comments at each call site if you use a strict type checker
• mypc can't optimize the callers sites
• If we want to add type annotations in the codebase, each call site need to be annotated if there's access to Expr properties, which is a LOT more work than if the definition site become type safe by design
• Unsafe code is not flagged. For example, expressions::core::Alias::this is across the code assumed to always be an Expr, but in the tests they can be None. Or expressions::functions::Cast::this, also assumed to be an Expr, but in generators::postgres::cast_sql we give it a str. Thus, it's hard to know when or why we should assume a given return type.

2) Implementation

The basic idea is to introduce a new class, expressions::core::ExprTyped[ET, EE], where the generic type ET represent the return type of Expr::this, and EE the return type of Expr::expression.
This class only herit from Expression, consider it an intermediate step.
On the long run, Expression could become generic in itself, which would avoid this step.

ET = t.TypeVar("ET")
EE = t.TypeVar("EE")


class ExprTyped(Expression, t.Generic[ET, EE]):
    """Expression with typed "this" (ET) and "expression" (EE) args."""

    @property
    def this(self) -> ET:
        return super().this

    @property
    def expression(self) -> EE:
        return super().expression

2.1) Changes

All the "actual" changes lives in the expressions folder.
Two additionnal TypeVar, and the new ExprTyped class are the only new code.
Otherwise, all other changes are Expression heritage replaced by ExprTyped.

The other impacted files outside of this folder are minor required changes to make mypy happy, and are explained in this PR comments.
The changes are quite large, but it's just to check if this could work at a large scale.

I think that a first, small PR that focuses on the most impactful classes with precises types would be better for review than this massive chunk. Later on, more complex classes, like the ones for which there are runtime incoherences, could be implemented one by one.

2.2) Pros and cons

2.2.1) Pros

• Not disruptive -> This doesn't impact old code, nor is needed for new implementations
• Don't had as much boilerplate, unlike a solution like overrides for each Expr subclasses would. This is only a new class, and a change of heritage.
• Has no runtime impact
• Can be done incrementally.

2.2.2) Cons

• No contract enforcement. But this is due to the actual design, and no solution that don't directly change the Expr constructor/kwargs/args attribute will be able to adress this
• Add some complexity to the Expr classes hierarchy -> one more intermediate class
• Is not "connected" to a static logic. You can't "see" why an ExprType[str, None] is str and None at the definition site.
• Does not solve the Expr::args::get typing. If that could be done, it would instantly solve a LOT of casting, implicit Any, and strict type checker errors.

2.2) Alternative solutions

IMO, the best solution would be a total refactor with concrete attributes for each Expr subclass, and no more args at all. But this is a massive, breaking one. I'm not opposed to try it tough. I'm just not sure how much you guys would be interested in it. But it would solve a lot of the typing problems by design.
Another option would be to use overrides for each property. This would result in the same outcome, but with like 10x the boilerplate.

3) Impact

For the expressions::core::Between class:

class Between(ExprTyped[Expr, None], Predicate):
    arg_types = {"this": True, "low": True, "high": True, "symmetric": False}

We now benefit from correct type inference when accessing the properties:

This clean up 550 errors from strict basedpyright, and as of now for the branch, can make fully type safe some nested .this.expression.this usages, which is a huge win IMO.

This also revealed a few unsafe code usage, i.e patterns where unsafe access to Expr.this.some_expr_attribute is done for example, accross the code, but then at some point in the tests or the dialect specific implementations we deal with a Expr::this that don't return an Expr.

Note that in those cases, it wasn't possible to make the Expr subclass herit from ExprTyped.

A few examples from the expressions folder:

• query::{Query, CTE}
• string::{Left, Right}
• functions::Cast
• core::Alias

4) Disclaimer

I came up with the architectural solution, naming, etc...
I asked Claude to "implement" the ExprTyped generic classes for each child class that could be determined by him statically. Then if mypy passes, to me it's valid. Otherwise, I either fixed the callers sites when it was pure typing considerations, or dropped the implementation to avoid too many changes

[OutSquareCapital]

This is needed, otherwise mypy fail. Since in_src can be Any if first is not an exp::PivotAlias, using this union was the strictest solution. We could also use directly Any

[OutSquareCapital]

Will cause mypy fail if not explicit

[tobymao]

this looks really complicated, what exactly is the value with it, i'm not sure i want a complicated generic type system

[OutSquareCapital]

this looks really complicated, what exactly is the value with it, i'm not sure i want a complicated generic type system

Currently, having Expr::{this, expression} completely untyped with Any as return types has multiple problems:

• A poor developper experience for chained .this.expression.this calls, with zero autocompletion
• The need to spam type: ignore comments at each call site if you use a strict type checker
• mypc can't optimize the callers sites
• If we want to add type annotations, each call site need to be annotated, which is a LOT more work than if the definition site become type safe by design
• Unsafe code is not flagged. For example, expressions::core::Alias::this is across the code assumed to always be an Expr, but in the tests they can be None. Or expressions::functions::Cast::this, assumed to be an Expr, but in generators::postgres::cast_sql we give it a str. Thus, it's hard to know when or why we should assume a given return type. Note that due to this I haven't typed Alias, and that others Expression subclasses weren't typed due to similar issues.

About the complexity, may I ask what you find complex and that could be simplified? Basically this is just an additional intermediate class ExprTyped with two generic types, that's it. Adding overloads to Expr::{this, expression} would achieve the same thing with like 10x the LOC

[OutSquareCapital]

Needed to avoid mypy failure, since date_with_offset is an implicit union, and mypy don't handle this very well. Better to simply use it as the parent class.

[OutSquareCapital]

Needed to avoid mypy failure. modifier first is an Expr then it's a str. This make both the code hard to read (personal opinion) and in any case isn't accepted by mypy nor basedpyright.

[OutSquareCapital]

Needed to avoid mypy issues.

sqlglot\transforms.py:351: error: Incompatible types in assignment (expression has type "Any | Expr", variable has type "Unnest")  [assignment]

Same issue than in generators::sqlite::dateadd_sql, first an Unnest, then an Expr.

[OutSquareCapital]

After some toughts, decided to close this for the time being.

My uni exams are in 1 month, and I won't have the time to handle a massive change like this one until they are finished.

Furthermore, the downside of "no contract enforcement" is too fragile, as the multiple failed CI runs can show.

Finally, continuing on the "small incremental changes" for a while can be beneficial, as this could make a major shift like this one with less frictions, as more "local issues" may be resolved until then.

[georgesittas]

@OutSquareCapital sounds good, thanks for the heads up. Good luck with your exams!