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

Fix crash on inference of __len__ #1234

Merged
merged 5 commits into from
Nov 8, 2021
Merged

Conversation

DanielNoord
Copy link
Collaborator

Steps

  • For new features or bug fixes, add a ChangeLog entry describing what your PR does.
  • Write a good description on what the PR does.

Description

This is a weird one.

I found this while working on the primer tests for pylint where running on the codebase of numpy created a crash. I issued pylint-dev/pylint#5244 for this. Initially I thought it had something to do with the brain for numpy as the crash only occurred when the directory name starts with numpy.
However, removing this single line seems to fix the issue. Furthermore, (locally) all tests for astroid and pylint pass with this change. I have looked at the commit that added this line (25384d4) but did not immediately understand why this was added. In any case, no tests seems to have been added or this line is redundant and the removing it is the way forward.

Type of Changes

Type
🐛 Bug fix

Related Issue

Would close pylint-dev/pylint#5244

@DanielNoord
Copy link
Collaborator Author

DanielNoord commented Nov 6, 2021

I'm marking this as a draft because I'm not 100% sure this is the right way forward, but I would like some input for any of the experienced astroid maintainers. I can't really continue this on my own.

I also have not thought of a good way to test this in astroid. The test in pylint work, but for astroid I do not know what will work best.

@Pierre-Sassoulas
Copy link
Member

@brycepg @hippo91 do you have an opinion on this change ?

@Pierre-Sassoulas Pierre-Sassoulas added the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Nov 6, 2021
@hippo91
Copy link
Contributor

hippo91 commented Nov 7, 2021

@DanielNoord @Pierre-Sassoulas i had a look to this PR. Honestly i do not understand why this line was added.
Maybe @brycepg will be more helpful than me.
IMHO if this line is not tested and all astroid and pylint tests are ok we can remove it.
Maybe we can ask @cdce8p to test it again home-assistant?

@Pierre-Sassoulas
Copy link
Member

Maybe we can ask @cdce8p to test it again home-assistant?

We're also putting primer tests in place in pylint-dev/pylint#5173 so maybe we'll get to be more confident in small changes like this in the near future :)

Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

I had some time to review this PR. A few notes.

The _proxied comparison is necessary. Otherwise this would also match.

class A:
    def __len__(self) -> int:
        return 42

class Crash:
    def __len__(self) -> int:
        a = A()
        return len(a)

Furthermore, I was able to reproduce the issue without numpy.

class MyClass:
    def some_func(self):
        return lambda: 42

    def __len__(self):
        return len(self.some_func())

Those two tests could probably be added to unittest_brain.py, similar to the existing one.
https://github.com/PyCQA/astroid/blob/b62f243b16b7f435c8be869577959e95a7927a91/tests/unittest_brain.py#L3112-L3126

--
As for numpy, the whole issue was only triggered because self.array was inferred as numpy FunctionDef. Not sure this is correct in this instance or if the numpy brain needs to be updated. Maybe @hippo91 know more about that? Anyway, if it needs to be updated, this should happen in another PR.
https://github.com/PyCQA/astroid/blob/b62f243b16b7f435c8be869577959e95a7927a91/astroid/brain/brain_numpy_core_multiarray.py#L41-L42
https://github.com/PyCQA/astroid/blob/b62f243b16b7f435c8be869577959e95a7927a91/astroid/brain/brain_numpy_utils.py#L90

astroid/helpers.py Show resolved Hide resolved
@DanielNoord
Copy link
Collaborator Author

Fixed the code with @cdce8p's suggestions and added test cases. The last test will fail without the changes in this PR, the first test is to make sure the inference on these kind of functions actually works.

As for the numpy error, I have not looked into it but we might want to open a separate issue to channel discussion. @cdce8p did you happen to find any hints as to why this inference was (probably) wrong? And if so, would you mind opening an issue?

@DanielNoord DanielNoord marked this pull request as ready for review November 7, 2021 20:24
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

Just some last comments. I know that the test names are already descriptive, but adding one more line in the docstring might be useful.

tests/unittest_brain.py Outdated Show resolved Hide resolved
tests/unittest_brain.py Outdated Show resolved Hide resolved
@DanielNoord
Copy link
Collaborator Author

Added docstrings, also for the existing test

ChangeLog Outdated Show resolved Hide resolved
tests/unittest_brain.py Outdated Show resolved Hide resolved
tests/unittest_brain.py Outdated Show resolved Hide resolved
tests/unittest_brain.py Outdated Show resolved Hide resolved
@cdce8p cdce8p removed the request for review from Pierre-Sassoulas November 8, 2021 11:24
@cdce8p cdce8p merged commit e1ecb6d into pylint-dev:main Nov 8, 2021
@DanielNoord DanielNoord deleted the crash-numpy branch November 8, 2021 11:27
@cdce8p
Copy link
Member

cdce8p commented Nov 8, 2021

As for the numpy issue, I don't think it's worth opening an issue for it as long as we don't encounter it in practice.
Maybe @hippo91 knows the answer to it, but not too important if not.

--
File placed in a folder which starts with numpy.

class MyClass:
    def __init__(self, array):
        self.array = array
        self.array  #@

self.array will (wrongly ?) be inferred as FunctionDef.
It seems to be related to this https://github.com/PyCQA/astroid/blob/b62f243b16b7f435c8be869577959e95a7927a91/astroid/brain/brain_numpy_core_multiarray.py#L41-L42
https://github.com/PyCQA/astroid/blob/e1ecb6d3d5a6555a807ab3db0f2f938d8d73e95c/astroid/brain/brain_numpy_core_multiarray.py#L90-L95

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs review 🔍 Needs to be reviewed by one or multiple more persons
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash on nodes.FunctionDef._proxied in numpy tests
4 participants