Skip to content

Sum and multiply #740

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

Closed
wants to merge 94 commits into from
Closed

Sum and multiply #740

wants to merge 94 commits into from

Conversation

hippo91
Copy link
Contributor

@hippo91 hippo91 commented Dec 30, 2019

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 PR tries to solve the issue #669. In fact the call to some functions of the numpy brains are inferred not only as numpy.ndarray but also as Uninferable. This lead to false negative for such function calls.

The origin of this bug has been hard to find but seems to be due to the fact that the context.push method return True as soon as the node in argument has been visited once for the current context.
However for some particular cases, such as those encountered in the numpy library, a node could be visited more than once due to peculiar imports.
The modification made here ensure that all calls to numpy ufunc functions are inferred only as numpy.ndarray and modifies two inference unit tests. Those ones seem more pertinent but a close look from the reviewer would be appreciated.

Type of Changes

Type
🐛 Bug fix

Related Issue

Closes #669

@hippo91 hippo91 requested a review from PCManticore February 9, 2020 19:57
@PCManticore
Copy link
Contributor

Hey @hippo91 The code looks good. I tested this against pylint and it causes a regression test to fail. Can you determine why that would be the case with this patch? It's regression_property_no_member_2641. I believe it's something to do with the new inference limit but haven't been able to investigate more closely.

…rrors (#804)

This also makes debugging a lot simpler reducing the complexity of the
function stack.
@hippo91
Copy link
Contributor Author

hippo91 commented Jun 28, 2020

@PCManticore thanks for looking at this. I confirm that there is a problem with this unit test. I'm trying to fix it.

@hippo91
Copy link
Contributor Author

hippo91 commented Aug 4, 2020

@PCManticore ,

i had a look at the regression_property_no_member_2641 unittest and i think that the problem is not directly linked to this PR.
In fact, the following code triggers also the no-member message :

# pylint: disable=missing-docstring,unused-argument,too-few-public-methods
class Person:
    def __init__(self, name):
        self.name = name

    @property
    def name(self):
        return self.__name

    @name.setter
    def name(self, value):
        self.__name = value

A = Person('toto')
Person.name.fset(A, 'toto')

In case fset is replaced by fget then no problem arises. I think the problem is a lack of attr_fset method in the PropertyModel class (objectmodel module).
@PCManticore do you thinks there is a reason for the absence of attr_fset and attr_del method in this class?
Is it wise to add them?
If yes i'll make another PR.
Thanks in advance.

hippo91 and others added 13 commits September 14, 2020 22:13
If a class inherits from two bases and the classes are expressed as a
non-trivial expression such as qualified with a module name, classes
after the first could not be inferred due to reusing the context
object in _inferred_bases
 Before this change, when there were import failures by Astroid, it
 would store the error in the module cache (for example a "module not
 found" error), including tracebacks. These tracebacks would take up a
 lot of memory, and don't seem to be used for any sort of purpose
 beyond debugging value.

 On one project stripping this value entirely reduces memory usage by
 75%.
 This fixes some `inconsistent-return-statement` pylint errors
@PCManticore
Copy link
Contributor

@hippo91 Yeah, that makes sense, let's add that in separate PRs.

@hippo91
Copy link
Contributor Author

hippo91 commented Jan 24, 2021

Replaced by sum_and_multiply_clean. Same sources with a cleaner log history

@hippo91 hippo91 closed this Jan 24, 2021
@cdce8p cdce8p mentioned this pull request Feb 6, 2021
@PCManticore PCManticore deleted the sum_and_multiply branch February 7, 2021 08:59
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.

Wrong inference maybe due to inference cache