Skip to content

make ExitStack/AsyncExitStack not appear as abstract#15488

Merged
srittau merged 1 commit intopython:mainfrom
KotlinIsland:ExitStack
Apr 1, 2026
Merged

make ExitStack/AsyncExitStack not appear as abstract#15488
srittau merged 1 commit intopython:mainfrom
KotlinIsland:ExitStack

Conversation

@KotlinIsland
Copy link
Copy Markdown
Contributor

@KotlinIsland KotlinIsland commented Mar 4, 2026

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

@KotlinIsland KotlinIsland force-pushed the ExitStack branch 2 times, most recently from 43c3848 to 737d851 Compare March 4, 2026 23:59
@AlexWaygood
Copy link
Copy Markdown
Member

in basedpyright/pycharm the explicit usage of ABCMeta/ABC causes the class to be treated as abstract and report an error

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

@github-actions

This comment has been minimized.

@KotlinIsland
Copy link
Copy Markdown
Contributor Author

KotlinIsland commented Mar 5, 2026

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

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"

image

@AlexWaygood
Copy link
Copy Markdown
Member

no, it's intended behaviour, most languages with abstract classes work like that

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.

@KotlinIsland
Copy link
Copy Markdown
Contributor Author

KotlinIsland commented Mar 6, 2026

but it's not how Python works at runtime and I think type checkers should follow the runtime here.

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

@github-actions

This comment has been minimized.

@jorenham
Copy link
Copy Markdown
Contributor

jorenham commented Mar 9, 2026

I think it's fair to assume that if the user explicitly specifies metaclass=ABCMeta on a class, that they've done this with the intention of marking the class as abstract, regardless of whether there are @abstractmethods present.
That's why these tools consider instantiating such classes to be a likely source of issues, or at least a code smell.
And yes, it's possible to do at runtime. But that doesn't mean that it's good idea to rely on it. Ruff, for example, reports errors in cases like this all the time.

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.

Copy link
Copy Markdown
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

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.

@KotlinIsland
Copy link
Copy Markdown
Contributor Author

I don't see why we can't just inherit from AbstractContextManager[Any]

in this case the Any wins over the Self in the subtype, making it really untyped. the only viable solution i can think of is to add a parameter to ExitStack:

_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

@github-actions

This comment has been minimized.

@KotlinIsland KotlinIsland requested a review from srittau April 1, 2026 09:12
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 1, 2026

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@srittau srittau merged commit e01edb5 into python:main Apr 1, 2026
51 checks passed
DetachHead added a commit to DetachHead/basedpyright that referenced this pull request Apr 1, 2026
DetachHead added a commit to DetachHead/basedpyright that referenced this pull request Apr 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants