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

False positive no-value-for-parameter due to inference edge case #8544

Closed
tushar-deepsource opened this issue Apr 6, 2023 · 3 comments · Fixed by pylint-dev/astroid#2117 or pylint-dev/astroid#2303
Labels
Astroid Related to astroid False Positive 🦟 A message is emitted but nothing is wrong with the code Needs PR This issue is accepted, sufficiently specified and now needs an implementation

Comments

@tushar-deepsource
Copy link
Contributor

tushar-deepsource commented Apr 6, 2023

Bug description

Due to incorrect inference, E1120 can be raised on code that is functional.

Example code:

def get_nums():
    nums = ()
    if input() == '1':
        nums = (1, 2)
    return nums

def add(x, y):
    return x + y

nums = get_nums()
if nums:
    add(*nums)  # issue raised here, pylint thinks `nums` will always be `()`

Command used

pylint asd.py --disable=all --enable=E1120

Pylint output

************* Module asd
asd.py:12:4: E1120: No value for argument 'x' in function call (no-value-for-parameter)
asd.py:12:4: E1120: No value for argument 'y' in function call (no-value-for-parameter)

Expected behavior

No issues raised.

Pylint version

pylint 2.17.2
astroid 2.15.2
Python 3.11.2 (main, Feb 16 2023, 02:55:59) [Clang 14.0.0 (clang-1400.0.29.202)]

OS / Environment

MacOS Ventura, 13.2.1

Additional dependencies

No response

@tushar-deepsource tushar-deepsource added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Apr 6, 2023
@Pierre-Sassoulas Pierre-Sassoulas added Astroid Related to astroid False Positive 🦟 A message is emitted but nothing is wrong with the code Needs PR This issue is accepted, sufficiently specified and now needs an implementation and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Apr 6, 2023
@jacobtylerwalls
Copy link
Member

jacobtylerwalls commented Apr 9, 2023

When CallSite._unpack_args infers the values of an unpacked Starred node, it should keep the knowledge that there were multiple inference results so that pylint's TypeCheck.visit_call can bail out early. Would you like to prepare PRs?

@tusharsadhwani
Copy link
Contributor

tusharsadhwani commented Apr 9, 2023

sure thing, can you point me to the right files? I'm assuming it'll go in the infer() implementation for CallSite in astroid?

@jacobtylerwalls
Copy link
Member

jacobtylerwalls commented Apr 9, 2023

Here's where we're dealing with the first inference result only (well, one of two places in this file):

https://github.com/pylint-dev/astroid/blob/5e8c89afe4a12eeab2ab97d95495187990e5df27/astroid/arguments.py#L138

If we change this simply to safe_infer, that should be enough to disregard ambiguous results, but it will affect other messages, too. That's probably what we want, but it would be good to see the pylint primer results with such a change. An alternative would be to just add this knowledge to the CallSite object with a "number of inferred nodes" attribute and then only bail out of this specific pylint check if > 1, but that's probably over-engineering things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Astroid Related to astroid False Positive 🦟 A message is emitted but nothing is wrong with the code Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Projects
None yet
4 participants