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

Pyright reports redeclaration when using types.DynamicClassAttribute after typeshed change #9620

Closed
tungol opened this issue Dec 22, 2024 · 4 comments
Labels
as designed Not a bug, working as intended bug Something isn't working

Comments

@tungol
Copy link

tungol commented Dec 22, 2024

Describe the bug
Currently, typeshed declares DynamicClassAttribute as an alias of property:

DynamicClassAttribute = property

I'd like to update this to define DynamicClassAttribute as it's own class. The new mypy 1.14.0 supports this change, but pyright is generating reportRedeclaration errors when declaring setters and deleters if this change is made.

Code or Screenshots

See the typeshed MR with the change and a test showing the pyright errors: python/typeshed#13276

test case:

class DCAtest:
    _value: int | None = None

    @types.DynamicClassAttribute
    def foo(self) -> int | None:
        return self._value

    @foo.setter
    def foo(self, value: int) -> None:
        self._value = value

    @foo.deleter
    def foo(self) -> None:
        self._value = None
@tungol tungol added the bug Something isn't working label Dec 22, 2024
@erictraut
Copy link
Collaborator

Pyright (and other type checkers) have significant special casing for property. These special cases are painful to maintain, and don't have plans to add similar special casing for other classes.

I might reconsider this in the future if there's significant signal from pyright users, but type.DynamicClassAttribute is probably never going to be popular enough to justify such special casing.

I'm not sure what prompted the change in typeshed, but it's not clear to me that this change is a net benefit to users.

@erictraut erictraut closed this as not planned Won't fix, can't repro, duplicate, stale Dec 22, 2024
@erictraut erictraut added the as designed Not a bug, working as intended label Dec 22, 2024
@tungol
Copy link
Author

tungol commented Dec 22, 2024

Fair enough. Mypy has a list of property-like classes (currently builtins.property, abc.abstractproperty, functools.cached_property, enum.property, and types.DynamicClassAttribute), and all it took to support this was adding types.DynamicClassAttribute to that list. I was hoping it was similarly easy to gain support in pyright, but it sounds like it wouldn't be. That's too bad, but understandable. Thanks for considering.

I'm not sure what prompted the change in typeshed, but it's not clear to me that this change is a net benefit to users.

To be clear, the motivation for the typeshed change is accuracy to runtime, but it's just a proposed change, not one that's already occurred. I don't think anyone has interest in making DynamicClassAttribute more accurate to runtime as long as it would cause a regression for pyright users.

@tungol
Copy link
Author

tungol commented Dec 23, 2024

I'm not a javascript or typescript programmer, but I took a look at problem anyway. This change fixes this for pyright, without causing any failures in the test suite:

--- a/packages/pyright-internal/src/analyzer/typeEvaluator.ts
+++ b/packages/pyright-internal/src/analyzer/typeEvaluator.ts
@@ -17035,7 +17035,7 @@ export function createTypeEvaluator(
                 classFlags |= ClassTypeFlags.TypingExtensionClass;
             }
 
-            if (node.d.name.d.value === 'property') {
+            if (node.d.name.d.value === 'property' || node.d.name.d.value === 'DynamicClassAttribute') {
                 classFlags |= ClassTypeFlags.PropertyClass;
             }

Would you be willing to consider this as a MR? It seems easy enough to me, but like I said I'm not a typescript programmer and I'm not familiar with the pyright code base, so I don't know if I'm missing anything.

@erictraut
Copy link
Collaborator

Would you be willing to consider this as a MR?

No, this is not a complete fix. I'm not interested in supporting additional special cases here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
as designed Not a bug, working as intended bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants