Can we make AbstractContextManager.__exit__ concrete?

Would it be feasible to make AbstractContextManager.__exit__ concrete?

While __exit__ is decorated with @abstractmethod in the runtime, it is also implemented. This is because cooperative multiple inheritance induces subclasses to call super in __exit__ to ensure that all parent classes handle their exit behaviour as well. The base class’s empty definition makes it so that child classes can blindly call super.

Unfortunately, the decoration of this method with @abstractmethod in both the runtime and the typeshed fools some type checkers and linters into understandably thinking that calling super will be an error.

The typeshed cannot be changed without the runtime being changed.

In general, marking methods abstract makes sense to force users to implement them. But in this case, the method also follows an “augmenting pattern” whereby child classes are typically supposed to call super, and therefore the base class has an empty stub implementation. Whereas most abstract methods simply raise NotImplementedError.

So, I wonder if it would be possible to remove the abstract method decoration in a future version of Python?

1 Like

There has been a discussion just recently how type checkers should treat this case: Calling abstract methods

I think this is a bug in type checker behavior, not a problem with the implementation.

1 Like

I think the problem, as it relates to typing, is that AbstractContextManager is not a protocol, probably because it was introduced in Python 3.6 whereas protocols were introduced in Python 3.8. I’d say that changing the type to be a protocol would make the most sense here, but that would probably be backwards incompatible so that’s pretty much a no-go.

There was a discussion about adding a “this method must be overridden but should not be considered abstract”-method decorator recently (might have been on this forum or in a gh issue tracker), and IIRC that discussion concluded that calling abstract methods through super() should be considered correct behaviour. If that conclusion is correct then pyright should not error in this case. I tried the example from your pyright issue and it works in mypy.

EDIT: David found the link to the discussion I mentioned above.

I agree that this would fix things, however there are some notable advantages to having base classes:

  • runtime instance checking is faster,
  • runtime instance checking is more precise, and
  • intent to implement is clearly specified.

Personally, I think we should prefer base classes to protocols, but I understand your point.

That’s a very cool idea, and would be a fine solution to this.

Nice find! So my suggestion is the same as Guido’s I guess?

Okay, I guess my preference would be to remove this odd decoration. I don’t think we do it in any other similar place? I don’t think I’ve ever written a method that I decorated with @abstractmethod that didn’t also raise in its body.

Preference doesn’t really play into it, abstractmethod specifically documents that having an implementation is a supported pattern. so type checkers need to be able to deal with it: abc — Abstract Base Classes — Python 3.12.1 documentation

Note: Unlike Java abstract methods, these abstract methods may have an implementation. This implementation can be called via the super() mechanism from the class that overrides it. This could be useful as an end-point for a super-call in a framework that uses cooperative multiple-inheritance.

1 Like

Fair enough. By that definition, PyRight should not be emitting an error. Is it worth removing the decoration from the typeshed then?

No, it shouldn’t be changed because instantiating AbstractContextManager is a runtime error, and so is instantiating any subclass that doesn’t override __exit__. You could argue about changing AbstractContextManager itself in the stdlib. But typeshed should match what the stdlib uses.

1 Like

Fair enough, so you think PyRight should not be emitting an error, right?

Yes, exactly.

The discussion linked above came to the conclusion that type checkers should differentiate between implemented and non-implemented abstractmethods and only emit an error if the function is called and the body is trivial. What exactly is trivial currently depends on the type checker, but AFAIK this behavior is already implemented in both pyright and mypy, but with different rules.

However, I don’t know how this maps stub files and therefore typeshed. Adding a non-trivial body (like return) isn’t really in the spirit of .pyi files.

Yeah I think in case of pyi file the type checker should probably not emit an error on calls through super() since it can’t know whether or not it has a non-trivial implementation.

Either that or we add a simple way to mark an abstractmethod as having a non-trivial implementation in pyi files. One possible way would be to add a body with pass rather than ... which would be one way to differentiate the two cases even inside a stub, although this would only work if None is among the possible return types, so maybe we’d need a sentinel that’s of type Any and return that instead, i.e. the opposite of NotImplemented, something like return Implemented.

But that would require flake8-pyi to stop emitting Y009/Y010 for methods decorated with abstractmethod.

I don’t think removing the decorator in the stub is an adequate solution, because then the type checker will no longer recognize AbstractContextManager() as an error, since it looks like it no longer has any abstract methods, but in fact it still does, so it will cause an error at runtime. The same goes for any subclasses that don’t implement __exit__

The false positive on super() is slightly less bad, so I could be convinced that the current behavior is fine and it’s better to have some false positives in stubs than false negatives.

Are there many examples of empty abstract methods in real code?

Most abstractmethods I write are empty, otherwise I wouldn’t have made them abstract.

I don’t really have a good intuition for which case is more common. Beyond that there’s also the question of how often abstractmethod have raise NotImplementedError as their body, which is actually the more worrying case, since an empty implementation is actually always fine, as long as None is a legal return value.

I think mypy strikes a pretty good balance, they raise an error for super() if they know for a fact it’s unsafe, otherwise they will keep quiet: mypy Playground

If you change all of the methods to return None, then it will actually complain about none of them, although it should probably still complain about e.

It seems like the issue is that typeshed has AbstractContextManager as a Protocol, while runtime has it as an ABC. Based on Eric’s comments in the pyright ticket, pyright wouldn’t object to calling super().__exit__ if it didn’t think it was a Protocol.

Protocol gives a lot more flexibility than an ABC, so the question is how much breakage would mypy_primer show if AbstractContextManager were an ABC in typeshed. Probably a lot, but before moving typeshed further away from implementation (or trying to change the implementation for the sake of typing) it’s probably worth checking if we can move typeshed closer to implementation instead.

1 Like

What I mean is empty as opposed to raising NotImplementedError. I would guess 100% of the abstract methods I write simply raise.

I didn’t even notice that it was a protocol in the typeshed. I don’t see why it should be though since in the runtime, it’s an abstract base class. Protocols are generally worse for the reasons I mentioned above, so I think I will just propose changing it to an ABC in the typeshed.

Edit: It can’t be an ABC do to limitations with generic type parameters not supporting Self.