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

Catch MRO error when applying missing hints feature. #4688

Merged
merged 8 commits into from Jul 8, 2021

Conversation

doranid
Copy link
Contributor

@doranid doranid commented Jul 7, 2021

Steps

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Add a ChangeLog entry describing what your PR does.
  • If it's a new feature or an important bug fix, add a What's New entry in
    doc/whatsnew/<current release.rst>.
  • Write a good description on what the PR does.

Description

This fixes #4687.
Missing type hints won't fail hard anymore when applied to an instance with an MRO with duplicated names.

Type of Changes

Type
🐛 Bug fix

Related Issue

Closes #4687

@coveralls
Copy link

coveralls commented Jul 7, 2021

Coverage Status

Coverage increased (+0.01%) to 92.053% when pulling c5b66fc on doranid:fix_mro into 708527d on PyCQA:main.

@Pierre-Sassoulas
Copy link
Member

I had a feeling this was something we had to fix in astroid, but no tests seems to fail when simply catching the exception. Do you have an opinion @cdce8p ?

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.9.4 milestone Jul 8, 2021
@Pierre-Sassoulas Pierre-Sassoulas added Crash 💥 A bug that makes pylint crash Blocker 🙅 Blocks the next release labels Jul 8, 2021
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.

Thank you for this fix :) ! Could you add the example you gave in the issue in tests/functionals/r/regression/regression_mro_error.py ?

# pylint: disable=missing-docstring,pointless-statement,useless-object-inheritance


class Klass(object, object):
    def get(self):
        self._non_existent_attribute [no-member]

If it's not working we'll have to modify astroid and release a new version.

@doranid
Copy link
Contributor Author

doranid commented Jul 8, 2021

Hey, I added the required tests. Thanks :)

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.

The test was failing because you're probably on python 3.7 and we do not check the column in the tests for python 3.7 because the ast got different and better in 3.8. Thanks for fixing this crash, you're helping us release 2.9.4 faster !

@Pierre-Sassoulas Pierre-Sassoulas merged commit 58d85b2 into pylint-dev:main Jul 8, 2021
@Pierre-Sassoulas
Copy link
Member

Congratulation on becoming a pylint contributor, especially on such an important issue, the fast crash fixes after a new release are very stressful as maintainer <3 !

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 this pull request may close these issues.

Missing member hint fails when object has duplicated bases
3 participants