-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix: Generic definition #14583
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
fix: Generic definition #14583
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I haven't looked at the changes in depth, but the fact that type checkers seem happy with it and the primer output indicates that this change has a positive impact look promising. But I'd like defer this decision to maintainers with more knowledge of typing internals. Maybe @JelleZijlstra or @AlexWaygood? |
|
We'd have to make changes to ty if this change was made in typeshed; it causes quite a few of ty's tests to start failing if you just naively apply this patch to ty's vendored copy of typeshed: The patch to fix ty after making this change isn't very complicated, though, so I wouldn't let that block that PR if it brings typeshed closer to the runtime semantics here: diff --git a/crates/ty_python_semantic/src/types/infer.rs b/crates/ty_python_semantic/src/types/infer.rs
index a1a3cb2e7b..483d6493be 100644
--- a/crates/ty_python_semantic/src/types/infer.rs
+++ b/crates/ty_python_semantic/src/types/infer.rs
@@ -4667,20 +4667,11 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
}
// Handle various singletons.
- if let Type::NominalInstance(instance) = declared.inner_type() {
- if instance
- .class(self.db())
- .is_known(self.db(), KnownClass::SpecialForm)
+ if let Some(name_expr) = target.as_name_expr() {
+ if let Some(special_form) =
+ SpecialFormType::try_from_file_and_name(self.db(), self.file(), &name_expr.id)
{
- if let Some(name_expr) = target.as_name_expr() {
- if let Some(special_form) = SpecialFormType::try_from_file_and_name(
- self.db(),
- self.file(),
- &name_expr.id,
- ) {
- declared.inner = Type::SpecialForm(special_form);
- }
- }
+ declared.inner = Type::SpecialForm(special_form);
}
}Ideally I feel like we'd just say that Looking at the primer report:
It definitely seems like a win that this change means that type checkers understand that The only last thing I'd like to check -- I'll kick off a draft PR on ty's CI to see if the ty diff I posted above has any performance implications for us. |
|
Yes, this seems surprisingly low-impact. I don't see why there has to be a change between pre-3.12 and post-3.12 version. |
|
I kicked off an experimental ty PR here to see if it has any impact on our mypy_primer report for ty users, or any performance impact for us: astral-sh/ruff#19969 |
| AnyStr = TypeVar("AnyStr", str, bytes) # noqa: Y001 | ||
|
|
||
| @type_check_only | ||
| class _Generic: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just class Generic: without the type[] part? It's a class at runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, pyright was complaining a lot without this workaround 😕
I think I misremembered from a previous version of this PR, the current version looks good in that regard. |
|
ty's CI does report a 5% perf regression as a result of the changes we'd have to make to accomodate this change, but only on one microbenchmark that's really there just to check that we don't have truly pathological performance on code that's very badly written. The primer report on the typeshed PR indicates that this wouldn't have the same benefits right now to ty users that it has to mypy users. But I think that's just because ty doesn't really type-check calls to |
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
|
Diff from mypy_primer, showing the effect of this PR on open source code: beartype (https://github.com/beartype/beartype)
+ beartype/_util/hint/pep/proposal/pep484/pep484generic.py:149: error: Unused "type: ignore" comment [unused-ignore]
+ beartype/_util/hint/pep/proposal/pep484/pep484generic.py:177: error: Unused "type: ignore" comment [unused-ignore]
pydantic (https://github.com/pydantic/pydantic)
- pydantic/main.py:580: error: Argument 2 to "issubclass" has incompatible type "<typing special form>"; expected "_ClassInfo" [arg-type]
- pydantic/dataclasses.py:203: error: Argument 2 to "issubclass" has incompatible type "<typing special form>"; expected "_ClassInfo" [arg-type]
- pydantic/dataclasses.py:205: error: Incompatible types in assignment (expression has type "tuple[type[DataclassInstance], <typing special form>]", variable has type "tuple[type[DataclassInstance]]") [assignment]
+ pydantic/dataclasses.py:205: error: Incompatible types in assignment (expression has type "tuple[type[DataclassInstance], Any]", variable has type "tuple[type[DataclassInstance]]") [assignment]
strawberry (https://github.com/strawberry-graphql/strawberry)
+ strawberry/utils/typing.py:135: error: Unused "type: ignore" comment [unused-ignore]
+ strawberry/utils/typing.py:185: error: Unused "type: ignore" comment [unused-ignore]
discord.py (https://github.com/Rapptz/discord.py)
- ...typeshed_to_test/stdlib/typing.pyi:1016: note: "update" of "TypedDict" defined here
+ ...typeshed_to_test/stdlib/typing.pyi:1029: note: "update" of "TypedDict" defined here
- ...typeshed_to_test/stdlib/typing.pyi:1016: note: "update" of "TypedDict" defined here
+ ...typeshed_to_test/stdlib/typing.pyi:1029: note: "update" of "TypedDict" defined here
- discord/ext/commands/converter.py:1280: error: Argument 2 to "issubclass" has incompatible type "<typing special form>"; expected "_ClassInfo" [arg-type]
- discord/ext/commands/hybrid.py:508: error: Overlap between argument names and ** TypedDict items: "name", "description" [misc]
+ discord/ext/commands/hybrid.py:508: error: Overlap between argument names and ** TypedDict items: "description", "name" [misc]
- discord/ext/commands/hybrid.py:834: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
+ discord/ext/commands/hybrid.py:834: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
- discord/ext/commands/hybrid.py:858: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
+ discord/ext/commands/hybrid.py:858: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
- discord/ext/commands/bot.py:290: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
+ discord/ext/commands/bot.py:290: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
- discord/ext/commands/bot.py:314: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
+ discord/ext/commands/bot.py:314: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
|
Unless there is a specific reason to use
_SpecialFormannotation, I'm proposing a slightly more refined way to defineGeneric:_SpecialFormsemantically prohibit subclassing whereasGeneric[...]can be subclassedProtocolif in the future its definition is also updated from_SpecialForm)Based on cpython spec,
Generichas a custom__class_getitem__that should be in the class template:>>> set(dir(Generic)) - set(dir(object)) {'__module__', '__class_getitem__'}