[contextmanager-generator-missing-cleanup][new feature] Warn about @contextlib.contextmanager without try/finally in generator functions #9133

rhyn0 · 2023-10-08T20:56:46Z

add: initial layout for new error
add: test cases for new checker
add: first pass at new checker

Type of Changes

	Type
✓	✨ New feature

Description

Add a checker for contextlib.contextmanager decorated generator functions that don't have cleanup handling for GeneratorExit.

Closes #2832

created new _BasicChecker inheriting class registered this new linter like the rest in __init__.py

original example case included a slightly modified positive case 2 negative cases where exception is handled properly

Checks that contextmanager decorated functions that are generators handle GeneratorExit cleanly

Pierre-Sassoulas

Thank you for your contribution, a lot of work went into this clearly. Would you mind adding a good/bad example in https://github.com/pylint-dev/pylint/tree/main/doc/data/messages too ? It's not enforced yet but we're close to being fully documented :)

pylint/checkers/base/function_checker.py

tests/functional/c/contextmanager_generator_missing_cleanup.py

pylint/checkers/base/function_checker.py

Changed the detection logic of a bad generator function to test whether there was attempt at cleanup code

removed extra pylint disables from heading of test cases

output files got updated due to newlines above

…ng_cleanup' error

codecov · 2024-03-03T21:50:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.83%. Comparing base (e31a155) to head (3bd7132).
Report is 181 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9133      +/-   ##
==========================================
+ Coverage   95.76%   95.83%   +0.07%     
==========================================
  Files         173      174       +1     
  Lines       18665    18887     +222     
==========================================
+ Hits        17874    18101     +227     
+ Misses        791      786       -5

Files	Coverage Δ
pylint/checkers/base/__init__.py	`100.00% <100.00%> (ø)`
pylint/checkers/base/function_checker.py	`100.00% <100.00%> (ø)`

... and 39 files with indirect coverage changes

…h items

pylint/checkers/base/function_checker.py

…ception

Pierre-Sassoulas · 2024-05-04T20:20:25Z

Does this need reviews or do you need to polish some things still @rhyn0 ? I would be glad to have this in 3.2.0 😄

jacobtylerwalls · 2024-05-04T20:48:49Z

From what I can tell it's very close:

some unresolved documentation comments
docs check is failing
TODO statements about yield and asyncwith

Happy to give the whole thing a once-over when those are in 👍

tests/functional/c/consider/consider_using_with.py

rhyn0 · 2024-05-05T19:43:42Z

Failing ReadTheDocs error seems to be the following

doc/user_guide/messages/warning/implicit-str-concat.rst:55: WARNING: Lexing literal_block '[STRING_CONSTANT]\ncheck-str-concat-over-line-jumps = yes' as "toml" resulted in an error at token: 'y'. Retrying in relaxed mode.

Not sure if I need to change this here

jacobtylerwalls

Looking forward to merging this shortly. Just some minor feedback.

doc/data/messages/c/contextmanager-generator-missing-cleanup/details.rst

doc/whatsnew/fragments/2832.feature

pylint/checkers/base/function_checker.py

tests/functional/c/consider/consider_using_with_open.py

jacobtylerwalls · 2024-05-09T01:58:00Z

Not sure if I need to change this here

Good call, let's not block your work on that. If it hasn't been fixed on main yet, please open an issue!

github-actions · 2024-05-11T18:14:38Z

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

This comment was generated for commit 3bd7132

jacobtylerwalls

Thanks for the new check @rhyn0!

rhyn0 added 4 commits October 7, 2023 23:32

add: initial layout for new error

062373e

created new _BasicChecker inheriting class registered this new linter like the rest in __init__.py

add: test cases for new checker

b6774f6

original example case included a slightly modified positive case 2 negative cases where exception is handled properly

add: first pass at new checker

b0547eb

Checks that contextmanager decorated functions that are generators handle GeneratorExit cleanly

add: towncrier fragment for changes

72068a2

Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label Oct 8, 2023

This comment has been minimized.

Sign in to view

Pierre-Sassoulas reviewed Oct 9, 2023

View reviewed changes

rhyn0 added 4 commits October 13, 2023 19:23

add: suggestions, better logic for this warning

81c7ab6

Changed the detection logic of a bad generator function to test whether there was attempt at cleanup code

add: good and bad example under doc

9be51c2

update: functional tests to use new message

314158b

removed extra pylint disables from heading of test cases

update: other tests to exclude new warning

74841ab

output files got updated due to newlines above

rhyn0 marked this pull request as ready for review October 14, 2023 02:29

This comment has been minimized.

Sign in to view

change: keep new checker name under 'basic'

f0d9fad

jacobtylerwalls added the Needs decision 🔒 Needs a decision before implemention or rejection label Nov 5, 2023

jacobtylerwalls added this to the 3.2.0 milestone Feb 23, 2024

rhyn0 added 4 commits March 3, 2024 13:09

change: update positive/negative examples to new spec for this feature

06204d7

Fix other pylint rule violations in test data, update expected output

9928473

change: new checker logic for refined 'contextmanager_generator_missi…

6c95035

…ng_cleanup' error

update: test files with unused suppress flags, add suppress flags

eef0ac0

rhyn0 requested a review from DudeNr33 as a code owner March 3, 2024 21:43

rhyn0 added 4 commits March 3, 2024 13:59

fix: CI docstring spelling errors

8022d8d

add: test cases for code coverage, order of functions and various Wit…

3ec527d

…h items

disable wrong spelling for AsyncWith

f1b6eb9

add more test cases from Primer errors

7f3874d

jacobtylerwalls reviewed Mar 5, 2024

View reviewed changes

pylint/checkers/base/function_checker.py Outdated Show resolved Hide resolved

pylint/checkers/base/function_checker.py Outdated Show resolved Hide resolved

rhyn0 added 2 commits March 23, 2024 21:54

add negative test case for decorated usage of contextmanager

4ac898a

rework checker to use simpler logic from utils.safe_infer

5c50505

This comment has been minimized.

Sign in to view

rhyn0 added 4 commits March 23, 2024 23:10

reword feature fragment, explain purpose more in depth

de54fc4

fix comment in good examples

f3f0876

use inference to check for handler of GeneratorExit

3c365ce

don't raise for contextmanagers that handle base Exception or bare Ex…

8730bc7

…ception

This comment has been minimized.

Sign in to view

jacobtylerwalls removed the Work in progress label Apr 2, 2024

jacobtylerwalls added the Work in progress label May 4, 2024

jacobtylerwalls reviewed May 4, 2024

View reviewed changes

tests/functional/c/consider/consider_using_with.py Outdated Show resolved Hide resolved

rhyn0 added 3 commits May 5, 2024 11:49

reword top level error message

1f3cb77

remove unneccessary disable comment, update expected output

03c5562

add details and related links to error message documentation

d87ebac

This comment has been minimized.

Sign in to view

remove TODO from checker

4d36753

This comment has been minimized.

Sign in to view

jacobtylerwalls reviewed May 9, 2024

View reviewed changes

jacobtylerwalls removed the Work in progress label May 9, 2024

rhyn0 added 3 commits May 11, 2024 11:01

revert test breaking formatting changes

db8db8f

update: documentation about new checker, grammar

b89e7df

fix: make checking for generators with try blocks cheaper

3bd7132

jacobtylerwalls approved these changes May 12, 2024

View reviewed changes

jacobtylerwalls merged commit d5ad55d into pylint-dev:main May 12, 2024

Uh oh!

[contextmanager-generator-missing-cleanup][new feature] Warn about @contextlib.contextmanager without try/finally in generator functions #9133

[contextmanager-generator-missing-cleanup][new feature] Warn about @contextlib.contextmanager without try/finally in generator functions #9133

Uh oh!

Conversation

rhyn0 commented Oct 8, 2023

Type of Changes

Description

Uh oh!

This comment has been minimized.

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

codecov bot commented Mar 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

Pierre-Sassoulas commented May 4, 2024

Uh oh!

jacobtylerwalls commented May 4, 2024

Uh oh!

Uh oh!

This comment has been minimized.

rhyn0 commented May 5, 2024

Uh oh!

This comment has been minimized.

jacobtylerwalls left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jacobtylerwalls commented May 9, 2024

Uh oh!

github-actions bot commented May 11, 2024

Uh oh!

jacobtylerwalls left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Mar 3, 2024 •

edited

Loading