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.

Sign up for GitHub

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

Jump to bottom

False negative expression-not-assigned on fonction calls that return values #7935

Open
AlexeySalmin opened this issue Dec 13, 2022 · 11 comments · May be fixed by #8732
Open

False negative expression-not-assigned on fonction calls that return values #7935

AlexeySalmin opened this issue Dec 13, 2022 · 11 comments · May be fixed by #8732
Labels
Enhancement ✨ Improvement to a component False Negative 🦋 No message is emitted but something is wrong with the code Help wanted 🙏 Outside help would be appreciated, good for new contributors Needs PR This issue is accepted, sufficiently specified and now needs an implementation

Comments

@AlexeySalmin
Copy link

AlexeySalmin commented Dec 13, 2022 •
edited by Pierre-Sassoulas
Loading

Current problem

This is a request to implement a check similar to the [[nodiscard]] function attribute in C++. A typical mistake is when people assume that the method modifies the current object while it actually creates and returns a copy of self. Calling this method without using the return value doesn't make sense and could be easily caught.

An example with sqlalchemy. Instead of:

if (id):
    query.filter(row.id == id)

You should write:

if (id):
    query = query.filter(row.id == id)

The problem is the first version looks legit: query.filter reads as "filter the query", so it's hard to spot this mistake. I've seen even experienced sqlalchemy users fall into this trap once in a while.

Desired solution

I'd suggest something like # pylint: enable=unused-return-value pragma for functions with a subsequent analysis of the call site.

There's already a very similar warning expression-not-assigned / W0106, but it specifically ignores the function calls. Which makes sense because in many cases functions have side effects besides just the return value. In some cases though they're limited to the return value and it would be nice to catch these.

Additional context

https://stackoverflow.com/questions/51858215/how-to-make-pylint-report-unused-return-values
https://stackoverflow.com/questions/74785720/python-prevent-discarding-of-the-function-return-value
@AlexeySalmin AlexeySalmin added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Dec 13, 2022
@Pierre-Sassoulas Pierre-Sassoulas added Enhancement ✨ Improvement to a component Proposal 📨 Needs specification 🔐 Accepted as a potential improvement, and needs to specify edge cases, message names, etc. and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling Proposal 📨 labels Dec 14, 2022
@Pierre-Sassoulas
Copy link
Member

It seem like it could be considered like a false negative of expression-not-assigned, but a call can have side effect so right now we're not raising this message on calls.

@Pierre-Sassoulas Pierre-Sassoulas added False Negative 🦋 No message is emitted but something is wrong with the code Needs decision 🔒 Needs a decision before implemention or rejection and removed Needs specification 🔐 Accepted as a potential improvement, and needs to specify edge cases, message names, etc. labels Dec 14, 2022
@Pierre-Sassoulas Pierre-Sassoulas changed the title FR: report unused function return values False negative expression-not-assigned on fonction calls that return values Dec 14, 2022
@AlexeySalmin
Copy link
Author

It seem like it could be considered like a false negative of expression-not-assigned, but a call can have side effect so right now we're not raising this message on calls.

This is an option. The thing is, I'm really sure this should be enabled on a per-function basis, otherwise it's going to be 99% noise. But annotations for a few key functions in popular libraries (like the @_generative decorator in the sqlalchemy) will make a huge difference.

And from that perspective it makes sense to have a separate warning ID, just not to mess with defaults for expression-not-assigned.

@Pierre-Sassoulas
Copy link
Member

Sounds like an option to list the calls that should definitely be assigned to something would be nice (calls-that-need-assignment ?). Amusingly it would make https://pylint.pycqa.org/en/latest/user_guide/messages/warning/useless-with-lock.html useless if we simply add threading.Lock()to this list 😄

@LukasKrocek LukasKrocek linked a pull request Jun 1, 2023 that will close this issue
Open
@nedrebo
Copy link

nedrebo commented Nov 28, 2023

This feature would also be very valuable when using libraries with return codes/bools where you need to remember to check them all the time 😓

@Pierre-Sassoulas Pierre-Sassoulas added Help wanted 🙏 Outside help would be appreciated, good for new contributors Needs PR This issue is accepted, sufficiently specified and now needs an implementation and removed Needs decision 🔒 Needs a decision before implemention or rejection labels Nov 28, 2023
@samskiter
Copy link

Very much in favour of this!

No discard is quite a common warning in other languages: https://www.avanderlee.com/swift/discardableresult/

@samskiter
Copy link

Any implementation hints on how to do this? Glancing through the code I wonder if it would require inference?
There have been previous issues with inference of statements... #8073

@Pierre-Sassoulas
Copy link
Member

Yes, infering the return of the function would work and false negatives might be fixed by making inference better in astroid directly. But there's an associated cost that can be very high, see this comment around the current implementation:

pylint/pylint/checkers/base/basic_checker.py

Lines 465 to 467 in 15a5ac0

# Heuristic: only run inference for names that begin with an uppercase char
# This reduces W0133's coverage, but retains acceptable runtime performance
# For more details, see: https://github.com/pylint-dev/pylint/issues/8073

The solution is probably going to be balancing better inference in astroid (in astroid), properly using/reusing the result we do infer (in pylint) and balancing acceptable performance and acceptable false negatives (in pylint and astroid).

@samskiter
Copy link

samskiter commented Nov 3, 2024 via email •
edited by Pierre-Sassoulas
Loading

@Pierre-Sassoulas
Copy link
Member

Not really no 😄 That would be "good first issues" (https://github.com/pylint-dev/pylint/issues?q=is%3Aissue%20state%3Aopen%20label%3A%22Good%20first%20issue%22)

@samskiter
Copy link

samskiter commented Nov 3, 2024 via email

@Pierre-Sassoulas
Copy link
Member

Probably contributing to this issue yourself even if this is hard for a first contribution, or offering a bounty somewhere so someone else does it (issue hunt, here, others I don't know.. ?). Creating comprehensive failing functional tests should not be too hard and is going to make the life of an implementer easier too.

@Pierre-Sassoulas Pierre-Sassoulas marked this as a duplicate of #10224 Feb 7, 2025
@Pierre-Sassoulas Pierre-Sassoulas mentioned this issue Feb 7, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Enhancement ✨ Improvement to a component False Negative 🦋 No message is emitted but something is wrong with the code Help wanted 🙏 Outside help would be appreciated, good for new contributors Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Unused function call extension LukasKrocek/pylint
4 participants
@samskiter @Pierre-Sassoulas @nedrebo @AlexeySalmin