make ExitStack/AsyncExitStack not appear as abstract#15488
make ExitStack/AsyncExitStack not appear as abstract#15488srittau merged 1 commit intopython:mainfrom
ExitStack/AsyncExitStack not appear as abstract#15488Conversation
43c3848 to
737d851
Compare
Hmm, isn't that a bug in those tools? These classes don't have any abstract methods on them. Having ABCMeta as the metaclass isn't sufficient to make a class abstract. Lots of classes have that metaclass but can still be soundly instantiated at runtime |
This comment has been minimized.
This comment has been minimized.
no, it's intended behaviour, most languages with abstract classes work like that: https://play.kotlinlang.org/#eyJ2ZXJzaW9uIjoiMi4zLjEwIiwicGxhdGZvcm0iOiJqYXZhIiwiYXJncyI6IiIsIm5vbmVNYXJrZXJzIjp0cnVlLCJ0aGVtZSI6ImlkZWEiLCJjb2RlIjoiYWJzdHJhY3QgY2xhc3MgQVxuXG5mdW4gbWFpbigpIHtcbiAgICBBKClcbn0ifQ== so the author of the class can indicate: "you shouldn't instantiate this class directly, it's abstract. you should subtype it"
|
that's certainly interesting, but it's not how Python works at runtime and I think type checkers should follow the runtime here. No other type checker has this behaviour -- here's a gist you can load into multiplay: 4abd6426cf1b97d8453e1822236246f9. |
at runtime python doesn't care about type annotations, so to say that we should always be as relaxed as the runtime permits isn't valid to me. i think this is a matter of opinion, i think it is a very valuable addition to the type checker that i have taken advantage of in my work, and the need for it was what motivated me and others to implement it to begin with of course i wouldn't want to corrupt typeshed to appease some bizarre non-standard feature, but for this specific case, it is a simple fix that removes some false positives from these tools |
This comment has been minimized.
This comment has been minimized.
|
I think it's fair to assume that if the user explicitly specifies This change avoids triggering false positives in tools that assume this with minimal downsides. So I don't see why this shouldn't be put in. Besides, this wouldn't be the first tool-specific code in typeshed (or any other stub packages for that matter). Whether or not this behavior should be changed in the typing spec is a different story, but I don't see why that should be required here. |
srittau
left a comment
There was a problem hiding this comment.
This whole thing is quite the mess. The comments on ExitStack and AsyncExitStack reference #7961, but while inheriting from AbstractContextManager[Self] doesn't work, I don't see why we can't just inherit from AbstractContextManager[Any], instead, and keep the overridden functions. I suggest we try this (possibly in a separate PR) before we're adding to the mess.
That said, if that doesn't work, I'm fine with this PR, although we should consolidate the comments for the hacks and clearly explain the problem in the comments, instead of linking to an issue.
in this case the _TExitStack_co = TypeVar("_TExitStack_co", default=ExitStack)
class ExitStack(_BaseExitStack[_ExitT_co], AbstractContextManager[_TExitStack_co]):
class Thing(ExitStack[bool | None, "Thing"]): ...but this seems really clunky |
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
…w `reportEmptyAbstractUsage` rule
…w `reportEmptyAbstractUsage` rule

in basedpyright/pycharm the explicit usage of
ABCMeta/ABCcauses the class to be treated as abstract and report an error. these classes are defined a little different to reality, so they hadABCMeta, i've just moved it to a base class to resolve the problems it was causing