-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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 crash on type alias definition inside dataclass declaration #12792
Conversation
This comment has been minimized.
This comment has been minimized.
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.
Thanks for the fix! Left a few comments.
If you could avoid modifying the symbol table while still fixing the crash, the fix would be less likely to cause trouble in weird edge cases.
mypy/plugins/dataclasses.py
Outdated
node=var, | ||
plugin_generated=True, | ||
) | ||
sym.node = node = var |
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.
Modifying the symbol table in a plugin is a little dangerous, since we can apply the plugin hook multiple times, and the error would generated only on the first run, I think. This might be fine here, but it would be easier to reason about what goes on if we'd just skip processing TypeAlias nodes, similar to what we do with ClassVar below. This wouldn't match runtime behavior, but it would be okay since the definition is invalid in any case. I'm not sure if this would cause other problems, however.
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.
The theoretical issue with this is that skipping the processing of the node means mypy will construct an incorrect __init__
signature. Given the following (weird) dataclass definition:
@dataclass
class Foo:
bar: TypeAlias = int
mypy will infer the following __init__
signature:
def __init__(self) -> None: ...
But the actual signature generated by the runtime is
def __init__(self, bar: type[int] = ...) -> None: ...
But as you say, maybe this doesn't really matter that much, since we've already warned the user that using TypeAlias
inside a dataclass definition isn't properly supported.
I think it would be nice if mypy accepted this: from dataclasses import dataclass
from typing import TYPE_CHECKING
from typing_extensions import TypeAlias
class Foo: ...
@dataclass
class A:
if TYPE_CHECKING:
T: TypeAlias = Foo
else:
T = Foo
x: T
A(x=A.T()) but I get that this is abusing the |
I can see the attraction, but I think this would be really tricky to support. I think it would be really hard for mypy to distinguish between |
766bd38 is a version of the PR that modifies the SymbolTable, and matches the runtime behaviour. I prefer 766bd38, but I'm okay with either :) |
This comment has been minimized.
This comment has been minimized.
1 similar comment
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
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.
Thanks for the updates! I think that skipping the type alias is a fine approach -- this error mode is probably quite rare and it is easy to work around anyway (just move the type alias to an outer scope). Maintainability and simplicity feel more important to me.
Skip processing a type alias node and generate an error. Fixes #12544.
Makes sense! |
Description
Fixes #12544.
At the moment, mypy crashes on the following code:
This is because mypy's
dataclasses
plugin does not currently account for the possibility of aTypeAlias
node being inside a dataclass definition.At runtime, no special treatment is given to fields annotated with
TypeAlias
-- they're treated just like normal instance fields with default values. As such, this PR proposes that mypy:TypeAlias
node inside a dataclass definition (it probably won't have the behaviour the user expects).type[X]
in simple cases such asS: TypeAlias = int
.Any
for anything more complex, such asS: TypeAlias = Callable[[int], str]
.Test Plan
I added tests.