| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
There was a problem hiding this comment.
Thanks Carl! I'm flattered to get a PR from you here. ✨
Thanks for the detailed explanation, super clear.
This will be available in SQLModel 0.0.38, released in the next few hours. 🚀
Sorry, something went wrong.
|
Wow, awesome, thank you @tiangolo for the speedy merge and release! It will be great to get the __new__ feature out to ty users without regressing behavior for SQLModel users. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Hello!
I work on the ty type checker, and I am implementing support for the part of the typing spec regarding __new__ methods. I noticed that implementing the specification will result in a regression for SQLModel users; it causes all construction of instances of SQLModel model classes to be inferred as Any. For example:
The reason for this is that the typing spec says that if a __new__ method is explicitly annotated to return Any, that annotation should override the usual constructor type inference, and be taken as-is. (See the sentence in the https://typing.python.org/en/latest/spec/constructors.html#new-method section which begins "For purposes of this test,".)
And SQLModel.__new__ is currently annotated to explicitly return Any.
As of this writing, Pyrefly type checker also has this behavior of inferring Any for all SQLModel instances. Mypy and pyright do not, for different reasons. Mypy does not follow the specified handling of constructors; in fact it doesn't even support __new__ ever returning a non-instance type. Pyright does follow the specified handling of constructors, but its dataclass-transform support acts as if the dataclass transformation of a SQLModel subclass synthesizes and adds a __new__ method, which overrides SQLModel.__new__, rendering it irrelevant. This is not accurate to what actually happens at runtime (no __new__ method is synthesized), so I would not like to take this route in ty.
I don't see a good option for how to handle this in ty while both conforming to the typing spec and avoiding this Any inference. So I'd like to suggest that the simplest solution is to remove the Any return annotation on SQLModel.__new__.
There are two options for how to replace it. This PR takes the minimally risky approach of just removing the return annotation altogether. This is semantically the same as an -> Any annotation, except that it doesn't override the normal constructor behavior like an Any annotation does. The only downside is that it requires suppressing no-untyped-defs on that line.
The other option would be to use -> Self, which I believe is accurate and should work well, but it is a stricter annotation than Any, so could carry some risk of new type errors in user code. (I haven't deeply investigated the actual consequences, so this is just "in theory".)
(Personally, I don't like that the specification mandates different behavior for an explicit Any return vs "no annotation" -- I believe those should be treated the same. But it may be difficult to change the spec in this area, given possible backward-compatibility concerns.)
Thanks for considering this PR!