Update `__exit__` parameters in stubs by Avasam · Pull Request #9696 · python/typeshed

Avasam · 2023-02-09T04:14:14Z

Follow-up to #9519 for third-party stubs now that a new version of mypy has been released
Ref #9297

I could not validate psycopg2, hdbcli and _cffi_backend so I left those as-is

srittau · 2023-02-09T07:38:11Z

I'm not sure that replacing proper __exit__ types with (essentially) object is the right course of action. I can't imagine a situation where you would want to pass some arbitrary into __exit__. In fact I'd say that's a sign of potential bug (even if __exit__ itself accepts it.) It also makes sub-classing more difficult, as an overridden __exit__ would also need to accept object, instead of the proper types.

…/typeshed into __exit__-parameters-stubs

Avasam · 2023-02-09T15:28:29Z

[...] I can't imagine a situation where you would want to pass some arbitrary into __exit__. In fact I'd say that's a sign of potential bug (even if __exit__ itself accepts it.)
It also makes sub-classing more difficult, as an overridden __exit__ would also need to accept object, instead of the proper types.

Should non-star __a?exit__ parameters always use their exact type then? (even if unused). Maybe except for mock.

srittau · 2023-02-09T15:35:01Z

I'd say ideally all __exit__ signatures would use the following signature (with the exception of the return type), even those that just use *args in the implementation:

_E=TypeVar("_E", bound=BaseException)
classX:
@overloaddef__exit__(self, __type: None, __value: None, __exc: None) ->None: ...
@overloaddef__exit__(self, __type: type[_E], __value: type[_E], __exc: TracebackType) ->None: ...

But since that is unwieldy, we can use the best approximation in a current situation. So I'm actually fine to use Unused for new annotations if that's the fastest way to do it, but we shouldn't regress back from the ideal situation. But I'd be interested in what the other maintainers think.

Avasam · 2023-02-09T15:49:00Z

[...] So I'm actually fine to use Unused for new annotations if that's the fastest way to do it, but we shouldn't regress back from the ideal situation.

Ok, I think my last commit satisfies your comment. (now the PR only adds new Unused when it's unused, new object for *args params and changes *args: object to *args: Unused when unused.

I'd say ideally all __exit__ signatures would use the following signature (with the exception of the return type), even those that just use *args in the implementation: [...]
But since that is unwieldy, we can use the best approximation in a current situation.

Given this isn't a "stopgap", "urgent" or "in the meantime" PR. It's just splitting off a would-be bigger PR about semantically using Unused instead of object or _Unused for unused parameters.
Since it's focusing entirely on exit dunder methods. I don't mind changing it to do it "the proper way" everywhere (and even documenting in CONTRIBUTING.md, and/or update Flake8-pyi, any decision about typing exit dunder methods)

srittau · 2023-02-09T16:15:49Z

Side note: Changing __exit__ to overloads could be disruptive or could even now work. I'd definitely would test it with a small explorative PR first.

JelleZijlstra · 2023-02-22T07:39:16Z

This PR has a lot of conflicts now.

I'd be hesitant to use the overloaded signature @srittau suggests, though it is probably technically correct:

There may be rare instances where only some of the arguments are None, though recent changes to CPython make this less and less likely.
Using this signature will force users writing subclasses to also write the verbose signature with overloads, which feels like unnecessary busywork.

Avasam · 2023-02-22T17:56:24Z

This PR has a lot of conflicts now.

Expected, but not hard to fix :)

About the signature, should I leave it a-is, (can be done in a different PR and/or discussed further). At least get someway there? Or full-on change all signatures?

…__-parameters-stubs

AlexWaygood · 2023-02-24T15:13:33Z

My preference would be to generally type __(a)exit__ methods like this, if the method uses *args at runtime:

def__exit__(self, *args: Unused) ->None: ...

And to generally type __(a)exit__ methods like this, if the method includes all three parameters explicitly at runtime:

def__exit__(self, exc_typ: type[BaseException] |None, exc_val: BaseException|None, exc_tb: TracebackType|None

(There will obviously be exceptions to these rules where doing something else makes sense.)

I agree with @srittau that using object or Unused for annotations with the three-argument form could make subclassing unnecessarily difficult, as subclasses will be obliged to use object for annotating those parameters in the subclass.

I also agree with @srittau that a truly precise signature for __(a)exit__ methods would require overloads, but that this would be pretty tedious to do everywhere, and probably wouldn't actually improve type safety significantly. And I agree with @JelleZijlstra that using overloads could also make it harder for people to subclass these classes, so has downsides as well.

Avasam · 2023-02-25T19:11:52Z

And to generally type __(a)exit__ methods like this, if the method includes all three parameters explicitly at runtime:
def__exit__(self, exc_typ: type[BaseException] |None, exc_val: BaseException|None, exc_tb: TracebackType|None

I know you said there could be exceptions. But that sounds like something Flake8-PYI could check for and/or add in its existing __(a)exit__ checks, if the exceptions are rare enough.

…/typeshed into __exit__-parameters-stubs

AlexWaygood · 2023-02-25T19:18:31Z

I know you said there could be exceptions. But that sounds like something Flake8-PYI could check for and/or add in its existing __(a)exit__ checks, if the exceptions are rare enough.

Y036 actually used to be stricter, but Jelle persuaded me to relax it slightly in PyCQA/flake8-pyi#209 after we came across a situation in #7625 (comment) where there was a good case for doing something different. Maybe the exceptions that break the rule are rare enough that we could consider reverting that change... idk, I don't really have a strong opinion on exactly how strict flake8-pyi should be on this ¯\_(ツ)_/¯

Avasam · 2023-02-25T19:20:33Z

 def __enter__(self) -> Self: ...
- def __exit__(self, exc_type: object, exc_value: object, traceback: object) -> None: ...
+ def __exit__(
+ self, exc_type: type[BaseException] | None, exc_value: BaseException | None, traceback: TracebackType | None
