diff --git a/ChangeLog b/ChangeLog index 8a244f3195..085c2c7ce2 100644 --- a/ChangeLog +++ b/ChangeLog @@ -82,6 +82,11 @@ Release date: TBA Closes #4668 +* No longer emit ``consider-using-with`` for ``ThreadPoolExecutor`` and ``ProcessPoolExecutor`` + as they have legitimate use cases without a ``with`` block. + + Closes #4689 + * Fix crash when inferring variables assigned in match patterns Closes #4685 diff --git a/doc/whatsnew/2.9.rst b/doc/whatsnew/2.9.rst index e3af4322b6..669b9c8bc7 100644 --- a/doc/whatsnew/2.9.rst +++ b/doc/whatsnew/2.9.rst @@ -99,3 +99,6 @@ Other Changes functions/methods. * Added various deprecated functions/methods for python 3.10, 3.7, 3.6 and 3.3 + +* No longer emit ``consider-using-with`` for ``ThreadPoolExecutor`` and ``ProcessPoolExecutor`` + as they have legitimate use cases without a ``with`` block. diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index 2225a7e4a7..8bba92238a 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -6,9 +6,10 @@ import itertools import tokenize from functools import reduce -from typing import List, Optional, Tuple, Union, cast +from typing import Dict, Iterator, List, NamedTuple, Optional, Tuple, Union, cast import astroid +from astroid.util import Uninferable from pylint import checkers, interfaces from pylint import utils as lint_utils @@ -41,8 +42,6 @@ "tarfile.TarFile", "tarfile.TarFile.open", "multiprocessing.context.BaseContext.Pool", - "concurrent.futures.thread.ThreadPoolExecutor", - "concurrent.futures.process.ProcessPoolExecutor", "subprocess.Popen", ) ) @@ -147,6 +146,33 @@ def _will_be_released_automatically(node: astroid.Call) -> bool: return func.qname() in callables_taking_care_of_exit +class ConsiderUsingWithStack(NamedTuple): + """Stack for objects that may potentially trigger a R1732 message + if they are not used in a ``with`` block later on.""" + + module_scope: Dict[str, astroid.NodeNG] = {} + class_scope: Dict[str, astroid.NodeNG] = {} + function_scope: Dict[str, astroid.NodeNG] = {} + + def __iter__(self) -> Iterator[Dict[str, astroid.NodeNG]]: + yield from (self.function_scope, self.class_scope, self.module_scope) + + def get_stack_for_frame( + self, frame: Union[astroid.FunctionDef, astroid.ClassDef, astroid.Module] + ): + """Get the stack corresponding to the scope of the given frame.""" + if isinstance(frame, astroid.FunctionDef): + return self.function_scope + if isinstance(frame, astroid.ClassDef): + return self.class_scope + return self.module_scope + + def clear_all(self) -> None: + """Convenience method to clear all stacks""" + for stack in self: + stack.clear() + + class RefactoringChecker(checkers.BaseTokenChecker): """Looks for code which can be refactored @@ -416,6 +442,7 @@ class RefactoringChecker(checkers.BaseTokenChecker): def __init__(self, linter=None): checkers.BaseTokenChecker.__init__(self, linter) self._return_nodes = {} + self._consider_using_with_stack = ConsiderUsingWithStack() self._init() self._never_returning_functions = None @@ -425,6 +452,7 @@ def _init(self): self._nested_blocks_msg = None self._reported_swap_nodes = set() self._can_simplify_bool_op = False + self._consider_using_with_stack.clear_all() def open(self): # do this in open since config not fully initialized in __init__ @@ -543,7 +571,12 @@ def process_tokens(self, tokens): if self.linter.is_message_enabled("trailing-comma-tuple"): self.add_message("trailing-comma-tuple", line=token.start[0]) + @utils.check_messages("consider-using-with") def leave_module(self, _): + # check for context managers that have been created but not used + self._emit_consider_using_with_if_needed( + self._consider_using_with_stack.module_scope + ) self._init() @utils.check_messages("too-many-nested-blocks") @@ -593,7 +626,14 @@ def visit_excepthandler(self, node): @utils.check_messages("redefined-argument-from-local") def visit_with(self, node): - for _, names in node.items: + for var, names in node.items: + if isinstance(var, astroid.Name): + for stack in self._consider_using_with_stack: + # We don't need to restrict the stacks we search to the current scope and outer scopes, + # as e.g. the function_scope stack will be empty when we check a ``with`` on the class level. + if var.name in stack: + del stack[var.name] + break if not names: continue for name in names.nodes_of_class(astroid.AssignName): @@ -818,9 +858,12 @@ def _check_simplifiable_ifexp(self, node): self.add_message("simplifiable-if-expression", node=node, args=(reduced_to,)) @utils.check_messages( - "too-many-nested-blocks", "inconsistent-return-statements", "useless-return" + "too-many-nested-blocks", + "inconsistent-return-statements", + "useless-return", + "consider-using-with", ) - def leave_functiondef(self, node): + def leave_functiondef(self, node: astroid.FunctionDef) -> None: # check left-over nested blocks stack self._emit_nested_blocks_message_if_needed(self._nested_blocks) # new scope = reinitialize the stack of nested blocks @@ -830,6 +873,19 @@ def leave_functiondef(self, node): # check for single return or return None at the end self._check_return_at_the_end(node) self._return_nodes[node.name] = [] + # check for context managers that have been created but not used + self._emit_consider_using_with_if_needed( + self._consider_using_with_stack.function_scope + ) + self._consider_using_with_stack.function_scope.clear() + + @utils.check_messages("consider-using-with") + def leave_classdef(self, _: astroid.ClassDef) -> None: + # check for context managers that have been created but not used + self._emit_consider_using_with_if_needed( + self._consider_using_with_stack.class_scope + ) + self._consider_using_with_stack.class_scope.clear() @utils.check_messages("stop-iteration-return") def visit_raise(self, node): @@ -1021,6 +1077,10 @@ def _emit_nested_blocks_message_if_needed(self, nested_blocks): args=(len(nested_blocks), self.config.max_nested_blocks), ) + def _emit_consider_using_with_if_needed(self, stack: Dict[str, astroid.NodeNG]): + for node in stack.values(): + self.add_message("consider-using-with", node=node) + @staticmethod def _duplicated_isinstance_types(node): """Get the duplicated types from the underlying isinstance calls. @@ -1282,12 +1342,22 @@ def _check_swap_variables(self, node): message = "consider-swap-variables" self.add_message(message, node=node) + @utils.check_messages( + "simplify-boolean-expression", + "consider-using-ternary", + "consider-swap-variables", + "consider-using-with", + ) + def visit_assign(self, node: astroid.Assign) -> None: + self._append_context_managers_to_stack(node) + self.visit_return(node) # remaining checks are identical as for return nodes + @utils.check_messages( "simplify-boolean-expression", "consider-using-ternary", "consider-swap-variables", ) - def visit_assign(self, node): + def visit_return(self, node: astroid.Return) -> None: self._check_swap_variables(node) if self._is_and_or_ternary(node.value): cond, truth_value, false_value = self._and_or_ternary_arguments(node.value) @@ -1317,9 +1387,54 @@ def visit_assign(self, node): ) self.add_message(message, node=node, args=(suggestion,)) - visit_return = visit_assign + def _append_context_managers_to_stack(self, node: astroid.Assign) -> None: + if _is_inside_context_manager(node): + # if we are inside a context manager itself, we assume that it will handle the resource management itself. + return + if isinstance(node.targets[0], (astroid.Tuple, astroid.List, astroid.Set)): + assignees = node.targets[0].elts + value = utils.safe_infer(node.value) + if value is None or not hasattr(value, "elts"): + # We cannot deduce what values are assigned, so we have to skip this + return + values = value.elts + else: + assignees = [node.targets[0]] + values = [node.value] + if Uninferable in (assignees, values): + return + for assignee, value in zip(assignees, values): + if not isinstance(value, astroid.Call): + continue + inferred = utils.safe_infer(value.func) + if not inferred or inferred.qname() not in CALLS_RETURNING_CONTEXT_MANAGERS: + continue + stack = self._consider_using_with_stack.get_stack_for_frame(node.frame()) + varname = ( + assignee.name + if isinstance(assignee, astroid.AssignName) + else assignee.attrname + ) + if varname in stack: + # variable was redefined before it was used in a ``with`` block + self.add_message( + "consider-using-with", + node=stack[varname], + ) + stack[varname] = value def _check_consider_using_with(self, node: astroid.Call): + if _is_inside_context_manager(node): + # if we are inside a context manager itself, we assume that it will handle the resource management itself. + return + if ( + node + in self._consider_using_with_stack.get_stack_for_frame( + node.frame() + ).values() + ): + # the result of this call was already assigned to a variable and will be checked when leaving the scope. + return inferred = utils.safe_infer(node.func) if not inferred: return @@ -1332,9 +1447,7 @@ def _check_consider_using_with(self, node: astroid.Call): and not _is_part_of_with_items(node) ) ) - if could_be_used_in_with and not ( - _is_inside_context_manager(node) or _will_be_released_automatically(node) - ): + if could_be_used_in_with and not _will_be_released_automatically(node): self.add_message("consider-using-with", node=node) def _check_consider_using_join(self, aug_assign): diff --git a/tests/functional/c/consider/consider_using_with.py b/tests/functional/c/consider/consider_using_with.py index 7ba58fdb0f..126bf2880d 100644 --- a/tests/functional/c/consider/consider_using_with.py +++ b/tests/functional/c/consider/consider_using_with.py @@ -8,7 +8,7 @@ import threading import urllib import zipfile -from concurrent import futures +from concurrent.futures import ThreadPoolExecutor, ProcessPoolExecutor def test_codecs_open(): @@ -112,6 +112,7 @@ class MyLockContext: """ The message must not be triggered if the resource allocation is done inside a context manager. """ + def __init__(self): self.lock = threading.Lock() @@ -140,16 +141,6 @@ def test_multiprocessing(): pass -def test_futures(): - _ = futures.ThreadPoolExecutor() # [consider-using-with] - with futures.ThreadPoolExecutor(): - pass - - _ = futures.ProcessPoolExecutor() # [consider-using-with] - with futures.ProcessPoolExecutor(): - pass - - def test_popen(): _ = subprocess.Popen("sh") # [consider-using-with] with subprocess.Popen("sh"): @@ -162,3 +153,65 @@ def test_suppress_in_exit_stack(): _ = stack.enter_context( open("/sys/firmware/devicetree/base/hwid,location", "r") ) # must not trigger + + +def test_futures(): + """ + Regression test for issue #4689. + ThreadPoolExecutor and ProcessPoolExecutor were formerly part of the callables that raised + the R1732 message if used outside a with block, but there are legitimate use cases where + Executor instances are used e.g. as a persistent background worker pool throughout the program. + """ + thread_executor = ThreadPoolExecutor() + thread_executor.submit(print, 1) + process_executor = ProcessPoolExecutor() + process_executor.submit(print, 2) + thread_executor.shutdown() + process_executor.shutdown() + + +pool = multiprocessing.Pool() # must not trigger, as it is used later on +with pool: + pass + + +global_pool = ( + multiprocessing.Pool() +) # must not trigger, will be used in nested scope + + +def my_nested_function(): + with global_pool: + pass + + +# this must also work for tuple unpacking +pool1, pool2 = ( + multiprocessing.Pool(), # must not trigger + multiprocessing.Pool(), # must not trigger +) + +with pool1: + pass + +with pool2: + pass + +unused_pool1, unused_pool2 = ( + multiprocessing.Pool(), # [consider-using-with] + multiprocessing.Pool(), # [consider-using-with] +) + +used_pool, unused_pool = ( + multiprocessing.Pool(), # must not trigger + multiprocessing.Pool(), # [consider-using-with] +) +with used_pool: + pass + +unused_pool, used_pool = ( + multiprocessing.Pool(), # [consider-using-with] + multiprocessing.Pool(), # must not trigger +) +with used_pool: + pass diff --git a/tests/functional/c/consider/consider_using_with.txt b/tests/functional/c/consider/consider_using_with.txt index 66c157231f..81de4266c4 100644 --- a/tests/functional/c/consider/consider_using_with.txt +++ b/tests/functional/c/consider/consider_using_with.txt @@ -1,22 +1,24 @@ -consider-using-with:15:9:test_codecs_open:Consider using 'with' for resource-allocating operations -consider-using-with:20:8:test_urlopen:Consider using 'with' for resource-allocating operations -consider-using-with:24:8:test_temporary_file:Consider using 'with' for resource-allocating operations -consider-using-with:28:8:test_named_temporary_file:Consider using 'with' for resource-allocating operations -consider-using-with:32:8:test_spooled_temporary_file:Consider using 'with' for resource-allocating operations -consider-using-with:36:8:test_temporary_directory:Consider using 'with' for resource-allocating operations -consider-using-with:40:12:test_zipfile:Consider using 'with' for resource-allocating operations -consider-using-with:41:8:test_zipfile:Consider using 'with' for resource-allocating operations -consider-using-with:45:12:test_pyzipfile:Consider using 'with' for resource-allocating operations -consider-using-with:50:8:test_pyzipfile:Consider using 'with' for resource-allocating operations -consider-using-with:57:9:test_tarfile:Consider using 'with' for resource-allocating operations -consider-using-with:63:9:test_tarfile:Consider using 'with' for resource-allocating operations -consider-using-with:72:4:test_lock_acquisition:Consider using 'with' for resource-allocating operations -consider-using-with:79:4:test_lock_acquisition:Consider using 'with' for resource-allocating operations -consider-using-with:86:4:test_lock_acquisition:Consider using 'with' for resource-allocating operations -consider-using-with:93:4:test_lock_acquisition:Consider using 'with' for resource-allocating operations -consider-using-with:128:8:test_multiprocessing:Consider using 'with' for resource-allocating operations -consider-using-with:133:4:test_multiprocessing:Consider using 'with' for resource-allocating operations -consider-using-with:138:4:test_multiprocessing:Consider using 'with' for resource-allocating operations -consider-using-with:144:8:test_futures:Consider using 'with' for resource-allocating operations -consider-using-with:148:8:test_futures:Consider using 'with' for resource-allocating operations -consider-using-with:154:8:test_popen:Consider using 'with' for resource-allocating operations +consider-using-with:15:9:test_codecs_open:Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:20:8:test_urlopen:Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:24:8:test_temporary_file:Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:28:8:test_named_temporary_file:Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:32:8:test_spooled_temporary_file:Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:36:8:test_temporary_directory:Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:40:12:test_zipfile:Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:41:8:test_zipfile:Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:45:12:test_pyzipfile:Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:50:8:test_pyzipfile:Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:57:9:test_tarfile:Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:63:9:test_tarfile:Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:72:4:test_lock_acquisition:Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:79:4:test_lock_acquisition:Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:86:4:test_lock_acquisition:Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:93:4:test_lock_acquisition:Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:129:8:test_multiprocessing:Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:134:4:test_multiprocessing:Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:139:4:test_multiprocessing:Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:145:8:test_popen:Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:201:4::Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:202:4::Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:207:4::Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:213:4::Consider using 'with' for resource-allocating operations:HIGH