Skip to content
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

Improve handling of attribute access on class objects #14988

Merged
merged 3 commits into from
Jun 16, 2023

Conversation

AlexWaygood
Copy link
Member

Fixes #14056

#14056 was originally reported as a mypyc issue (because that's how it presented itself to the user), but the underlying bug is really a bug to do with how mypy understands metaclasses. On mypy master:

class Meta(type):
    bar: str

class Foo(metaclass=Meta):
    bar: int

reveal_type(Foo().bar)  # Revealed type is int (correct!)
reveal_type(Foo.bar)  # Revealed type is int, but should be str

This PR fixes that incorrect behaviour.

Since this is really a mypy bug rather than a mypyc bug, I haven't added a mypyc test, but I'm happy to if that would be useful. (I'll need some guidance as to exactly where it should go -- I don't know much about mypyc internals!)

if (
isinstance(node.node, Var)
and not node.node.is_classvar
and not hook
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and not hook was required here, or this test started to fail:

[case testClassAttrPluginMetaclassRegularBase]
# flags: --config-file tmp/mypy.ini
from typing import Any, Type
class M(type):
attr = 'test'
class B:
attr = None
class Cls(B, metaclass=M):
pass
reveal_type(Cls.attr) # N: Revealed type is "builtins.int"
[file mypy.ini]
\[mypy]
plugins=<ROOT>/test-data/unit/plugins/class_attr_hook.py

Comment on lines +329 to 333
# In typeshed's stub for builtins.pyi, __annotations__ is `dict[str, Any]`,
# but it's inferred as `Mapping[str, object]` here due to the fixture we're using
reveal_type(X.__annotations__) # N: Revealed type is "typing.Mapping[builtins.str, builtins.object]"

[builtins fixtures/dict.pyi]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test needed to be altered slightly, as with this change, mypy starts inferring the correct type of X.__annotations__ according to the fixture that this test uses:

class type:
__annotations__: Mapping[str, object]

Comment on lines +4485 to +4487
bar: ClassVar[str] = 'bar'
eggs: str
spam: ClassVar[str] = 'spam'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could argue that bar and spam here violate LSP (since setting a class variable on a class is ~basically the same as setting an instance variable on a metaclass). This PR doesn't change the behaviour there, however: mypy emits no errors here on master, and doesn't with this PR applied either.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Mar 31, 2023

Mypy_primer analysis:

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but this isn't an area of mypy that I'm familiar with.

Maybe a test case involving a generic metaclass would be helpful.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Apr 14, 2023

Thanks for the review, appreciate it!

Maybe a test case involving a generic metaclass would be helpful.

The new behaviour doesn't work with generic metaclasses. But I don't think that's really anything to do with this PR: mypy in general hates generic metaclasses:

from typing import TypeVar, Generic

T = TypeVar("T")

class Meta(type, Generic[T]):
    attr: T

class Foo(metaclass=Meta[int]): ...  # error: Dynamic metaclass not supported for "Foo"  [misc]

https://mypy-play.net/?mypy=latest&python=3.11&gist=f360b7b7366c4618ac0b6d79f0e404b9

Mypy emits the same "dynamic metaclasses not supported" error with this PR, so I think users will have fair warning that what they're trying to do isn't supported, if they do something like that. I'm not sure if a test would be helpful here or not, since I guess ideally mypy would support generic metaclasses, and I don't want to assert incorrect or undesirable behaviour in a test.

@ikonst
Copy link
Contributor

ikonst commented Apr 14, 2023

I tried to reproduce the steam error but couldn't yet. Here's where I stopped: https://mypy-play.net/?mypy=latest&python=3.11&gist=fdd3d72f616b8ce786c6c7f8bc2a79fd

@github-actions
Copy link
Contributor

github-actions bot commented Jun 1, 2023

Diff from mypy_primer, showing the effect of this PR on open source code:

steam.py (https://github.com/Gobot1234/steam.py)
- steam/id.py:123: error: Access to generic instance variables via class is ambiguous  [misc]

pandera (https://github.com/pandera-dev/pandera)
+ pandera/api/pandas/model.py:416: error: Need type annotation for "attrs" (hint: "attrs: Dict[<type>, <type>] = ...")  [var-annotated]

discord.py (https://github.com/Rapptz/discord.py)
- discord/ext/commands/cog.py:358: error: Incompatible types in assignment (expression has type "Group | discord.app_commands.commands.Command[Cog, [VarArg(Any), KwArg(Any)], Any]", variable has type "discord.ext.commands.core.Command[Self, [VarArg(Any), KwArg(Any)], Any]")  [assignment]
+ discord/ext/commands/cog.py:358: error: Incompatible types in assignment (expression has type "Group | discord.app_commands.commands.Command[Any, [VarArg(Any), KwArg(Any)], Any]", variable has type "discord.ext.commands.core.Command[Self, [VarArg(Any), KwArg(Any)], Any]")  [assignment]

@ilevkivskyi ilevkivskyi merged commit 21cc1c7 into python:master Jun 16, 2023
@AlexWaygood AlexWaygood deleted the metaclass-instance-vars branch June 16, 2023 20:25
@AlexWaygood
Copy link
Member Author

Thanks @ilevkivskyi and @JelleZijlstra!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mypyc: Cls.__dict__ is mappingproxy, not dict
4 participants