Skip to content

Add a py.typed file? #1112

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

Closed
AlexWaygood opened this issue Oct 4, 2022 · 15 comments
Closed

Add a py.typed file? #1112

AlexWaygood opened this issue Oct 4, 2022 · 15 comments
Milestone

Comments

@AlexWaygood
Copy link

Hi! It looks like the latest release of Flask-SQLAlchemy ships with inline type annotations. This is awesome!

Would you consider adding a py.typed file to the library? This would mean that we could mark our typeshed stubs for Flask-SQLAlchemy as officially obsolete, and not have to worry about updating our stubs to reflect the changes in 3.0.* (cf. python/typeshed#8849).

@davidism
Copy link
Member

davidism commented Oct 4, 2022

Both MyPy and PyRight have various issues that would be annoying for users if I exported the types, so for now they are used internally only. Neither project seemed like they were going to fix them, MyPy has them sitting open for years, PyRight has them closed.

@AlexWaygood
Copy link
Author

That makes sense. We can continue to maintain our typeshed stubs for now. Thanks!

@AlexWaygood
Copy link
Author

The inline types should make our job at typeshed substantially easier anyway, so this is definitely still a win for us :)

@davidism
Copy link
Member

davidism commented Oct 4, 2022

I'm confused how the types work any better in typeshed.

Both MyPy and PyRight don't allow subclassing db.Model, they don't like that it's a class on an instance object.

PyRight also doesn't understand import sqlalchemy as sa; import sqlalchemy.orm; sa.orm, it refuses to add namespaces when using an import alias. microsoft/pyright#3884 This would make the internal code more verbose, and doesn't match a pattern SQLAlchemy itself uses, although it doesn't affect users.

Would be great if you as a member of typeshed could revisit the linked issues.

@AlexWaygood
Copy link
Author

Unfortunately, I don't know much about Flask-SQLAlchemy -- which was partly why I was interested in seeing whether we could maybe mark our typeshed stubs as obsolete :)

Could you maybe give a complete example of Flask-SQLAlchemy code that doesn't work with mypy, so that I can play around with it and offer some suggestions?

(I'm not looking for a "minimal example" exactly -- I'm looking for an example that's as small as possible, but which still uses Flask-SQLAlchemy code (in an idiomatic way) to reproduce the problem. I'm not looking for an example that uses non-Flask-SQLAlchemy code to reproduce the problem.)

@davidism
Copy link
Member

davidism commented Oct 4, 2022

from flask_sqlalchemy import SQLAlchemy

db = SQLAlchemy()

class User(db.Model):
    id = db.Column(db.Integer, primary_key=True)

MyPy and PyRight both show errors on the class line because of db.Model.

@AlexWaygood
Copy link
Author

I'll see if I can have a play around.

@davidism
Copy link
Member

davidism commented Oct 4, 2022

I would love to export the types by the way, I just don't want to end up making things more annoying for users.

@AlexWaygood
Copy link
Author

Okay, I took a look. Unfortunately, this is quite a difficult problem to solve :(

First up: this pattern just isn't very typing-friendly, unfortunately. That's not a criticism of this pattern -- it's just a thing that we have to deal with here. Type checkers are always going to, by default, reject having a dynamic variable as a base class. That's because it violates some fundamental tools type checkers use to do their job. Type checkers need to be able to construct precise mros internally, so that they can work out which methods various classes have and which attribute various classes have, etc. It's reasonable for type checkers to emit a warning if a user tries to inherit from a dynamic variable, to warn the user that things aren't going to work as expected (from a typing perspective) if that's something you're doing.

However, type checkers are pretty happy for users to inherit from statically defined class variables. Type checkers are also (providing you don't have the absolute strictest settings applied) happy for you to inherit from Any1. For example, this code type-checks fine with mypy:

from typing import Any
from typing_extensions import TypeAlias

class Foo:
    X: TypeAlias = Any

class Bar(Foo.X): ...

I was hoping we might be able to do some kind of hack where we pretended to mypy that Model was a class variable, even though it isn't at runtime. Something like this:

diff --git a/src/flask_sqlalchemy/extension.py b/src/flask_sqlalchemy/extension.py
index 3fc2689..c8c16e6 100644
--- a/src/flask_sqlalchemy/extension.py
+++ b/src/flask_sqlalchemy/extension.py
@@ -23,6 +23,11 @@ from .query import Query
 from .session import _app_ctx_id
 from .session import Session

+if t.TYPE_CHECKING:
+    # mypy thinks typing_extensions is part of the stdlib,
+    # even though that isn't true at runtime
+    import typing_extensions as te
+

 class SQLAlchemy:
     """Integrates SQLAlchemy with Flask. This handles setting up one or more engines,
@@ -120,6 +125,9 @@ class SQLAlchemy:
         Added the ``session_options`` parameter.
     """

+    if t.TYPE_CHECKING:
+        Model: te.TypeAlias = t.Any
+
     def __init__(
         self,
         app: Flask | None = None,
@@ -190,7 +198,8 @@ class SQLAlchemy:
             This is a subclass of SQLAlchemy's ``Table`` rather than a function.
         """

-        self.Model = self._make_declarative_base(model_class)
+        if not t.TYPE_CHECKING:
+            self.Model = self._make_declarative_base(model_class)
         """A SQLAlchemy declarative model class. Subclass this to define database
         models.

However... that still doesn't work. If you apply that diff, then mypy is fine with this code, where Model is accessed as an attribute on the class object:

from flask_sqlalchemy import SQLAlchemy

db = SQLAlchemy()

class User(SQLAlchemy.Model):
    id = db.Column(db.Integer, primary_key=True)

But mypy still isn't happy with the original snippet you gave me, where Model is accessed from the instance object -- and this is despite the fact that we're lying to mypy, and pretending that Model is a class attribute:

from flask_sqlalchemy import SQLAlchemy

db = SQLAlchemy()

class User(db.Model):  # error: Name "db.Model" is not defined
    id = db.Column(db.Integer, primary_key=True)

I think mypy could be more flexible here. If mypy believes that Model is a class variable, then it shouldn't be any less type-safe to inherit from Model-accessed-as-an-attribute-on-the-instance-object than it is to inherit from Model-accessed-as-an-attribute-on-the-class-object. (The error message is, of course, also terrible, and should definitely be improved.)

But yeah. I can't see a workaround here, even a hacky workaround, without either changing mypy or changing Flask-SQLAlchemy idioms. Sorry to be the bearer of bad news here :(


1(Note that inheriting from `Any` itself doesn't actually work at runtime on Python <3.11.)

@AlexWaygood
Copy link
Author

AlexWaygood commented Oct 5, 2022

I'm confused how the types work any better in typeshed.

It doesn't look like they do. It looks like mypy emits exactly the same error for the snippet in #1112 (comment) if it uses typeshed's stubs for Flask-SQLAlchemy. My best guess is that users of the typeshed stubs probably just type: ignore the type-checker complaints away whenever they use that idiom.

@davidism
Copy link
Member

davidism commented Oct 6, 2022

Do you think it's better for us to export the types directly then, since either way users are getting the same experience in MyPy?

@AlexWaygood
Copy link
Author

Do you think it's better for us to export the types directly then, since either way users are getting the same experience in MyPy?

I think that's probably the best option for users? Mypy emits this error even if you have an environment without inline types or the typeshed stubs. It just really doesn't like classes inheriting from instance attributes:

from foo import bar  # type: ignore[import]

reveal_type(bar)  # Revealed type is "Any"
baz = bar()

class Spam(baz.X): ...  # error: Name "baz.X" is not defined

It would also make our life a lot easier at typeshed if you exported the types, since none of us are Flask-SQLAlchemy experts.

But I can also totally understand if you don't want to add the py.typed file -- I can see how that could be seen as "taking responsibility" for the types in some way, and I definitely don't want to pressure anybody to declare their library as py.typed if it's not something they're ready for or interested in.

@davidism davidism reopened this Oct 6, 2022
@davidism davidism added this to the 3.0.1 milestone Oct 6, 2022
@AlexWaygood
Copy link
Author

Thanks @davidism!

@baggiponte
Copy link

Just to make sure I understood: the code snippet below is invalid because (even if it removes the warning) it breaks inheritance, am I right?

from typing import Type
from flask_sqlalchemy import SQLAchemy

db = SQLAlchemy()

SQLAlchemyModel = Type[db.Model]


class User(SQLAlchemyModel):
    ...  

@davidism
Copy link
Member

That doesn't look like valid code, no. Please use another place for questions about Python typing, not this issue tracker.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

3 participants