sqlalchemy / sqlalchemy2-stubs

PEP-484 typing stubs for SQLAlchemy 1.4
MIT License
157 stars 41 forks source link

Incompatible types in assignment when base declaration is in a separate file #264

Open Stacey-Valon opened 8 months ago

Stacey-Valon commented 8 months ago

Scenario that works as expected: test_db.py:

from sqlalchemy import String
from sqlalchemy import Column
from sqlalchemy.orm import declarative_base

Base = declarative_base()
class User(Base):
    __tablename__ = "user"
    name = Column(String)

    def __init__(self, *, name: str):
        self.name = name

Command: pipenv run mypy --follow-imports=skip test_db.py Result: Everything passes, yay! 🎉

Failure scenario: test_db.py:

from sqlalchemy import String
from sqlalchemy import Column
from base_file import Base

class User(Base):
    __tablename__ = "user"
    name = Column(String)

    def __init__(self, *, name: str):
        self.name = name

base_file.py:

Base = declarative_base()

Command plus output:

pipenv run mypy --follow-imports=skip test_db.py
test_db.py:11: error: Incompatible types in assignment (expression has type "str", variable has type "Column[String]")  [assignment]

Versions.

CaselIT commented 8 months ago

Hi,

Are you using the mypy plugin? That may be an issue with that and I'm not sure it's worth fixing

Stacey-Valon commented 8 months ago

Yes I'm using the mypy plugin sqlalchemy.ext.mypy.plugin. There is no way around that I believe? I upgraded to this package from sqlalchemy-stubs because I was under the impression that's how I could fix the RemoveIn20Warnings and have MyPy pass. The old stubs don't seem to recognize the newer 2.0 syntax that 1.4 supports.

The reason I need this pattern is that we have a common base class with some fields defined for every table:

from sqlalchemy import String
from sqlalchemy import Column
from my_base import MyBase

class User(MyBase):
    __tablename__ = "user"
    name = Column(String)

    def __init__(self, *, name: str):
        self.name = name

my_base:

Base = declarative_base()
class MyBase(Base):
     ... common fields defined...
CaselIT commented 8 months ago

There is no way around that I believe?

Don't know honestly.

SQLAlchemy v2 has moved on wrt the plugin, that's now basically deprecated, along with these stubs.

If you want to have use types in any meaningful way I would suggest migrating to v2

Stacey-Valon commented 8 months ago

If you want to have use types in any meaningful way I would suggest migrating to v2

That is definitely the plan, I was just hoping to continue to use the proper types during the migration. Perhaps that won't be possible.

Morikko commented 6 months ago

Yes I'm using the mypy plugin sqlalchemy.ext.mypy.plugin. There is no way around that I believe? I upgraded to this package from sqlalchemy-stubs because I was under the impression that's how I could fix the RemoveIn20Warnings and have MyPy pass. The old stubs don't seem to recognize the newer 2.0 syntax that 1.4 supports.

That is definitely the plan, I was just hoping to continue to use the proper types during the migration. Perhaps that won't be possible.

I am in the kind of same situation. Also to fix the RemoveIn20Warnings, it requires to make changes that break the types from the (old stubs). The idea by using the "not recommended" stubs from this repo would be to:

  1. Keep typing the code during the migration
  2. Reduce the false positives (as the old stubs are mostly 13. based)
  3. Making it closer to sqlalchemy 2.0 typing

The codebase can not quickly upgrade to 2.0 so it has to live in this grey area for quite some time as it is big.


I also tried to bypass the inferred types by using Mapped but I got this:

Incompatible types in assignment (expression has type "Column[Text]", variable has type "Mapped[str]")
CaselIT commented 6 months ago

I guess you could try doing something like this, not sure if it will help:

if TYPE_CHECKING:
  Base = declarative_base()
else:
  from base_file import Base

@Morikko not sure, upgrading a 1.4 codebase to v2 should not require too much effort, since all the typing related changes are all optional and can be added later on (or never)

Morikko commented 6 months ago

After a second analysis, I found the root cause. Initially, the code used the declarative way declarative_base() but it used to crash mypy.

The solution was to use the class way directly, eg the transformation the plugin is doing. (Creating an Explicit Base Non-Dynamically (for use with mypy, similar)

class Base(metaclass=DeclarativeMeta):
   ...

However, the mapping was not working because the plugin is very picky on using the exact name while the code was using:

class DeclarativeABCMeta(sqlalchemy.orm.decl_api.DeclarativeMeta, abc.ABCMeta):
   ...

class Base(metaclass=DeclarativeABCMeta):
   ...

In the meanwhile, I managed to fix the issues related to the declarative way usage so the mappings are generated as expected again.

So it is a bit different from the original issue where the --follow-imports=skip is causing the issue.