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 nested-min-max #8168

Closed
XuehaiPan opened this issue Feb 2, 2023 · 7 comments · Fixed by #8234
Closed

False positive nested-min-max #8168

XuehaiPan opened this issue Feb 2, 2023 · 7 comments · Fixed by #8234
Assignees
Labels
Enhancement ✨ Improvement to a component Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Milestone

Comments

@XuehaiPan
Copy link

Bug description

The nested-min-max suggests max(3, max(1, 2)) to be max(3, 1, 2). However, if the min / max function takes an iterable max(iterable) instead of multiple inputs max(a, b, c). The nested min/max cannot be rewritten as a single statement max(x, max(iterable)) -> max(x, iterable).

"""test.py"""

lst = [1, 2]
max(3, max(lst))

If rewrite it to max(3, lst), it will raise:

TypeError: '>' not supported between instances of 'list' and 'int'

Configuration

No response

Command used

pylint test.py

Pylint output

************* Module test
test.py:4:0: W3301: Do not use nested call of 'max'; it's possible to do 'max(3, lst)' instead (nested-min-max)

------------------------------------------------------------------
Your code has been rated at 5.00/10 (previous run: 5.00/10, +0.00)

Expected behavior

No warnings.

Pylint version

pylint 2.16.0
astroid 2.14.1
Python 3.8.13 (default, Oct 21 2022, 23:50:54)

OS / Environment

No response

Additional dependencies

No response

@XuehaiPan XuehaiPan added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Feb 2, 2023
@clavedeluna
Copy link
Collaborator

I think this makes sense and follows the logic for pylint also not raising the message for these types of inputs:

# This is too complicated (for now) as there is no clear better way to write it
max(max(i for i in range(10)), 0)
max(max(max(i for i in range(10)), 0), 1)

@clavedeluna clavedeluna 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 and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Feb 2, 2023
@Pierre-Sassoulas
Copy link
Member

What is wrong with max(lst + [3])?

@nickdrozd
Copy link
Collaborator

What is wrong with max(lst + [3])?

It allocates an extra list object, which might be undesirable.

How about splicing in the list?

lst = [1, 2]
max(3, *lst)

@Pierre-Sassoulas Pierre-Sassoulas added Question and removed 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 Feb 2, 2023
@Pierre-Sassoulas Pierre-Sassoulas closed this as not planned Won't fix, can't repro, duplicate, stale Feb 2, 2023
@solney-syn
Copy link

solney-syn commented Feb 2, 2023

Splatting the list (or allocating a new one) might be ways to avoid the nested min/max - but that's not what the fixer recommends. It says

it's possible to do 'max(3, lst)'

Which is incorrect in this case, no?

@Pierre-Sassoulas Pierre-Sassoulas added Enhancement ✨ Improvement to a component Needs PR This issue is accepted, sufficiently specified and now needs an implementation and removed Question labels Feb 2, 2023
@nickdrozd
Copy link
Collaborator

Maybe it could suggest max(3, *lst) if the value of lst could be inferred to be an iterable? Is that feasible?

@vejkse
Copy link

vejkse commented Mar 24, 2023

I’m not sure I should open a new issue or not, but the current solution (in pylint 2.17.0) still gives wrong suggestions in some cases.

With

max(3, max([5] + [4]))

we get the correct suggestion

test.py:4:6: W3301: Do not use nested call of 'max'; it's possible to do 'max(3, *[5] + [4])' instead (nested-min-max)

but with

max(3, max([5] + [i for i in range(6,10) if i % 2 == 0]))

we get the incorrect one

test.py:5:6: W3301: Do not use nested call of 'max'; it's possible to do 'max(3, [5] + [i for i in range(6, 10) if i % 2 == 0])' instead (nested-min-max)

@Pierre-Sassoulas
Copy link
Member

@vejkse thank you for reporting. Can you open a new issue please ? It's going to get lost in a closed issue otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants