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

Select last inferred value for usage #4357

Conversation

Alphadelta14
Copy link

Fixes the underlying issue associated with #3825, where a module with two function definitions in a third party should have the last definition selected instead of the first.

In the numpy.lib case, there are two functions with the same name in the same scope. The first is selected resulting in bad signatures:

# from .function_base import *
def unique(x):

# from .arraysetops import *
def unique(ar, return_index=False, return_inverse=False,
           return_counts=False, axis=None):

This was fixed upstream by numpy by removing the first definition, but this was definitely something bothering me because the behavior did not match python behavior properly. I hope it fixes other folks' issues as well.

At the same time, safe_infer is a heavily used function in the pylint codebase and should be subject to additional scrutiny.

Most inference cases are expected to have a single inferred result, so they wouldn't be affected by this.
For inference cases where two different types are seen, we return None. This is the same behavior.

In the case of multiple inference results of the same pytype, this behavior changes:
FunctionDef: later function definition is chosen (seems like fixed behavior)
Import by name: last import with same name wins (seems like fixed behavior)
Assign: last value inferred (seems like fixed behavior)
Sequence: last item inferred instead of first (different behavior, still one-of-many).
Returns: last return value used (different behavior than before, but still using one-of-many).

Steps

Todo after acknowledgement

  • 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

Type of Changes

Type
βœ“ πŸ› Bug fix

Related Issue

Closes #3825

@Pierre-Sassoulas
Copy link
Member

Hello @Alphadelta14, is this merge request still relevant ? It seems the test do not pass and the issue it aime to fix is closed now.

@Alphadelta14 Alphadelta14 force-pushed the alphadelta14/3825-multiple-import-infer branch from 9be02e2 to 143406a Compare July 11, 2021 19:12
@Alphadelta14
Copy link
Author

At the time of submission, all PRs were failing in the same way. I just rebased to see if the issue was resolved on the main branch.
The underlying issue was addressed in the one-off numpy case, but this resolves it generally instead of requiring each violator to fix it before yielding the correct results.

@Pierre-Sassoulas
Copy link
Member

Fixing the problem generically sounds great, thank you for rebasing ! If you have time to finish this MR, I'd be glad to review it.

@Alphadelta14
Copy link
Author

@Pierre-Sassoulas, there was an issue with partials where they were being written into the wrong scope which this PR exposed. I addressed it in this PR and also added an upstream fix to astroid pylint-dev/astroid#1097.
If these changes look good to you, I'll gladly go through the rest of the contribution process. :)

@Pierre-Sassoulas
Copy link
Member

Nice, let's start with the astroid MR then :)

@Pierre-Sassoulas Pierre-Sassoulas added Needs astroid update Needs an astroid update (probably a release too) before being mergable Work in progress labels Jul 29, 2021
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.14.0 milestone Apr 3, 2022
@Pierre-Sassoulas Pierre-Sassoulas force-pushed the alphadelta14/3825-multiple-import-infer branch from d62cc05 to 186cae4 Compare April 3, 2022 11:46
@Pierre-Sassoulas Pierre-Sassoulas removed Needs astroid update Needs an astroid update (probably a release too) before being mergable Work in progress labels Apr 3, 2022
@Pierre-Sassoulas
Copy link
Member

Rebased following the merging of pylint-dev/astroid#1097 and the upgrade of astroid.

@Pierre-Sassoulas Pierre-Sassoulas added the Needs review πŸ” Needs to be reviewed by one or multiple more persons label Apr 3, 2022
@Pierre-Sassoulas
Copy link
Member

I'm going to close as the code makes assumption that would result in false positives in particular in this example:

def temp():
    """This is not weird"""
    if True:
        return [1, 2]
    return [2, 3, 4]

def temp2():
    """This is weird, but correct"""
    if True:
        return (1, 2)

    if True:
        return (2, 3)
    return (4, 5)


class UnbalancedUnpacking(object):
    """Test unbalanced tuple unpacking in instance attributes."""

    # pylint: disable=attribute-defined-outside-init, invalid-name, too-few-public-methods
    def test(self):
        """unpacking in instance attributes"""
        # we're not sure if temp() returns two or three values
        # so we shouldn't emit an error
        self.a, self.b = temp()
        self.a, self.b = temp2()
        self.a, self.b = unpack()  # [unbalanced-tuple-unpacking]

We would start emitting an # [unbalanced-tuple-unpacking] for self.a, self.b = temp() which is wrong.

@Pierre-Sassoulas Pierre-Sassoulas removed the Needs review πŸ” Needs to be reviewed by one or multiple more persons label Apr 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pylint incorrectly issues error E1123 on numpy.unique kwargs (numpy v1.15.4)
2 participants