Type annotations for sqlalchemy.sql.elements #9143

jazzthief · 2023-01-25T14:18:21Z

Description

Added type annotations for sql.elements (issue #6810).
There is also a tiny change to sql.base - it solves a problem in elements.

Checklist

This pull request is:

A documentation / typographical error fix
- Good to go, no issue or tests are needed
A short code fix
- please include the issue number, and create an issue if none exists, which
  must include a complete example of the issue. one line code fixes without an
  issue and demonstration will not be accepted.
- Please include: Fixes: #<issue number> in the commit message
- please include tests. one line code fixes without tests will not be accepted.
A new feature implementation
- please include the issue number, and create an issue if none exists, which must
  include a complete example of how the feature would look.
- Please include: Fixes: #<issue number> in the commit message
- please include tests.

Have a nice day!

jazzthief · 2023-01-25T14:19:53Z

lib/sqlalchemy/sql/elements.py

@@ -1837,7 +1853,7 @@ def _dedupe_anon_label_idx(self, idx: int) -> str:
            return self._dedupe_anon_tq_label_idx(idx)

    @property
-    def _proxy_key(self):
+    def _proxy_key(self) -> Optional[str]:


Changing @property to @HasMemoized.memoized_attribute (superclass definition has it) removes the error. This isn't the only case, guessing there's an access-related conflict somewhere between @property, @HasMemoized.memoized_attribute and @util.memoized_property from mypy's point of view.

the @property situation with mypy is very difficult. it's not in general safe to change a @property to a memoized so I'd have to look at this.

jazzthief · 2023-01-25T14:20:24Z

lib/sqlalchemy/sql/elements.py

        return True

    @HasMemoized.memoized_attribute
-    def _non_anon_label(self):
+    def _non_anon_label(self) -> Optional[str]:


Replacing @HasMemoized.memoized_attribute with @property resolves the error.

jazzthief · 2023-01-25T14:20:58Z

lib/sqlalchemy/sql/elements.py

+        self, against: Optional[OperatorType] = None
+    ) -> ClauseElement:
+        if (
+            against


Some of the self_group() overrides require against argument because of a call to is_precedent(), which conflicts with Optional arg annotation - added against to the conditional to deal with that without changing the function signature.

jazzthief · 2023-01-25T14:24:44Z

lib/sqlalchemy/sql/elements.py

@@ -2223,13 +2264,13 @@ def _select_iterable(self) -> _SelectIterable:
    _allow_label_resolve = False

    @property
-    def _is_star(self):
+    def _is_star(self) -> bool:


Mypy doesn't like overriding writable attribute with read-only property. My first thought was that it's mypy's general weirdness with @property, or a bug - but looks like this has been fixed. Here a guy makes a point about how this might become a legitimate problem. Anyway, not sure how to proceed, and this isn't the only case.

jazzthief · 2023-01-25T14:26:22Z

lib/sqlalchemy/sql/elements.py

@@ -4155,7 +4267,7 @@ def over(self, partition_by=None, order_by=None, range_=None, rows=None):
        )

    @util.memoized_property
-    def type(self):
+    def type(self) -> TypeEngine[_T]:
        wgt = self.element.within_group_type(self)
        if wgt is not None:
            return wgt


within_group_type() returns None by default, it's likely meant to be overridden - so mypy infers Any for wgt here.

jazzthief · 2023-01-25T14:28:02Z

lib/sqlalchemy/sql/elements.py

@@ -3869,7 +3955,7 @@ def __init__(self, start, stop, step, _name=None):
        )
        self.type = type_api.NULLTYPE

-    def self_group(self, against=None):
+    def self_group(self, against: Optional[OperatorType] = None) -> Slice:
        assert against is operator.getitem


Getting a non-overlapping identity check in the assert. operator.getitem() is interpreted as an overloaded function by mypy. Hovewer the overloads it sees are from typeshed-fallback/stdlib/_operator.pyi stub. Guess it shouldn't work that way

jazzthief · 2023-01-25T14:29:47Z

lib/sqlalchemy/sql/elements.py

        return self

-    def _negate(self):
+    def _negate(self) -> Union[AsBoolean, False_, True_]:


In _negate() return mypy infers Optional[OperatorType] for the 3rd argument to AsBoolean when OperatorType is expected. There is no Optional in AsBoolean.__init__, so guessing mypy treats it that way because of superclass UnaryExpression Optional annotation.

jazzthief · 2023-01-25T14:32:34Z

lib/sqlalchemy/sql/elements.py

-    def columns(self, *cols, **types):
+    def columns(
+        self, *cols: ColumnElement[Any], **types: TypeEngine[_T]
+    ) -> util.preloaded.sql_selectable.TextualSelect:


Line 2546: mypy expects List[ColumnClause[Any]] for TextualSelect.__init__ - as per recently merged sql.selectable annotations. However, a columns() description says its *cols argument is "a series of _expression.ColumnElement objects, typically". Not sure which is the best way to resolve this.

jazzthief · 2023-01-25T14:34:11Z

Lines 3150-3151: for and_/or_ mypy complained about no such attribures here, looked like it didn't like BooleanClauseList.__init__ - according to Mike's previous comments, removed __init__ return annotation altogether and that fixed it. Hope that's ok, and that comment didn't refer to one particular case.

jazzthief · 2023-01-25T14:39:25Z

Didn't # type: ignore anything so that it's quicker to see all the issues. There are quite a few problems with overrides incompatible with supertype, and decorators issues which might require some changes to the source - which I stayed away from.

Also, mypy showed a few issues inside sql.selectable - didn't seem like I could fix any of these, will look into it some more.

zzzeek · 2023-01-25T16:03:32Z

a lot of these require that I think pretty long and hard about what should be done, so I would say if you have most everything annotated here, just hand off to me and I can figure out what to do with the thorny issues because these are right at the core of things I really had to spend a lot of time getting to work originally, and there may need to be bigger changes.

jazzthief · 2023-01-25T16:36:35Z

@zzzeek Sure - that's my thoughts exactly, so I didn't touch a lot of these. Still, I am interested in knowing how some of these issues are going to be resolved, especially @property/decorator thing - feels like it is important to understand. I'll keep an eye on the module, please hit me up if you need anything.

CaselIT · 2023-03-13T19:02:20Z

Hi @jazzthief sorry for the delay. Can you update the change to remove the conflicts?

thanks!

jazzthief · 2023-03-14T14:38:00Z

@CaselIT The conflicts are resolved, left other changes as-is like we discussed with Mike above

CaselIT · 2023-03-14T19:19:04Z

great thank you.

I'll move it to gerrit

sqla-tester

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision cb068fd of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2023-03-14T19:19:38Z

New Gerrit review created for change cb068fd: https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4512

sqla-tester · 2023-03-14T19:23:29Z

Federico Caselli (CaselIT) wrote:

Mike I think this is at the point where you can take it over to fix the last type errors

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4512

sqla-tester

mike bayer (zzzeek) wrote:

just some notes so far

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4512

sqla-tester · 2023-03-16T21:45:47Z

lib/sqlalchemy/sql/elements.py

@@ -80,12 +79,20 @@
 from ..util.typing import Self

 if typing.TYPE_CHECKING:
+    from re import Match
+
+    from mypy_extensions import NoReturn


mike bayer (zzzeek) wrote:

NoReturn should be in typing or sqlalchemy.util.typing

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4512

sqla-tester · 2023-03-16T21:45:47Z

lib/sqlalchemy/sql/elements.py

+
+    from mypy_extensions import NoReturn
+
+    from ._py_util import cache_anon_map


mike bayer (zzzeek) wrote:

import this from visitors

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4512

sqla-tester · 2023-03-16T21:45:47Z

lib/sqlalchemy/sql/elements.py

+        anon_map: cache_anon_map,
+        bindparams: List[BindParameter[_T]],
+    ) -> Optional[
+        Union[


mike bayer (zzzeek) wrote:

this whole thing I think overall is just Tuple[Any], it wasn't intended to be specific to what's in the tuple.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4512

sqla-tester · 2023-03-16T21:45:47Z

lib/sqlalchemy/sql/elements.py

+        self, against: Optional[OperatorType] = None
+    ) -> ClauseElement:
+        if (
+            against


mike bayer (zzzeek) wrote:

this is a logic change

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4512

sqla-tester · 2023-03-16T21:45:47Z

lib/sqlalchemy/sql/elements.py

        if (
-            self.group
+            against


mike bayer (zzzeek) wrote:

this looks like a logic change

there's a lot of sudden test failures so I'd guess these changes are why

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4512

You're right, removed it

sqla-tester

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision c85b4b9 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision c85b4b9 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision c85b4b9 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2023-03-26T01:53:39Z

Patchset c85b4b9 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4512

sqla-tester

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision c85b4b9 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision c85b4b9 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2023-03-26T01:55:49Z

Patchset c85b4b9 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4512

jazzthief commented Jan 25, 2023

View reviewed changes

jazzthief marked this pull request as ready for review January 25, 2023 16:22

jazzthief added 16 commits March 14, 2023 15:17

Apply some runtime types, start fixing errors

ebf5e73

Allow untyped calls; fix some more types

5ec44bb

Fix more types

710c178

Fix some more types

2ca8fc9

Fix some more types; focus on method overrides

7e6036f

Disallow untyped calls; fix more types

0de1535

Add return annotation to fix untyped calls from sql.elements

b119b9e

Clean up

ef6ca74

Clean up comments

3a135f7

Refine some types; clean up

28f43c9

Remove remaining ignores; refine some types

ccad2f0

Refine more types

85aeecc

More type fixes

85b8101

Fix some of remaining errors

bdc1f76

Replace asserts with conditionals

1951e75

Remove explicit None check

2bc6140

jazzthief added 2 commits March 14, 2023 15:24

Narrow down a problematic type

e705f39

Use Self instead of a TypeVar

cb068fd

jazzthief force-pushed the issue_6810_type_annotate_sqlalchemy.sql.elements branch from 2d84434 to cb068fd Compare March 14, 2023 14:34

CaselIT requested a review from sqla-tester March 14, 2023 19:19

sqla-tester reviewed Mar 14, 2023

View reviewed changes

sqla-tester reviewed Mar 16, 2023

View reviewed changes

jazzthief added 2 commits March 24, 2023 15:40

Address comments

34561d3

Update cache_anon_map import

c85b4b9

CaselIT requested a review from sqla-tester March 25, 2023 17:42

sqla-tester reviewed Mar 25, 2023

View reviewed changes

sqla-tester reviewed Mar 26, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Type annotations for sqlalchemy.sql.elements #9143

Type annotations for sqlalchemy.sql.elements #9143

jazzthief commented Jan 25, 2023

jazzthief Jan 25, 2023

zzzeek Jan 25, 2023

jazzthief Jan 25, 2023

jazzthief Jan 25, 2023

jazzthief Jan 25, 2023

jazzthief Jan 25, 2023

jazzthief Jan 25, 2023

jazzthief Jan 25, 2023

jazzthief Jan 25, 2023

jazzthief commented Jan 25, 2023

jazzthief commented Jan 25, 2023 •

edited

zzzeek commented Jan 25, 2023

jazzthief commented Jan 25, 2023

CaselIT commented Mar 13, 2023

jazzthief commented Mar 14, 2023

CaselIT commented Mar 14, 2023

sqla-tester left a comment

sqla-tester commented Mar 14, 2023

sqla-tester commented Mar 14, 2023

sqla-tester left a comment

sqla-tester Mar 16, 2023

sqla-tester Mar 16, 2023

sqla-tester Mar 16, 2023

sqla-tester Mar 16, 2023

sqla-tester Mar 16, 2023

jazzthief Mar 24, 2023

sqla-tester left a comment

sqla-tester left a comment

sqla-tester left a comment

sqla-tester commented Mar 26, 2023

sqla-tester left a comment

sqla-tester left a comment

sqla-tester commented Mar 26, 2023


		from mypy_extensions import NoReturn

		from ._py_util import cache_anon_map

Type annotations for sqlalchemy.sql.elements #9143

Are you sure you want to change the base?

Type annotations for sqlalchemy.sql.elements #9143

Conversation

jazzthief commented Jan 25, 2023

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jazzthief commented Jan 25, 2023

jazzthief commented Jan 25, 2023 • edited

zzzeek commented Jan 25, 2023

jazzthief commented Jan 25, 2023

CaselIT commented Mar 13, 2023

jazzthief commented Mar 14, 2023

CaselIT commented Mar 14, 2023

sqla-tester left a comment

Choose a reason for hiding this comment

sqla-tester commented Mar 14, 2023

sqla-tester commented Mar 14, 2023

sqla-tester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sqla-tester left a comment

Choose a reason for hiding this comment

sqla-tester left a comment

Choose a reason for hiding this comment

sqla-tester left a comment

Choose a reason for hiding this comment

sqla-tester commented Mar 26, 2023

sqla-tester left a comment

Choose a reason for hiding this comment

sqla-tester left a comment

Choose a reason for hiding this comment

sqla-tester commented Mar 26, 2023

jazzthief commented Jan 25, 2023 •

edited