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

stdlib/logging use object over Any for parameter types #6025

Merged
merged 5 commits into from Sep 10, 2021
Merged

stdlib/logging use object over Any for parameter types #6025

merged 5 commits into from Sep 10, 2021

Conversation

sanderr
Copy link
Contributor

@sanderr sanderr commented Sep 10, 2021

The logging stub uses a lot of Any types for method and function parameters. In reality, as far as I'm aware, these parameters accept values of any type. As such, object would be the strict and correct type for those parameters (see Contributing.md).

The reason I'm opening this pull request is that mypy, when run in strict mode, considers all calls to functions with Any types invalid. As a result, the type coverage report includes a lot of log lines. Making the type of these parameters strict resolves this issue.

Copy link
Collaborator

@Akuli Akuli left a comment

Choose a reason for hiding this comment

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

Not a full review.

exc_info: _ExcInfoType = ...,
stack_info: bool = ...,
stacklevel: int = ...,
extra: dict[str, Any] | None = ...,
extra: dict[str, object] | None = ...,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't work for dicts, because dicts are invariant: a dict[str, int] is not a valid dict[str, object]. The reason is that if foo is a dict[str, object], then foo["bar"] = "lol" is valid, but that really shouldn't be valid if foo is a dict[str, int].

Any is really needed for this: we want to say "do not complain about the value type, whatever type it is" (Any), not "the values can be any objects" (object).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I missed that, thanks. However, I don't think there's a need for the invariant dict here, is there? Replacing this with the covariant Mapping should work, shouldn't it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, it can be Mapping, or even one of the mapping-like SupportsFooAndBar protocols in _typeshed. The extra dict gets passed to makeRecord, so it can be whatever makeRecord supports.

@Akuli Akuli requested a review from srittau September 10, 2021 13:25
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

pip (https://github.com/pypa/pip.git)
+ src/pip/_internal/utils/subprocess.py:159: error: Incompatible types in assignment (expression has type "Callable[[str, VarArg(Any), KwArg(Any)], None]", variable has type "Callable[[object, VarArg(object), DefaultNamedArg(Union[None, bool, Union[Tuple[Type[BaseException], BaseException, Optional[TracebackType]], Tuple[None, None, None]], BaseException], 'exc_info'), DefaultNamedArg(bool, 'stack_info'), DefaultNamedArg(int, 'stacklevel'), DefaultNamedArg(Optional[Mapping[str, object]], 'extra')], None]")

sphinx (https://github.com/sphinx-doc/sphinx.git)
+ sphinx/util/logging.py: note: In member "log" of class "SphinxLoggerAdapter":
+ sphinx/util/logging.py:121:5: error: Signature of "log" incompatible with supertype "LoggerAdapter"

Copy link
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.

Thanks. It seems a few projects need to change their annotations, but I think the disruption is tolerable.

@srittau srittau merged commit 7bfdacb into python:master Sep 10, 2021
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.

None yet

3 participants