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

Issue 6810 type annotate sqlalchemy.sql.elements #5

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

jazzthief
Copy link

Type annotations for sql.elements (sqlalchemy#6810)

Description

  • apart from sql.elements, added a small annotation to sql.base to fix a few untyped calls

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 jazzthief self-assigned this Jan 9, 2023
Comment on lines +2553 to +2585
def self_group(
self, against: Optional[OperatorType] = None
) -> Union[Grouping[TextClause], TextClause]:
Copy link
Author

@jazzthief jazzthief Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self_group() has a lot of overrides with potentially conflicting signatures. It is possible to narrow some types down with @overloads matching self and return types, but that seems overkill, so most of the time I annotated the return with superclass ColumnElement or ClauseElement. Here it was possible to narrow types down without tripping mypy.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inside ColumnElement there is an implementation of self_group() with two @overloads that seem redundant - but mypy is happy. Would be nice to ask the author about them, because I cannot understand what's going on there

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think those overloads are needed to cover both cases where we return just self (L1402) without loosing information about the contained type and where we return self wrapped into other classes which conceal self's contained type (L1398) and (L1400). Not sure if it's actually the case but it's the best idea I can come up with off the top of my head.

@overload
def self_group(
self: ColumnElement[_T], against: Optional[OperatorType] = None
) -> ColumnElement[_T]:
...
@overload
def self_group(
self: ColumnElement[Any], against: Optional[OperatorType] = None
) -> ColumnElement[Any]:
...
def self_group(
self, against: Optional[OperatorType] = None
) -> ColumnElement[Any]:
if (
against in (operators.and_, operators.or_, operators._asbool)
and self.type._type_affinity is type_api.BOOLEANTYPE._type_affinity
):
return AsBoolean(self, operators.is_true, operators.is_false)
elif against in (operators.any_op, operators.all_op):
return Grouping(self)
else:
return self

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a fair point and might indeed be what the author intended, but I fail to see how exactly is this going to work technically. Normally, Mypy would choose the best matching overload, and if there are several candidates, it chooses whichever one comes first in the declaration. However, if there is Any among the candiates - Mypy just infers Any. So here, when Mypy resolves ColumnElements type argument, why would it ever pick ColumnElement[_T]?

@@ -4122,7 +4238,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
Copy link
Author

@jazzthief jazzthief Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calls an external function within_group_type(), which returns None by default, in which case normal type is returned; otherwise WithinGroup.type() has to return the result of within_group_type(), which is unknown - it is probably meant to be overridden by some other classes that require such behaviour. That's why mypy complains about returning Any here - guessing I have to just # type: ignore that. It is also

  • an untyped call
  • incompatible with ColumnElement

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

@property
def _is_star(self):
def _is_star(self) -> bool:
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes when there is an attribute overridden with a @property mypy would complain about it, despite it seemingly being a good practice. This might be due to a mypy bug, however here a guy makes a point about how this might become a legitimate bug and suggests a few fixes. All of them require modification of the source, and I cannot find any other fixes - have to ask the author

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a thread from Mike with a bit more info on mypy's relationship with @property. It also looks related to this comment regarding incompatible definitions in base classes.

@@ -3836,7 +3926,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
Copy link
Author

@jazzthief jazzthief Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (from inside pylance) stub. I assume this should not work this way. Found something on typeshed here, will try to find a solution.

@@ -1503,6 +1517,7 @@ def proxy_set(self) -> FrozenSet[ColumnElement[Any]]:

@util.memoized_property
def _expanded_proxy_set(self) -> FrozenSet[ColumnElement[Any]]:
# type: ignore [no-untyped-call]
return frozenset(_expand_cloned(self.proxy_set))
Copy link
Author

@jazzthief jazzthief Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Untyped call to sql.base:_expand_cloned(). I think I can see which types it needs, but left it as-is for now and removed the ignore

@@ -2499,21 +2543,25 @@ def columns(self, *cols, **types):
)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Untyped call to selectable.TextualSelect

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fixed with Dmitry's PR.

@@ -2527,7 +2575,7 @@ class Null(SingletonConstant, roles.ConstExprRole[None], ColumnElement[None]):
_singleton: Null

@util.memoized_property
def type(self):
def type(self) -> NullType:
Copy link
Author

@jazzthief jazzthief Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mypy says it is incompatible with supertype ColumnElement, which (provided TYPE_CHECKING == True) has an attribute type: TypeEngine[_T] defined. Putting the same return type doesn't work. Possibly a decorator problem? Same goes for other type() overrides returning Boolean and TypeEngine[_T]. Might be a @property problem again, but mypy gives a different error here

@@ -4307,11 +4429,11 @@ def _tq_label(self) -> Optional[str]:
return self._gen_tq_label(self.name)

@HasMemoized.memoized_attribute
def _render_label_in_columns_clause(self):
def _render_label_in_columns_clause(self) -> bool:
Copy link
Author

@jazzthief jazzthief Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another attribute override, this time mypy says it's incompatible with supertype. This isn't the only case of such error. Annotating the superclass attribute with bool doesn't do anything

return True

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as _render_label_in_columns_clause() here. Thinking about these decorators now - seems like this problem only occurs in decorated functions

Copy link

@g-kisenkov g-kisenkov Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the error is: Signature of "_non_anon_label" incompatible with supertype "ColumnElement" [override] mypy.

If you look at the signature of the method on ColumnElement and replace @HasMemoized.memoized_attribute on child method with @property the error is gone.

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

Any other combination leaves the error in place, so looks like there's currently no way to type annotate such cases without rethinking how methods are defined. So just # type: ignore [override] this and other cases and let the author now the problem in the future PR.

sub_element = fn(*arg, **kw)
if sub_element is not self._element:
return Label(self.name, sub_element, type_=self.type)
else:
return self

@property
def primary_key(self):
def primary_key(self) -> bool:
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another writable attribute override issue. Same goes for foreign_key() below

Comment on lines +4815 to +4887
def _compare_name_for_result(
self, other: ColumnElement[Any]
) -> Union[bool, FrozenSet[ColumnElement[Any]]]:
Copy link
Author

@jazzthief jazzthief Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returns bool normally, but has to return Union[bool, FrozenSet] here. Writing a single @overload makes mypy unhappy - it requires at least two. Could be solved by adding the Union to all 3 definitions, but that's a bit misleading - there is even a comment saying specifically that the function returns bool. Not sure how to proceed

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I'm afraid in such case there are only 2 ways. Either split the method onto 2 different methods (it looks like being a part of private API, so then you'd have to update all of the internal calls to it) or update parent signatures like that:

diff --git a/lib/sqlalchemy/sql/elements.py b/lib/sqlalchemy/sql/elements.py
index 10a63ecc4..49b18aefa 100644
--- a/lib/sqlalchemy/sql/elements.py
+++ b/lib/sqlalchemy/sql/elements.py
@@ -1537,7 +1537,7 @@ class ColumnElement(
 
         return bool(self.proxy_set.intersection(othercolumn.proxy_set))
 
-    def _compare_name_for_result(self, other: ColumnElement[Any]) -> bool:
+    def _compare_name_for_result(self, other: ColumnElement[Any]) -> Union[bool, FrozenSet[ColumnElement[Any]]]:
         """Return True if the given column element compares to this one
         when targeting within a result row."""
 
@@ -4382,7 +4382,7 @@ class NamedColumn(KeyedColumnElement[_T]):
     name: str
     key: str
 
-    def _compare_name_for_result(self, other: ColumnElement[Any]) -> bool:
+    def _compare_name_for_result(self, other: ColumnElement[Any]) -> Union[bool, FrozenSet[ColumnElement[Any]]]:
         return (hasattr(other, "name") and self.name == other.name) or (
             hasattr(other, "_label") and self._label == other._label
         )

@jazzthief jazzthief marked this pull request as ready for review January 10, 2023 11:28
@jazzthief
Copy link
Author

jazzthief commented Jan 10, 2023

Note: Tuple in this module's context is an SQLAlchemy class representing an SQL tuple - it is defined right here, in sql.elements. The regular tuple from typing is aliased as typing_Tuple. However, PyAnnotate calls a regular tuple Tuple, which caused a lot of errors. Hopefully, I resolved all of them - still, need to pay extra attention to this.

@@ -430,7 +444,7 @@ def _with_binary_element_type(self, type_):
return self

@property
def _constructor(self):
def _constructor(self) -> Any:
Copy link
Author

@jazzthief jazzthief Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.__class__ returns Type[ClauseElement], however this return type is incompatible with SupportsWrappingAnnotations supertype - putting SupportsWrappingAnnotations in return doesn't help either.
Inside SupportsWrappingAnnotations, when TYPE_CHECKING == True, _constructor is actually an attribute with Callable[..., SupportsWrappingAnnotations] type - so even with Any return type I get yet another writable attribute override with read-only property.

Comment on lines 426 to 427
negated_op: Callable[..., Any],
original_op: Callable[..., Any],
Copy link
Author

@jazzthief jazzthief Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like that could be narrowed down to OperatorType, will look into that

Copy link

@g-kisenkov g-kisenkov Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's a good idea to do it. I think you can safely narrow it down for any *_op parameter.

return self

def _negate(self):
def _negate(self) -> Union[AsBoolean, False_, True_]:
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calls AsBoolean with self.operator as the third argument. Mypy says Argument 3 to "AsBoolean" has incompatible type "Optional[OperatorType]"; expected "OperatorType", despite having no Optional annotations in the __init__ method.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's defined as optional in its parent UnaryExpression class. I guess mypy still treats it as optional here since _negate could be called in places where parent class is expected and hence break LSP.

def comparator(self):
return self.type.comparator_factory(self)
def comparator(self) -> NullType.Comparator[Any]:
return self.type.comparator_factory(self) # type: ignore [arg-type]
Copy link
Author

@jazzthief jazzthief Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mypy: Argument 1 has incompatible type "TextClause"; expected "ColumnElement[_T]" - complete mystery

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comparator_factory: _ComparatorFactory[Any] = Comparator

class _ComparatorFactory(Protocol[_T]):
def __call__(self, expr: ColumnElement[_T]) -> TypeEngine.Comparator[_T]:
...

But TextClause doesn't inherit from ColumnElement. Maybe it's worth to loosen expr: ColumnElement[_T] up to ClauseElement?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will probably not change anything in sql.type_api, but this is helpful - will definitely leave it in a comment

Comment on lines +3937 to +4024
def self_group(
self, against: Optional[OperatorType] = None
) -> ClauseElement:
Copy link
Author

@jazzthief jazzthief Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mypy puts an error at Grouping class definition and complains about group_self() in base classes GroupedElement and ColumnElement being incompatible. Can't find anything to put here to get it work without breaking something else.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, since the 2 parent classes clash with LSP when they're both inherited in a single subclass there's no way but to either simply class Grouping(GroupedElement, ColumnElement[_T]): # type: ignore [misc] or to generalize the return type of self_group inside ColumnElement up to ClauseElement and update all of the dependent code throughout the codebase. Or rearchitect the whole mechanism altogether. ¯\_(ツ)_/¯

return self._apply_to_inner(self._element.self_group, against=against)

def _negate(self):
def _negate(self) -> ColumnElement[_T]:
Copy link
Author

@jazzthief jazzthief Jan 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incompatible with supertype ColumnElement. ColumnElement._negate() has overloads with matching self to return types with ColumnElement[bool] and ColumnElement[_T] - tried using Label[_T], which didn't work. Mypy is happy with AsBoolean._negate() override, for example, so it might be because of different inheritance. Maybe the right way is to just stop worrying about private methods so much

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can open the return type a little (ColumnElement[Any]). You can also try to create a MRE to figure out what exactly is wrong with this case and maybe come up with a better solution. But I think the simple one will do.

@@ -3540,7 +3604,7 @@ def _order_by_label_element(self) -> Optional[Label[Any]]:
def _from_objects(self) -> List[FromClause]:
return self.element._from_objects

def _negate(self):
def _negate(self) -> ClauseElement:
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the "incompatible with supertype ColumnElement" cases with _negate(). Can't do much here, have to return ClauseElement from the body (ClauseElement._negate() is annotated by the author), and however I change it, it refuses to work with the supertype.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, not much can be done here besides opening up the parent class' type hint up to ClauseElement as well. Or refactoring the whole structure. :D So just silencing it up with # type: ignore [override] will do.

@@ -1831,7 +1845,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]:
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mypy reports incompatible signature with supertype ColumnElement. The signature is the same, tried changing the @property to @HasMemoized.memoized_attribute and that removed the error. Will look closer at decorated overrides that currently raise errors.

Copy link

@g-kisenkov g-kisenkov Jan 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it could be solved with typing the decorator @HasMemoized.memoized_attribute someway like this or that.

If it works maybe its worth applying the same strategy to the rest of the decorator-related problems in this PR.

@g-kisenkov
Copy link

To consider: Use type aliases for clarity

def _gen_cache_key(
self,
anon_map: cache_anon_map,
bindparams: List[BindParameter[_T]],
) -> Optional[
Union[
typing_Tuple[str, Type[BindParameter[_T]]],
typing_Tuple[
str,
Type[BindParameter[_T]],
Union[CacheConst, typing_Tuple[Any, ...]],
str,
bool,
],
]
]:

Copy link

@g-kisenkov g-kisenkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'm done with reviewing the PR. Try addressing all of my comments so far the best you can and ship it off to upstream repo. You can also link this PR in the upstream PR and reference any unsolved cases left so the author could use more context if he needs to.

As for calls to untyped functions that are left, I'm not sure if it's worth to partially type-annotate the rest of the modules or just leave it as is and addressed in the future iterations when the PR is merged. If the upstream maintainers don't want PR's containing partial updates to secondary modules leave it as is.

Comment on lines 4358 to 4419
self, against: Optional[OperatorType] = None
) -> ColumnElement[_T]:
assert against is not None
if operators.is_precedent(operators.filter_op, against):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess against might be None, so I suggest here not an assertion, but adding against to the condition: if against and operators.is_precedent.....

@jazzthief jazzthief force-pushed the issue_6810_type_annotate_sqlalchemy.sql.elements branch 2 times, most recently from ec9ac64 to b31e72c Compare January 24, 2023 13:55
@jazzthief jazzthief force-pushed the issue_6810_type_annotate_sqlalchemy.sql.elements branch from 2d84434 to cb068fd Compare March 14, 2023 14:34

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wanted to check-out GitHub's new file-level comments feature. :D

Anyways, I'll do a review of the changes tomorrow, if you need it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, well, it's a bit late for that - Mike has taken over it already. I had to push a rebase because main has gone pretty far forward, so sorry if you got an alert

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

Successfully merging this pull request may close these issues.

None yet

3 participants