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

Crash when checking function overwrites #4443

Closed
cdce8p opened this issue May 6, 2021 · 5 comments · Fixed by #4456
Closed

Crash when checking function overwrites #4443

cdce8p opened this issue May 6, 2021 · 5 comments · Fixed by #4456
Labels
Blocker 🙅 Blocks the next release Crash 💥 A bug that makes pylint crash
Milestone

Comments

@cdce8p
Copy link
Member

cdce8p commented May 6, 2021

Steps to reproduce

from __future__ import annotations

class Parent:
    async def func(self, user_input: dict[str, int] | None) -> None:
        pass

class Child(Parent):
    async def func(self, user_input: dict[str, int] | None) -> None:
        pass

Current behavior

Exception on node <AsyncFunctionDef.func l.10 at 0x104dbf970> in file '/../pylint/a.py'
Traceback (most recent call last):
  File "/../pylint/venv-39_link/bin/pylint", line 33, in <module>
    sys.exit(load_entry_point('pylint', 'console_scripts', 'pylint')())
  File "/../pylint/pylint/__init__.py", line 24, in run_pylint
    PylintRun(sys.argv[1:])
  File "/../pylint/pylint/lint/run.py", line 384, in __init__
    linter.check(args)
  File "/../pylint/pylint/lint/pylinter.py", line 919, in check
    self._check_files(
  File "/../pylint/pylint/lint/pylinter.py", line 953, in _check_files
    self._check_file(get_ast, check_astroid_module, name, filepath, modname)
  File "/../pylint/pylint/lint/pylinter.py", line 979, in _check_file
    check_astroid_module(ast_node)
  File "/../pylint/pylint/lint/pylinter.py", line 1113, in check_astroid_module
    retval = self._check_astroid_module(
  File "/../pylint/pylint/lint/pylinter.py", line 1158, in _check_astroid_module
    walker.walk(ast_node)
  File "/../pylint/pylint/utils/ast_walker.py", line 77, in walk
    self.walk(child)
  File "/../pylint/pylint/utils/ast_walker.py", line 77, in walk
    self.walk(child)
  File "/../pylint/pylint/utils/ast_walker.py", line 74, in walk
    callback(astroid)
  File "/../pylint/pylint/checkers/classes.py", line 994, in visit_functiondef
    self._check_signature(node, parent_function, "overridden", klass)
  File "/../pylint/pylint/checkers/classes.py", line 1813, in _check_signature
    arg_differ_output = _different_parameters(
  File "/../pylint/pylint/checkers/classes.py", line 354, in _different_parameters
    different_positional = _has_different_parameters(
  File "/../pylint/pylint/checkers/classes.py", line 288, in _has_different_parameters
    if original_type.name != overridden_type.name:
AttributeError: 'BinOp' object has no attribute 'name'

Expected behavior

No error

pylint --version output

pylint 2.8.3.dev15+gce55bce5
astroid 2.5.6
Python 3.9.5 (v3.9.5:0a7dcbdb13, May  3 2021, 13:05:53) 
[Clang 12.0.5 (clang-1205.0.22.9)]

Additional information

The error is most likely caused by #4422

/CC: @Pierre-Sassoulas @ksaketou

@cdce8p cdce8p added Blocker 🙅 Blocks the next release Crash 💥 A bug that makes pylint crash labels May 6, 2021
@cdce8p
Copy link
Member Author

cdce8p commented May 6, 2021

Similar errors for:

import typing

class Parent:
    async def func(self, user_input: typing.Dict[str, int]) -> None:
        pass

class Child(Parent):
    async def func(self, user_input: typing.Dict[str, int]) -> None:
        pass
AttributeError: 'Subscript' object has no attribute 'name'

--

import typing

class Parent:
    async def func(self, user_input: typing.List) -> None:
        pass

class Child(Parent):
    async def func(self, user_input: typing.List) -> None:
        pass
AttributeError: 'Attribute' object has no attribute 'name'

@Pierre-Sassoulas
Copy link
Member

Thanks for giving so much test cases, clearly we lacked some examples :) How did you find the problem ? (Could it be prevented by #4413 ?)

@cdce8p
Copy link
Member Author

cdce8p commented May 6, 2021

Thanks for giving so much test cases, clearly we lacked some examples :) How did you find the problem ?

I tested the current master against the Home Assistant repo. And then some educated guessing.

Could it be prevented by #4413 ?

It probably would have been. I was thinking we should really add something like it lately. One problem that I noticed though while thinking about it is that in contrast to black we'll need to install all dependencies as well. That might be more difficult. Then again mypy probably needs to do that too.

@Pierre-Sassoulas
Copy link
Member

One problem that I noticed though while thinking about it is that in contrast to black we'll need to install all dependencies as well.

Yes, this is going to cost a shallow git clone + dependencies installation for each repositories at least. But I think it's worth it, so we do not release preventable crashes. It's really easy to not cover every edge case in our functional tests.

@ksaketou
Copy link
Contributor

ksaketou commented May 6, 2021

Thank you for these tests. I agree that we lacked several test cases here, like the 'BinOp' case. Also, regarding the typing case, I think it can be fixed, more tests need to be added anyway since the functionality of #4422 is a new addition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocker 🙅 Blocks the next release Crash 💥 A bug that makes pylint crash
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants