-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Regression: descriptor not working in protocol #19054
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
Comments
This was an intentional change, to make descriptor access more consistent, see this PR comment for more motivation #18831 (comment). IMO the code in the example is a bug, such pattern should be reserved for when the actual attribute type is a descriptor, otherwise one should just use |
Ok, let me play around with this a little. The impact to the Dropbox internal codebase (where I found this) is very minor, so if using |
I couldn't find a way to make a concrete class that uses the descriptor to be compatible with the protocol: import typing
A = typing.TypeVar('A')
class Attribute(typing.Generic[A]):
@typing.overload
def __get__(self, instance: None, owner: typing.Any) -> Attribute[A]: ...
@typing.overload
def __get__(self, instance: object, owner: typing.Any) -> A: ...
def __get__(self, instance, owner): ...
class PathArg(typing.Protocol):
@property # Attempt to use @property since the descriptor is read only
def path(self) -> str: ...
class C:
path = Attribute[str]()
def f(arg: PathArg) -> None:
s: str = arg.path
def g(c: C) -> None:
reveal_type(c.path) # "str", as expected
f(c) # error: C is not compatible with PathArg <<-- this should be fine? Is there a way to modify the old example so that it works using the new semantics? |
Another use case where the new behavior may cause issues is when using import typing
A = typing.TypeVar('A')
class Attribute(typing.Generic[A]):
@typing.overload
def __get__(self, instance: None, owner: typing.Any) -> Attribute[A]: ...
@typing.overload
def __get__(self, instance: object, owner: typing.Any) -> A: ...
def __get__(self, instance, owner): ...
class PathArg(typing.Protocol):
path: Attribute[str] # no error with Attribute[str], but error when using str
class C:
path = Attribute[str]()
def f(arg: type[PathArg]) -> None:
reveal_type(arg.path) # Attribute[str]
def g() -> None:
f(C) # error if using "path: str" in PathArg |
OK, I understand what happened (i.e. why someone defined such weird protocol). The problem is that protocols never worked with descriptors, see #5481, so someone found this workaround, and now this workaround doesn't work anymore. I think the best way forward is to just wait for the proper descriptor support added in #18943. I you are really worried about this, you can revert it in the release branch. But I wouldn't bother, because IMO this is really niche workaround. |
This only impacts two files in the Dropbox codebase, so it's not a big deal. As you said, the code is kind of questionable anyway, so this shouldn't block the release. |
After #18831 this example started generating an error, which looks like a regression:
If
PathArg
is a regular class, there is no error.cc @ilevkivskyi as the author of the PR
The text was updated successfully, but these errors were encountered: