python / mypy

Optional static typing for Python
https://www.mypy-lang.org/
Other
18.42k stars 2.82k forks source link

"Already defined" + "not defined" errors with SQLAlchemy 1.2 hybrid_property #4430

Open Deimos opened 6 years ago

Deimos commented 6 years ago

SQLAlchemy 1.2 was recently released, and changed its behavior of hybrid_property a bit to line up with how Python's @property works. Specifically, it's now necessary to use the same method name for both the getter and setter, as seen in the example here (both are def name(...)): http://docs.sqlalchemy.org/en/latest/changelog/migration_12.html#hybrid-attributes-support-reuse-among-subclasses-redefinition-of-getter

This is causing mypy to error though, it now emits two (contradictory?) errors on the @name.setter line:

test.py:11: error: Name 'name' already defined
test.py:11: error: Name 'name' is not defined
gvanrossum commented 6 years ago

Can you include the content of test.py?

Deimos commented 6 years ago

Oh sorry, it's just exactly the same as that example code (only the first class def):

from sqlalchemy import Base, Column, String
from sqlalchemy.ext.hybrid import hybrid_property

class FirstNameOnly(Base):
    first_name = Column(String)

    @hybrid_property
    def name(self):
        return self.first_name

    @name.setter
    def name(self, value):
        self.first_name = value

I had the wrong line numbers on the error above (edited now), the error is on line 11, @name.setter.

gvanrossum commented 6 years ago

I guess this is because mypy doesn't understand that @hybrid_property because similar to @property.

Also possibly you may have to take this up with https://github.com/JelleZijlstra/sqlalchemy-stubs.

JukkaL commented 6 years ago

Since there are no stubs for sqlalchemy by default, the decorator will be seen as Any value by mypy. One option would be to not complain about redefinitions if the first definition is known to have an Any type.

More generally, hybrid_property could be an alias for property, and in that case the errors are more clearly false positives.

lincolnq commented 6 years ago

Guido suggested in #220 that someone could try to build hybrid property support on top of mypy's descriptor support. I was able to get this going by defining a typed_hybrid_property generic class (in a pyi file), parameterized by python type and sql type. I had to define my hybrid properties as separate functions with different names (like _prop_get, _prop_expr and such), and then merge them all into the name I wanted using prop = hybrid_property(_prop_get, expr=_prop_expr).

I have so far been unable to convince mypy to type-check the multipart definition (using @prop.setter) because that relies on the magic in mypy's semantic analyzer which notices @property declarations.

It would be super-nice if mypy provided a way to "get the @property behavior" for additional decorators not named in the mypy source.

ilevkivskyi commented 6 years ago

@lincolnq This is something that probably can be done by a plugin. We currently have a plugin hook for classes decorated with a given decorator. We can probably just add a similar hook for functions.

Btw, we are currently working on SQLAlchemy stubs (and soon mypy plugins) at https://github.com/dropbox/sqlalchemy-stubs

ckarnell commented 5 years ago

@ilevkivskyi Do you have an idea of how a plugin could be used to silence these errors mentioned above if you use a .setter or .expression?:

test.py:11: error: Name 'name' already defined
test.py:11: error: Name 'name' is not defined

It sounds like the support for @property is baked into mypy itself, so I'm not sure how we would use a plugin to avoid these errors.

ilevkivskyi commented 5 years ago

@ckarnell I don't think it is possible with the current plugin API. This would require either https://github.com/python/mypy/issues/7468 or https://github.com/python/mypy/issues/6760 (depending on how exactly we implement these).

marc-mabe commented 4 years ago

I was running into the same problem with two hybrid fields. I was able to fix the error: Name 'XXX' already defined by adding # type: ignore and interestingly also fix the other issue by just reordering the functions.

Original:

class Channel(db.Model):
    __tablename__ = 'Channel'
    id = db.Column('id', db.Integer, nullable=False, primary_key=True)
    names = db.relationship(
        ChannelName,
        primaryjoin=ChannelName.channel_id == id,
        backref='channel',
        order_by="desc(ChannelName.default)"
    )

    @hybrid_property
    def name(self) -> Optional[ChannelName]:
        return self.names[0] if len(self.names) else None

    @name.expression
    def name(cls):
        return select([ChannelName.name]) \
            .where(ChannelName.channel_id == cls.id) \
            .limit(1) \
            .label('name')

    @hybrid_property
    def aliases(self) -> List[ChannelName]:
        return self.names[1:]

    @aliases.expression
    def aliases(cls):
        return select([ChannelName.name]) \
            .where(ChannelName.channel_id == cls.id) \
            .offset(1) \
            .label('aliases')

def on_change(channel: Channel):
    if channel.name:
        channel.name.default = True
    for alias in model.aliases:
        alias.default = False

Error;

error: Name 'name' already defined on line XXX
error: Name 'aliases' already defined on line XXX
error: overloaded function has no attribute "default"
error: overloaded function has no attribute "__iter__" (not iterable)

Modified to:

class Channel(db.Model):
    __tablename__ = 'Channel'
    id = db.Column('id', db.Integer, nullable=False, primary_key=True)
    names = db.relationship(
        ChannelName,
        primaryjoin=ChannelName.channel_id == id,
        backref='channel',
        order_by="desc(ChannelName.default)"
    )

    @hybrid_property  # type: ignore
    def name(self) -> Optional[ChannelName]:
        return self.names[0] if len(self.names) else None

    @hybrid_property  # type: ignore
    def aliases(self) -> List[ChannelName]:
        return self.names[1:]

    @name.expression  # type: ignore
    def name(cls):
        return select([ChannelName.name]) \
            .where(ChannelName.channel_id == cls.id) \
            .limit(1) \
            .label('name')

    @aliases.expression  # type: ignore
    def aliases(cls):
        return select([ChannelName.name]) \
            .where(ChannelName.channel_id == cls.id) \
            .offset(1) \
            .label('aliases')

def on_change(channel: Channel):
    if channel.name:
        channel.name.default = True
    for alias in model.aliases:
        alias.default = False

Result:

Success: no issues found in 9 source files
ennnas commented 3 years ago

Another solution that I found and that doesn't require using #type: ignore is to make the hybrid_property have the same typing as a normal property when type checking.

from sqlalchemy import Base, Column, String

if TYPE_CHECKING:
    # This makes hybrid_property's have the same typing as normal property until stubs are improved.
    hybrid_property = property
else:
    from sqlalchemy.ext.hybrid import hybrid_property

class FirstNameOnly(Base):
    first_name = Column(String)

    @hybrid_property
    def name(self):
        return self.first_name

    @name.setter
    def name(self, value):
        self.first_name = value

reference

TornaxO7 commented 3 years ago

@ennnas I'm getting

error: Name "TYPE_CHECKING" is not defined

back. How do you get this variable?

hauntsaninja commented 3 years ago

from typing import TYPE_CHECKING

TornaxO7 commented 3 years ago

Thank you! :)