Skip to content
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

logging.StreamHandler.stream is sys.stderr by default ; "SupportsWrite['str']" is too narrow #5680

Closed
mr-c opened this issue Jun 23, 2021 · 14 comments · Fixed by #5681
Closed

Comments

@mr-c
Copy link
Contributor

mr-c commented Jun 23, 2021

This prevents code like logging.StreamHandler().stream.close() from being type checked as valid

https://docs.python.org/3/library/logging.handlers.html#logging.StreamHandler

This problem was introduced in https://github.com/python/typeshed/pull/5069/files#diff-23ef9e6197f6f95428d7333190fc2357c1c4f110b89c27a7fc71e2fab7d7897aR702

@srittau
Copy link
Collaborator

srittau commented Jun 23, 2021

This is a tradeoff. SupportsWrite[str] is the only thing needed by the implementation of StreamHandler and users are free to use any stream that fulfills this protocol. Therefore using any other method from a stream is potentially unsafe (although it's not in practice of setStream() is never used.

I was deliberating whether to make StreamHandler generic over the stream. I refrained from it, because in theory setStream() could change the type of stream. But it's probably the best solution.

@Akuli
Copy link
Collaborator

Akuli commented Jun 23, 2021

because in theory setStream() could change the type of stream.

I agree that this is a good use case for an invariant TypeVar, since this is the same situation as with lists. For example, list_of_strings.append(None) can change the type of a list, similarly to textiowrapper_handler.setStream(io.StringIO()).

@srittau
Copy link
Collaborator

srittau commented Jun 23, 2021

Yes. In theory if someone would do the following, type checkers would warn:

h = StreamHandler()
h.setStream(StringIO())

But I think that's a good thing that users need to be explicit about this:

h: StreamHandler[MyProto] = StreamHandler()
h.setStream(StringIO())

And the following is a bad idea anyway:

h = StreamHandler(open("abc", "w"))
h.setStream(StringIO())
h.stream.getvalue()

@gaborbernat
Copy link
Contributor

@srittau How does typehint the class now with this change?

❯ py
Python 3.10.1 (main, Dec  9 2021, 07:52:48) [Clang 13.0.0 (clang-1300.0.29.3)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from logging import StreamHandler
>>> StreamHandler[str]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'type' object is not subscriptable
h: StreamHandler[MyProto] = StreamHandler()

This only works if the annotation is not evaluated 🤔

@srittau
Copy link
Collaborator

srittau commented Jan 4, 2022

Indeed. You have to either use from __future__ import annotations or quote the annotation.

@gaborbernat
Copy link
Contributor

Sometimes you cannot do that, can we revert this in the meantime? 🤔 E.g. now impossible to write:

class MyHanlder(StreamHandler):
      pass

🤔 you need to add a type ignore to the inheritance line otherwise mypy will complain that is missing a generic parameter.

@srittau
Copy link
Collaborator

srittau commented Jan 4, 2022

The ultimate solution is to have defaults for generics (python/typing#307), but for now we unfortunately require # type: ignore.

@AlexWaygood
Copy link
Member

You could also submit a PR to CPython to add __class_getitem__ to StreamHandler. Previous PRs along those lines have been accepted, e.g. python/cpython#28714

@gaborbernat
Copy link
Contributor

That wouldn't work for the older Python version though only the latest 3.11 or later 🤔

@Akuli
Copy link
Collaborator

Akuli commented Jan 4, 2022

A bit annoying, but maybe better than making a pull request to CPython or using # type: ignore:

if TYPE_CHECKING:
    _base_class = StreamHandler[MyProto]
else:
    _base_class = StreamHandler

class MyHanlder(_base_class):
    pass

@hauntsaninja
Copy link
Collaborator

In case it's useful for future reference, this section of the mypy documentation covers ways to work around this class of issue: https://mypy.readthedocs.io/en/stable/runtime_troubles.html#using-classes-that-are-generic-in-stubs-but-not-at-runtime

@mr-c
Copy link
Contributor Author

mr-c commented Jan 6, 2022

In cases others did not notice it, the stream attribute of logging.StreamHandler is not part of the official API, so in my case we track stream separately, removing the issue.

@AlexWaygood
Copy link
Member

AlexWaygood commented May 1, 2022

I've opened python/cpython#92129 to add __class_getitem__ to logging.StreamHandler in 3.11+.

(Update: merged!)

@vectro
Copy link

vectro commented Jan 25, 2023

With this change, how would one type hint handler in

if output_file is None:
    handler = logging.StreamHandler()
else:
    handler = logging.FileHandler(output_file)

Because of the invariant generic, it seems that the only valid type for handler is StreamHandler[Any].

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 a pull request may close this issue.

7 participants