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

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

Closed
Deimos opened this issue Jan 5, 2018 · 14 comments
Closed
Labels
bug mypy got something wrong topic-descriptors Properties, class vs. instance attributes

Comments

@Deimos
Copy link
Contributor

Deimos commented Jan 5, 2018

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
Copy link
Member

Can you include the content of test.py?

@Deimos
Copy link
Contributor Author

Deimos commented Jan 5, 2018

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
Copy link
Member

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
Copy link
Collaborator

JukkaL commented Jan 5, 2018

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.

@JukkaL JukkaL added the bug mypy got something wrong label Jan 5, 2018
@lincolnq
Copy link
Contributor

lincolnq commented Sep 4, 2018

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
Copy link
Member

@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
Copy link
Contributor

ckarnell commented Sep 11, 2019

@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
Copy link
Member

@ckarnell I don't think it is possible with the current plugin API. This would require either #7468 or #6760 (depending on how exactly we implement these).

@marc-mabe
Copy link

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

jace added a commit to hasgeek/funnel that referenced this issue Oct 20, 2020
@ennnas
Copy link

ennnas commented Feb 18, 2021

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
Copy link

@ennnas I'm getting

error: Name "TYPE_CHECKING" is not defined

back. How do you get this variable?

@hauntsaninja
Copy link
Collaborator

from typing import TYPE_CHECKING

@TornaxO7
Copy link

Thank you! :)

@emmatyping
Copy link
Collaborator

It seems that sqlalchemy is deprecating their mypy plugin. Unfortunately, that means it is unlikely for this issue to be solvable in a neat way. That being said, another plugin could eventual implement either #7468 or #6760, to fix this.

That being said, I don't think this issue is actionable beyond the above plugin issues, so I am going to close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong topic-descriptors Properties, class vs. instance attributes
Projects
None yet
Development

No branches or pull requests