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

Add broken Callable check #5891

Merged
merged 2 commits into from
Mar 10, 2022
Merged

Add broken Callable check #5891

merged 2 commits into from
Mar 10, 2022

Conversation

cdce8p
Copy link
Member

@cdce8p cdce8p commented Mar 10, 2022

Type of Changes

Type
✨ New feature

Description

Similar to the broken NoReturn check - #5304, however this time for Callable.
Using collections.abc.Callable inside Optional and Union is broken in Python 3.9.0 and 3.9.1.

This one is a bit more difficult as we currently currently suggest replacing the deprecated typing.Callable with collections.abc.Callable. Doing that can result in unexpected errors. The PR will change that behavior and not emit deprecated-typing-alias for typing.Callable if py-version < (3, 9, 2). In addition it adds a new check to warn about potential errors.

Example broken code

from collections.abc import Callable
from typing import Optional, Union

Alias1 = Optional[Callable[[int], None]
Alias2 = Union[Callable[[int], None], None]

When exactly is it broken?

  • Python 3.9.0 or 3.9.1
  • And collections.abc.Callable as part of Optional or Union
  • And argument list for Callable, e.g. [int].

https://bugs.python.org/issue42965

@cdce8p cdce8p added Enhancement ✨ Improvement to a component typing Optional Checkers Related to a checked, disabled by default labels Mar 10, 2022
@coveralls
Copy link

coveralls commented Mar 10, 2022

Pull Request Test Coverage Report for Build 1965675033

  • 39 of 39 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 94.019%

Totals Coverage Status
Change from base Build 1964199132: 0.01%
Covered Lines: 14996
Relevant Lines: 15950

💛 - Coveralls

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Looks good already!

isinstance(parent_subscript, nodes.Subscript)
and isinstance(parent_subscript.value, (nodes.Name, nodes.Attribute))
):
return False
Copy link
Member

Choose a reason for hiding this comment

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

This line is not covered by the tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Saw that too. Seems like my previous filter steps are quite good already. Took me a moment to find something that would run this line.

pylint/extensions/typing.py Show resolved Hide resolved
pylint/extensions/typing.py Show resolved Hide resolved
self.add_message(
"deprecated-typing-alias",
node=msg.node,
args=(msg.qname, msg.alias),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
args=(msg.qname, msg.alias),
args=(msg.qname, msg.alias),
confidence=interfaces.INFERENCE,

AFAIK because of inferred.qname() line 275 / 282 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's do that separately. It would change existing tests.

@cdce8p cdce8p added this to the 2.13.0 milestone Mar 10, 2022
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

👌

@cdce8p cdce8p merged commit 6d30e12 into pylint-dev:main Mar 10, 2022
@cdce8p cdce8p deleted the broken-callable branch March 10, 2022 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component Optional Checkers Related to a checked, disabled by default typing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants