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

Spurious W3301: min(value, min(iterable)) does not do the same thing as min(value, iterable) #9923

Open
zackw opened this issue Sep 11, 2024 · 1 comment
Labels
Needs decision 🔒 Needs a decision before implemention or rejection

Comments

@zackw
Copy link

zackw commented Sep 11, 2024

Bug description

min(iterable) produces the minimum element of the iterable. min(value, value, ...) produces the minimum of all its arguments; it does not notice iterables among the values. Therefore min(value, min(iterable)) is a useful thing to do, but min(value, iterable) will typically throw a TypeError because int and list[int] (for instance) are not comparable. The same applies to max().

To sum up: The code below is the natural way to take the minimum/maximum of an iterable plus some other values, and it should not trigger any W3301 warnings.

#pylint:disable=missing-docstring
#pylint:enable=nested-min-max

def bounds(lo, hi, elements):
    return (
        min(lo, hi, min(elements)),
        max(lo, hi, max(elements)),
    )

Command used

pylint a.py

Pylint output

************* Module a
a.py:18:8: W3301: Do not use nested call of 'min'; it's possible to do 'min(lo, hi, elements)' instead (nested-min-max)
a.py:19:8: W3301: Do not use nested call of 'max'; it's possible to do 'max(lo, hi, elements)' instead (nested-min-max)

Expected behavior

Do not issue W3301 when the inner call(s) to min/max pass only one argument.

Pylint version

pylint 3.2.7
astroid 3.2.4
Python 3.8.19 (default, Sep  9 2024, 11:01:37)
[GCC 13.3.1 20240614]
@zackw zackw added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Sep 11, 2024
@zenlyj zenlyj added 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 Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling 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 labels Nov 8, 2024
@zenlyj
Copy link
Contributor

zenlyj commented Nov 8, 2024

Thank you for this, an alternative to min(value, min(iterable)) is min(value, *iterable), without the nested call.

I agree that the suggested message - Do not use nested call of 'min'; it's possible to do 'min(lo, hi, elements)' instead is inaccurate when the arguments are not comparable. But at the same time, i am not sure if we should not raise any message when the inner call has only one argument, as it could lead to false negatives like:

def bounds(lo: int, hi: int, element: int):
    return (
        min(lo, hi, min(element)),  # should raise W3301 and suggest `min(lo, hi, element)`
    )

We'll probably want to keep the check as it is when the passed arguments are untyped, and perhaps improve the accuracy of the suggested code correction when typing is involved.

@zenlyj zenlyj added Needs decision 🔒 Needs a decision before implemention or rejection and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs decision 🔒 Needs a decision before implemention or rejection
Projects
None yet
Development

No branches or pull requests

2 participants