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

Prevent usage of return, yield in try/finally #8260

Closed
hofrob opened this issue Feb 10, 2023 · 11 comments · Fixed by #8440 or #9093
Closed

Prevent usage of return, yield in try/finally #8260

hofrob opened this issue Feb 10, 2023 · 11 comments · Fixed by #8440 or #9093
Assignees
Labels
Checkers Related to a checker Enhancement ✨ Improvement to a component Good first issue Friendly and approachable by new contributors
Milestone

Comments

@hofrob
Copy link
Contributor

hofrob commented Feb 10, 2023

Current problem

Code in finally blocks will always be executed. If there's a return inside the try, it's possible to overwrite this return value with a return in the finally.

def add(a: int, b: int) -> int:
    try:
        return a + b
    finally:
        return 0


if __name__ == "__main__":
    print(add(1, 2))  # those args will be ignored; "0" is printed

Desired solution

There should not be any return or yield statements in a finally block.

Additional context

https://docs.python.org/3/reference/compound_stmts.html#finally-clause

The return value of a function is determined by the last return statement executed.

@hofrob hofrob added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Feb 10, 2023
@nickdrozd
Copy link
Contributor

Sounds like a good idea to me. I did a quick search of a variety of repos and I couldn't find a single instance, so it seems to be a rare pattern.

@nickdrozd nickdrozd added Enhancement ✨ Improvement to a component Checkers Related to a checker Good first issue Friendly and approachable by new contributors and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Feb 10, 2023
@deepgohil
Copy link

I would like to solve this issue.

@Pierre-Sassoulas
Copy link
Member

I assigned you to it @deepgohil :)

@deepgohil
Copy link

Can you tell me where I have to change the code? I want to know folder name

@Pierre-Sassoulas
Copy link
Member

You can create or modify a checker in pylint/checkers, or create and modify an extension in pylint/extensions. This check feel like a nasty underhanded bug that could be a default checker of warning level imo.

Design by ChatGPT would indicate that it could be in the basic checker:

In what pylint checker should a check for 'return-superseded-by-finally' should go ?

The return-superseded-by-finally check in Pylint belongs to the "basic" category of checks. The "basic" checks are a set of checks that Pylint performs by default, without the need for any additional configuration. This check reports a warning when a return statement appears in a function that also has a finally block.

But I think that ChatGPT is basic and might be giving bad design advise here. I don't know where it should really go though. Or if it should really be called 'return-superseded-by-finally'.

@ollie-iterators
Copy link
Contributor

What should be in the finally block instead of return?

@Pierre-Sassoulas Pierre-Sassoulas added the Needs specification 🔐 Accepted as a potential improvement, and needs to specify edge cases, message names, etc. label Mar 7, 2023
@Pierre-Sassoulas
Copy link
Member

What should be in the finally block instead of return?

The finally block should be removed for example like this:

def add(a: int, b: int) -> int:
    try:
        return a + b
    except:
        return 0

@deepgohil do you have an opinion regarding the name and the checker it should go in ? (maybe a new one ?)

@hofrob
Copy link
Contributor Author

hofrob commented Mar 7, 2023

I'd say in general: finally blocks should not contain any yield or return statements. I would not make any suggestion on how to fix it.

I'm also wondering where to put this new rules 🙂

@Pierre-Sassoulas
Copy link
Member

Regarding the name it could also be yield as @hofrob pointed out, so return-superseded-by-finally is not working. Maybe superseded-by-finally shadowed-by-finally, return-or-yield-in-finally ?

@nickdrozd
Copy link
Contributor

return-or-yield-in-finally sounds the clearest, in the sense that it answers "what needs to be changed to make this warning go away".

@hofrob
Copy link
Contributor Author

hofrob commented Mar 11, 2023

Since nobody is working on this, I'd try to come up with something. Let's see how this goes.

@Pierre-Sassoulas Pierre-Sassoulas added this to the 3.0.0a7 milestone May 22, 2023
@Pierre-Sassoulas Pierre-Sassoulas removed the Needs specification 🔐 Accepted as a potential improvement, and needs to specify edge cases, message names, etc. label May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Checkers Related to a checker Enhancement ✨ Improvement to a component Good first issue Friendly and approachable by new contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants