add checker for return in finally

[hofrob]

Type of Changes

	Type
✓	✨ New feature

Description

Closes #8260

[codecov]

Codecov Report

Merging #8440 (a1d3404) into main (d7e9fd6) will increase coverage by 0.08%.
The diff coverage is 100.00%.

❗ Current head a1d3404 differs from pull request most recent head 32213e9. Consider uploading reports for the commit 32213e9 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8440      +/-   ##
==========================================
+ Coverage   95.82%   95.90%   +0.08%     
==========================================
  Files         173      174       +1     
  Lines       18375    18370       -5     
==========================================
+ Hits        17607    17618      +11     
+ Misses        768      752      -16     

Impacted Files	Coverage Δ
pylint/checkers/base/basic_checker.py	97.91% <100.00%> (+0.04%)	⬆️

... and 53 files with indirect coverage changes

[Pierre-Sassoulas]

Thank you for working on this, it looks pretty good already !

[hofrob]

I'm not too happy with those examples. The problem is, that pylint should probably not make recommendations on how to fix it. The only real suggestion this checker could offer, would be "the finally should only contain shutdown/close commands". If there's a good source where it says what to do in more detail, I'd add it.

I added this checker to the pylint/checkers/refactoring/refactoring_checker.py module, but if you have another suggestion, I'll gladly move it. I'm also walking through all statements in a finalbody. This might impact performance, though I hope that projects do not have a lot of logic inside of their finally clauses 😬.

Other than that, let me know if I should improve the wording or anything else really.

The fact that this new checker only produces one more message in all the checked repos is a good sign in my opinion. I'm not sure if the usage of yield in finally in pycopg2 is a legitimate one?

[Pierre-Sassoulas]

It's a very good sign when there's only one new message raised and it's hard to tell if it's a false positive :)

[hofrob]

After looking at the generated docs, I thought adding comments to the bad example might be a bit more helpful.

[Pierre-Sassoulas]

I like the embedded comment in the doc

[nickdrozd]

The primer is showing one hit that looks like a false positive:

    def _exit_gen(self) -> PQGen[None]:
        """
        Exit current pipeline by sending a Sync and fetch back all remaining results.
        """
        try:
            self._enqueue_sync()
            yield from self._communicate_gen()
        finally:
            # No need to force flush since we emitted a sync just before.
            yield from self._fetch_gen(flush=False)

That function yields at least one item in any case, and possibly more. But I don't see any bug there.

I think the warning should only trigger if there is a return somewhere.

[hofrob]

    def _exit_gen(self) -> PQGen[None]:
        """
        Exit current pipeline by sending a Sync and fetch back all remaining results.
        """
        try:
            self._enqueue_sync()
            yield from self._communicate_gen()
        finally:
            # No need to force flush since we emitted a sync just before.
            yield from self._fetch_gen(flush=False)

I'm trying to understand why this yield is in a finally clause. It more or less looks like they're trying to skip "bare-except" lint warnings this way 😀. Because that's what this actually does, right?

So I'm still not convinced that this is actually a false positive. yield from self._fetch_gen(flush=False) should come after the try/except with actual exceptions they want to catch.

😕

[nickdrozd]

I'm trying to understand why this yield is in a finally clause. It more or less looks like they're trying to skip "bare-except" lint warnings this way grinning. Because that's what this actually does, right?

I'm not sure why it's like that. It's not skipping bare-except because it doesn't actually catch anything. So if there's an exception in try, it will interfere with the yield in finally? But self._fetch_gen needs to get called in any case, which might result in side effects.

Barring side effects, I can't think of a good reason to do it this way. So maybe it isn't a false positive. But I could be wrong.

[jacobtylerwalls]

Thanks for this!

Agree with @nickdrozd we should just deal with return. I don't think yield is really a problem in finally. I'd be happy to merge this if just affecting return.

[hofrob]

Ok, I see that including yield is a step too far. I removed references to yield now but will do a final (:wink:) check and wait for the CI to finish.

Thanks for chiming in! This PR has bothered me for weeks. But I totally agree now, that this should only look at return statements.

[Pierre-Sassoulas]

The doc fail is due to a bug that was fixed on main :)

[github-actions]

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 32213e9

[jacobtylerwalls]

Thanks for the contribution!

All comments addressed

[Pierre-Sassoulas]

Great new check, thank you !