From 04dba4f7b8bcf6a83a57f2a25f3a9db61ef9e20e Mon Sep 17 00:00:00 2001 From: CrazyBolillo Date: Tue, 6 Feb 2024 23:46:03 -0600 Subject: [PATCH 01/15] New message `use-yield-from` A new message has been added to suggest refactoring when yielding from a for loop can be replaced by 'yield from'. This message is only emitted when there are no other statements in the containing for loop. Closes #9229. --- doc/whatsnew/fragments/9229.feature | 3 +++ .../refactoring/refactoring_checker.py | 23 +++++++++++++++++++ tests/functional/u/use/use_yield_from.py | 23 +++++++++++++++++++ tests/functional/u/use/use_yield_from.txt | 2 ++ 4 files changed, 51 insertions(+) create mode 100644 doc/whatsnew/fragments/9229.feature create mode 100644 tests/functional/u/use/use_yield_from.py create mode 100644 tests/functional/u/use/use_yield_from.txt diff --git a/doc/whatsnew/fragments/9229.feature b/doc/whatsnew/fragments/9229.feature new file mode 100644 index 0000000000..22d059ecdf --- /dev/null +++ b/doc/whatsnew/fragments/9229.feature @@ -0,0 +1,3 @@ +New message `use-yield-from` added to the refactoring checker. This message is emitted when yielding from a loop can be replaced by `yield from`. + +Closes #9229. diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index 97e509025c..682d8fdf96 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -483,6 +483,11 @@ class RefactoringChecker(checkers.BaseTokenChecker): "value by index lookup. " "The value can be accessed directly instead.", ), + "R1737": ( + "Consider directly using 'yield from %s' instead", + "use-yield-from", + "Emitted when yielding from a loop can be replaced by yield from.", + ), } options = ( ( @@ -1123,6 +1128,24 @@ def visit_call(self, node: nodes.Call) -> None: self._check_use_list_literal(node) self._check_use_dict_literal(node) + @utils.only_required_for_messages("use-yield-from") + def visit_yield(self, node: nodes.Yield) -> None: + parent = node.parent.parent + if not isinstance(parent, nodes.For): + return + + if len(parent.body) != 1: + return + + if parent.target.name == node.value.name: + gen = "" + if isinstance(parent.iter, nodes.Name): + gen = parent.iter.name + elif isinstance(parent.iter, nodes.Call): + gen = f"{parent.iter.func.name}()" + + self.add_message("use-yield-from", node.lineno, node, gen) + @staticmethod def _has_exit_in_scope(scope: nodes.LocalsDictNodeNG) -> bool: exit_func = scope.locals.get("exit") diff --git a/tests/functional/u/use/use_yield_from.py b/tests/functional/u/use/use_yield_from.py new file mode 100644 index 0000000000..2c34c79396 --- /dev/null +++ b/tests/functional/u/use/use_yield_from.py @@ -0,0 +1,23 @@ +# pylint: disable=missing-docstring, import-error + +from magic import shazam, turbogen + + +def bad(generator): + for item in generator: + yield item # [use-yield-from] + + +def out_of_names(): + for item in turbogen(): + yield item # [use-yield-from] + + +def good(generator): + for item in generator: + shazam() + yield item + + +def yield_something(): + yield 5 diff --git a/tests/functional/u/use/use_yield_from.txt b/tests/functional/u/use/use_yield_from.txt new file mode 100644 index 0000000000..e542ff2ae8 --- /dev/null +++ b/tests/functional/u/use/use_yield_from.txt @@ -0,0 +1,2 @@ +use-yield-from:8:8:8:18:bad:Consider directly using 'yield from generator' instead:UNDEFINED +use-yield-from:13:8:13:18:out_of_names:Consider directly using 'yield from turbogen()' instead:UNDEFINED From bf33b169f5b4ceefbfaebaa008d547d6479308c2 Mon Sep 17 00:00:00 2001 From: CrazyBolillo Date: Thu, 8 Feb 2024 12:58:04 -0600 Subject: [PATCH 02/15] Update confidence level for 'use-yield-from' --- pylint/checkers/refactoring/refactoring_checker.py | 2 +- tests/functional/u/use/use_yield_from.txt | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index 682d8fdf96..efcb3b4a73 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -1144,7 +1144,7 @@ def visit_yield(self, node: nodes.Yield) -> None: elif isinstance(parent.iter, nodes.Call): gen = f"{parent.iter.func.name}()" - self.add_message("use-yield-from", node.lineno, node, gen) + self.add_message("use-yield-from", node.lineno, node, gen, confidence=HIGH) @staticmethod def _has_exit_in_scope(scope: nodes.LocalsDictNodeNG) -> bool: diff --git a/tests/functional/u/use/use_yield_from.txt b/tests/functional/u/use/use_yield_from.txt index e542ff2ae8..6055e4e4d3 100644 --- a/tests/functional/u/use/use_yield_from.txt +++ b/tests/functional/u/use/use_yield_from.txt @@ -1,2 +1,2 @@ -use-yield-from:8:8:8:18:bad:Consider directly using 'yield from generator' instead:UNDEFINED -use-yield-from:13:8:13:18:out_of_names:Consider directly using 'yield from turbogen()' instead:UNDEFINED +use-yield-from:8:8:8:18:bad:Consider directly using 'yield from generator' instead:HIGH +use-yield-from:13:8:13:18:out_of_names:Consider directly using 'yield from turbogen()' instead:HIGH From 9ae722f67fdb4e8d6b6c18d57f8aabcc07f2ab70 Mon Sep 17 00:00:00 2001 From: CrazyBolillo Date: Thu, 8 Feb 2024 19:13:14 -0600 Subject: [PATCH 03/15] Better handling for other loop targets and yield types It is also possible to call attributes so the case has been handled regardless of the nesting level. Yield value may not always be a name as the result of function calls or other elements may be yielded, therefore nodes will be discarded if they don't yield a name. --- .../refactoring/refactoring_checker.py | 28 ++++++++++++++++--- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index efcb3b4a73..c85f8ee8fe 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -1130,6 +1130,9 @@ def visit_call(self, node: nodes.Call) -> None: @utils.only_required_for_messages("use-yield-from") def visit_yield(self, node: nodes.Yield) -> None: + if not isinstance(node.value, nodes.Name): + return + parent = node.parent.parent if not isinstance(parent, nodes.For): return @@ -1138,13 +1141,30 @@ def visit_yield(self, node: nodes.Yield) -> None: return if parent.target.name == node.value.name: - gen = "" + generator_name = "" if isinstance(parent.iter, nodes.Name): - gen = parent.iter.name + generator_name = parent.iter.name elif isinstance(parent.iter, nodes.Call): - gen = f"{parent.iter.func.name}()" + if isinstance(parent.iter.func, nodes.Attribute): + generator_name = self._get_full_name_from_attribute( + parent.iter.func + ) + else: + generator_name = f"{parent.iter.func.name}()" - self.add_message("use-yield-from", node.lineno, node, gen, confidence=HIGH) + self.add_message( + "use-yield-from", node.lineno, node, generator_name, confidence=HIGH + ) + + @staticmethod + def _get_full_name_from_attribute(node: nodes.Attribute) -> str: + name = f"{node.attrname}()" + current_node = node.expr + while isinstance(current_node, nodes.Attribute): + name = f"{current_node.attrname}.{name}" + current_node = current_node.expr + + return f"{current_node.name}.{name}" @staticmethod def _has_exit_in_scope(scope: nodes.LocalsDictNodeNG) -> bool: From 4fd42d815954bb87546f91ed31c6afdf9e1e2bc9 Mon Sep 17 00:00:00 2001 From: CrazyBolillo Date: Thu, 8 Feb 2024 19:16:45 -0600 Subject: [PATCH 04/15] Disable use-yield-from on unrelated tests --- tests/functional/b/bad_reversed_sequence.py | 2 +- .../functional/c/consider/consider_using_enumerate.py | 2 +- .../ext/docparams/return/missing_return_doc_Sphinx.py | 2 +- tests/functional/i/iterable_context_py36.py | 2 +- tests/functional/n/non/non_iterator_returned.py | 2 +- tests/functional/s/stop_iteration_inside_generator.py | 2 +- tests/functional/u/undefined/undefined_variable.py | 2 +- .../functional/u/unpacking/unpacking_non_sequence.py | 2 +- .../u/use/use_implicit_booleaness_not_len.py | 2 +- tests/functional/u/use/use_yield_from.py | 11 +++++++++++ tests/functional/u/use/use_yield_from.txt | 6 ++++-- tests/functional/y/yield_assign.py | 1 + 12 files changed, 25 insertions(+), 11 deletions(-) diff --git a/tests/functional/b/bad_reversed_sequence.py b/tests/functional/b/bad_reversed_sequence.py index c4380491e9..d9c3c68b3f 100644 --- a/tests/functional/b/bad_reversed_sequence.py +++ b/tests/functional/b/bad_reversed_sequence.py @@ -1,6 +1,6 @@ """ Checks that reversed() receive proper argument """ # pylint: disable=missing-docstring -# pylint: disable=too-few-public-methods +# pylint: disable=too-few-public-methods,use-yield-from from collections import deque, OrderedDict from enum import IntEnum diff --git a/tests/functional/c/consider/consider_using_enumerate.py b/tests/functional/c/consider/consider_using_enumerate.py index 1b8bb4c6b1..b49ffd9f0b 100644 --- a/tests/functional/c/consider/consider_using_enumerate.py +++ b/tests/functional/c/consider/consider_using_enumerate.py @@ -1,6 +1,6 @@ """Emit a message for iteration through range and len is encountered.""" -# pylint: disable=missing-docstring, import-error, unsubscriptable-object, too-few-public-methods, unnecessary-list-index-lookup +# pylint: disable=missing-docstring, import-error, unsubscriptable-object, too-few-public-methods, unnecessary-list-index-lookup, use-yield-from def bad(): iterable = [1, 2, 3] diff --git a/tests/functional/ext/docparams/return/missing_return_doc_Sphinx.py b/tests/functional/ext/docparams/return/missing_return_doc_Sphinx.py index 41b0ce1ae5..1351d1cef8 100644 --- a/tests/functional/ext/docparams/return/missing_return_doc_Sphinx.py +++ b/tests/functional/ext/docparams/return/missing_return_doc_Sphinx.py @@ -1,7 +1,7 @@ """Tests for missing-return-doc and missing-return-type-doc for Sphinx style docstrings""" # pylint: disable=function-redefined, invalid-name, undefined-variable, missing-function-docstring # pylint: disable=unused-argument, disallowed-name, too-few-public-methods, missing-class-docstring -# pylint: disable=unnecessary-pass +# pylint: disable=unnecessary-pass, use-yield-from import abc diff --git a/tests/functional/i/iterable_context_py36.py b/tests/functional/i/iterable_context_py36.py index d50d3da981..ecb48a241f 100644 --- a/tests/functional/i/iterable_context_py36.py +++ b/tests/functional/i/iterable_context_py36.py @@ -1,4 +1,4 @@ -# pylint: disable=missing-docstring,too-few-public-methods,unused-variable,unnecessary-comprehension +# pylint: disable=missing-docstring,too-few-public-methods,unused-variable,unnecessary-comprehension,use-yield-from import asyncio class AIter: diff --git a/tests/functional/n/non/non_iterator_returned.py b/tests/functional/n/non/non_iterator_returned.py index 3bc24a23e0..7dffd711ec 100644 --- a/tests/functional/n/non/non_iterator_returned.py +++ b/tests/functional/n/non/non_iterator_returned.py @@ -1,6 +1,6 @@ """Check non-iterators returned by __iter__ """ -# pylint: disable=too-few-public-methods, missing-docstring, consider-using-with, import-error +# pylint: disable=too-few-public-methods, missing-docstring, consider-using-with, import-error, use-yield-from from uninferable import UNINFERABLE class FirstGoodIterator: diff --git a/tests/functional/s/stop_iteration_inside_generator.py b/tests/functional/s/stop_iteration_inside_generator.py index fcd20a6836..4b034e2506 100644 --- a/tests/functional/s/stop_iteration_inside_generator.py +++ b/tests/functional/s/stop_iteration_inside_generator.py @@ -2,7 +2,7 @@ Test that no StopIteration is raised inside a generator """ # pylint: disable=missing-docstring,invalid-name,import-error, try-except-raise, wrong-import-position -# pylint: disable=not-callable,raise-missing-from,broad-exception-raised +# pylint: disable=not-callable,raise-missing-from,broad-exception-raised,use-yield-from import asyncio class RebornStopIteration(StopIteration): diff --git a/tests/functional/u/undefined/undefined_variable.py b/tests/functional/u/undefined/undefined_variable.py index e1b66910fc..194de114d3 100644 --- a/tests/functional/u/undefined/undefined_variable.py +++ b/tests/functional/u/undefined/undefined_variable.py @@ -1,7 +1,7 @@ # pylint: disable=missing-docstring, multiple-statements, import-outside-toplevel # pylint: disable=too-few-public-methods, bare-except, broad-except # pylint: disable=using-constant-test, import-error, global-variable-not-assigned, unnecessary-comprehension -# pylint: disable=unnecessary-lambda-assignment +# pylint: disable=unnecessary-lambda-assignment, use-yield-from from typing import TYPE_CHECKING diff --git a/tests/functional/u/unpacking/unpacking_non_sequence.py b/tests/functional/u/unpacking/unpacking_non_sequence.py index feb465ecbe..0a13c656c8 100644 --- a/tests/functional/u/unpacking/unpacking_non_sequence.py +++ b/tests/functional/u/unpacking/unpacking_non_sequence.py @@ -1,6 +1,6 @@ """Check unpacking non-sequences in assignments. """ -# pylint: disable=too-few-public-methods, invalid-name, attribute-defined-outside-init, unused-variable +# pylint: disable=too-few-public-methods, invalid-name, attribute-defined-outside-init, unused-variable, use-yield-from # pylint: disable=using-constant-test, missing-docstring, wrong-import-order,wrong-import-position,no-else-return from os import rename as nonseq_func from functional.u.unpacking.unpacking import nonseq diff --git a/tests/functional/u/use/use_implicit_booleaness_not_len.py b/tests/functional/u/use/use_implicit_booleaness_not_len.py index 79547d99e1..1261aa3014 100644 --- a/tests/functional/u/use/use_implicit_booleaness_not_len.py +++ b/tests/functional/u/use/use_implicit_booleaness_not_len.py @@ -1,4 +1,4 @@ -# pylint: disable=too-few-public-methods,import-error, missing-docstring +# pylint: disable=too-few-public-methods,import-error, missing-docstring, use-yield-from # pylint: disable=useless-super-delegation,wrong-import-position,invalid-name, wrong-import-order, condition-evals-to-constant if len('TEST'): # [use-implicit-booleaness-not-len] diff --git a/tests/functional/u/use/use_yield_from.py b/tests/functional/u/use/use_yield_from.py index 2c34c79396..dcaf72bfa5 100644 --- a/tests/functional/u/use/use_yield_from.py +++ b/tests/functional/u/use/use_yield_from.py @@ -1,5 +1,6 @@ # pylint: disable=missing-docstring, import-error +import factory from magic import shazam, turbogen @@ -21,3 +22,13 @@ def good(generator): def yield_something(): yield 5 + + +def yield_attr(): + for item in factory.gen(): + yield item # [use-yield-from] + + +def yield_attr_nested(): + for item in factory.kiwi.gen(): + yield item # [use-yield-from] diff --git a/tests/functional/u/use/use_yield_from.txt b/tests/functional/u/use/use_yield_from.txt index 6055e4e4d3..2e08eceec1 100644 --- a/tests/functional/u/use/use_yield_from.txt +++ b/tests/functional/u/use/use_yield_from.txt @@ -1,2 +1,4 @@ -use-yield-from:8:8:8:18:bad:Consider directly using 'yield from generator' instead:HIGH -use-yield-from:13:8:13:18:out_of_names:Consider directly using 'yield from turbogen()' instead:HIGH +use-yield-from:9:8:9:18:bad:Consider directly using 'yield from generator' instead:HIGH +use-yield-from:14:8:14:18:out_of_names:Consider directly using 'yield from turbogen()' instead:HIGH +use-yield-from:29:8:29:18:yield_attr:Consider directly using 'yield from factory.gen()' instead:HIGH +use-yield-from:34:8:34:18:yield_attr_nested:Consider directly using 'yield from factory.kiwi.gen()' instead:HIGH diff --git a/tests/functional/y/yield_assign.py b/tests/functional/y/yield_assign.py index e7a938c692..841fe5db31 100644 --- a/tests/functional/y/yield_assign.py +++ b/tests/functional/y/yield_assign.py @@ -1,3 +1,4 @@ +# pylint: disable=use-yield-from """https://www.logilab.org/ticket/8771""" From 2f890603386268fcfc39820b029d31831affbdb1 Mon Sep 17 00:00:00 2001 From: CrazyBolillo Date: Fri, 9 Feb 2024 20:18:19 -0600 Subject: [PATCH 05/15] Don't show generator name on message There are too many possible generators to use in a for loop and handling all the cases to provide a human friendly display name in emitted messages seems too cumbersome compared to the benefit it brings, specially considering this message will have a doc page with examples. --- .../refactoring/refactoring_checker.py | 33 ++----------------- tests/functional/u/use/use_yield_from.py | 5 +++ tests/functional/u/use/use_yield_from.txt | 9 ++--- 3 files changed, 13 insertions(+), 34 deletions(-) diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index c85f8ee8fe..698d6d3c81 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -484,7 +484,7 @@ class RefactoringChecker(checkers.BaseTokenChecker): "The value can be accessed directly instead.", ), "R1737": ( - "Consider directly using 'yield from %s' instead", + "Consider directly using 'yield from' instead", "use-yield-from", "Emitted when yielding from a loop can be replaced by yield from.", ), @@ -1134,37 +1134,10 @@ def visit_yield(self, node: nodes.Yield) -> None: return parent = node.parent.parent - if not isinstance(parent, nodes.For): + if not isinstance(parent, nodes.For) or len(parent.body) != 1: return - if len(parent.body) != 1: - return - - if parent.target.name == node.value.name: - generator_name = "" - if isinstance(parent.iter, nodes.Name): - generator_name = parent.iter.name - elif isinstance(parent.iter, nodes.Call): - if isinstance(parent.iter.func, nodes.Attribute): - generator_name = self._get_full_name_from_attribute( - parent.iter.func - ) - else: - generator_name = f"{parent.iter.func.name}()" - - self.add_message( - "use-yield-from", node.lineno, node, generator_name, confidence=HIGH - ) - - @staticmethod - def _get_full_name_from_attribute(node: nodes.Attribute) -> str: - name = f"{node.attrname}()" - current_node = node.expr - while isinstance(current_node, nodes.Attribute): - name = f"{current_node.attrname}.{name}" - current_node = current_node.expr - - return f"{current_node.name}.{name}" + self.add_message("use-yield-from", node.lineno, node, confidence=HIGH) @staticmethod def _has_exit_in_scope(scope: nodes.LocalsDictNodeNG) -> bool: diff --git a/tests/functional/u/use/use_yield_from.py b/tests/functional/u/use/use_yield_from.py index dcaf72bfa5..ab84ad24d3 100644 --- a/tests/functional/u/use/use_yield_from.py +++ b/tests/functional/u/use/use_yield_from.py @@ -32,3 +32,8 @@ def yield_attr(): def yield_attr_nested(): for item in factory.kiwi.gen(): yield item # [use-yield-from] + + +def yield_expr(): + for item in [1, 2, 3]: + yield item # [use-yield-from] diff --git a/tests/functional/u/use/use_yield_from.txt b/tests/functional/u/use/use_yield_from.txt index 2e08eceec1..59a4be1477 100644 --- a/tests/functional/u/use/use_yield_from.txt +++ b/tests/functional/u/use/use_yield_from.txt @@ -1,4 +1,5 @@ -use-yield-from:9:8:9:18:bad:Consider directly using 'yield from generator' instead:HIGH -use-yield-from:14:8:14:18:out_of_names:Consider directly using 'yield from turbogen()' instead:HIGH -use-yield-from:29:8:29:18:yield_attr:Consider directly using 'yield from factory.gen()' instead:HIGH -use-yield-from:34:8:34:18:yield_attr_nested:Consider directly using 'yield from factory.kiwi.gen()' instead:HIGH +use-yield-from:9:8:9:18:bad:Consider directly using 'yield from' instead:HIGH +use-yield-from:14:8:14:18:out_of_names:Consider directly using 'yield from' instead:HIGH +use-yield-from:29:8:29:18:yield_attr:Consider directly using 'yield from' instead:HIGH +use-yield-from:34:8:34:18:yield_attr_nested:Consider directly using 'yield from' instead:HIGH +use-yield-from:39:8:39:18:yield_expr:Consider directly using 'yield from' instead:HIGH From 5f7ba3b2a92f5de78381f3a4f75e1067a9f2c5f5 Mon Sep 17 00:00:00 2001 From: CrazyBolillo Date: Fri, 9 Feb 2024 20:41:11 -0600 Subject: [PATCH 06/15] Add documentation The corresponding good/bad examples have been added and the message description touched up a bit. --- doc/data/messages/u/use-yield-from/bad.py | 3 +++ doc/data/messages/u/use-yield-from/good.py | 2 ++ doc/user_guide/checkers/features.rst | 3 +++ doc/user_guide/messages/messages_overview.rst | 1 + pylint/checkers/refactoring/refactoring_checker.py | 2 +- 5 files changed, 10 insertions(+), 1 deletion(-) create mode 100644 doc/data/messages/u/use-yield-from/bad.py create mode 100644 doc/data/messages/u/use-yield-from/good.py diff --git a/doc/data/messages/u/use-yield-from/bad.py b/doc/data/messages/u/use-yield-from/bad.py new file mode 100644 index 0000000000..2d609546a3 --- /dev/null +++ b/doc/data/messages/u/use-yield-from/bad.py @@ -0,0 +1,3 @@ +def bad_yield_from(generator): + for item in generator: + yield item diff --git a/doc/data/messages/u/use-yield-from/good.py b/doc/data/messages/u/use-yield-from/good.py new file mode 100644 index 0000000000..1331a6fd29 --- /dev/null +++ b/doc/data/messages/u/use-yield-from/good.py @@ -0,0 +1,2 @@ +def good_yield_from(generator): + yield from generator diff --git a/doc/user_guide/checkers/features.rst b/doc/user_guide/checkers/features.rst index 84b967a82c..5ac58f1315 100644 --- a/doc/user_guide/checkers/features.rst +++ b/doc/user_guide/checkers/features.rst @@ -797,6 +797,9 @@ Refactoring checker Messages Emitted when a boolean condition can be simplified to a constant value. :simplify-boolean-expression (R1709): *Boolean expression may be simplified to %s* Emitted when redundant pre-python 2.5 ternary syntax is used. +:use-yield-from (R1737): *Consider directly using 'yield from' instead* + Emitted when yielding from a loop can be replaced by yield from the iterator + directly. :consider-using-in (R1714): *Consider merging these comparisons with 'in' by using '%s %sin (%s)'. Use a set instead if elements are hashable.* To check if a variable is equal to one of many values, combine the values into a set or tuple and check if the variable is contained "in" it instead of diff --git a/doc/user_guide/messages/messages_overview.rst b/doc/user_guide/messages/messages_overview.rst index 8d9f197502..e928ac1a24 100644 --- a/doc/user_guide/messages/messages_overview.rst +++ b/doc/user_guide/messages/messages_overview.rst @@ -544,6 +544,7 @@ All messages in the refactor category: refactor/use-dict-literal refactor/use-list-literal refactor/use-set-for-membership + refactor/use-yield-from refactor/useless-object-inheritance refactor/useless-option-value refactor/useless-return diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index 698d6d3c81..9440a3ec53 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -486,7 +486,7 @@ class RefactoringChecker(checkers.BaseTokenChecker): "R1737": ( "Consider directly using 'yield from' instead", "use-yield-from", - "Emitted when yielding from a loop can be replaced by yield from.", + "Emitted when yielding from a loop can be replaced by yielding from the iterator directly.", ), } options = ( From 6301df2a2ebed89ac12bbde1915582153af1b890 Mon Sep 17 00:00:00 2001 From: CrazyBolillo Date: Fri, 9 Feb 2024 20:49:03 -0600 Subject: [PATCH 07/15] Add message to bad.py --- doc/data/messages/u/use-yield-from/bad.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/data/messages/u/use-yield-from/bad.py b/doc/data/messages/u/use-yield-from/bad.py index 2d609546a3..a322b0fa64 100644 --- a/doc/data/messages/u/use-yield-from/bad.py +++ b/doc/data/messages/u/use-yield-from/bad.py @@ -1,3 +1,3 @@ def bad_yield_from(generator): for item in generator: - yield item + yield item # [use-yield-from] From 7197d97b6f6372093e7a70cbd221942ce57729bd Mon Sep 17 00:00:00 2001 From: CrazyBolillo Date: Fri, 9 Feb 2024 22:05:22 -0600 Subject: [PATCH 08/15] Update doc files --- doc/user_guide/checkers/features.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/user_guide/checkers/features.rst b/doc/user_guide/checkers/features.rst index 5ac58f1315..61982121c5 100644 --- a/doc/user_guide/checkers/features.rst +++ b/doc/user_guide/checkers/features.rst @@ -798,8 +798,8 @@ Refactoring checker Messages :simplify-boolean-expression (R1709): *Boolean expression may be simplified to %s* Emitted when redundant pre-python 2.5 ternary syntax is used. :use-yield-from (R1737): *Consider directly using 'yield from' instead* - Emitted when yielding from a loop can be replaced by yield from the iterator - directly. + Emitted when yielding from a loop can be replaced by yielding from the + iterator directly. :consider-using-in (R1714): *Consider merging these comparisons with 'in' by using '%s %sin (%s)'. Use a set instead if elements are hashable.* To check if a variable is equal to one of many values, combine the values into a set or tuple and check if the variable is contained "in" it instead of From a389d3a0265f13e574e9c478a7e448b64200573c Mon Sep 17 00:00:00 2001 From: CrazyBolillo Date: Sat, 10 Feb 2024 10:26:56 -0600 Subject: [PATCH 09/15] Fix FP, add more test cases Got to carried away deleting code and deleted the most important comparison to see if what is yielded are the items iterated on. Also added more cases to the test file. --- pylint/checkers/refactoring/refactoring_checker.py | 3 +++ tests/functional/u/use/use_yield_from.py | 12 ++++++++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index 9440a3ec53..500870bb93 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -1137,6 +1137,9 @@ def visit_yield(self, node: nodes.Yield) -> None: if not isinstance(parent, nodes.For) or len(parent.body) != 1: return + if parent.target.name != node.value.name: + return + self.add_message("use-yield-from", node.lineno, node, confidence=HIGH) @staticmethod diff --git a/tests/functional/u/use/use_yield_from.py b/tests/functional/u/use/use_yield_from.py index ab84ad24d3..3b506d535e 100644 --- a/tests/functional/u/use/use_yield_from.py +++ b/tests/functional/u/use/use_yield_from.py @@ -1,8 +1,8 @@ -# pylint: disable=missing-docstring, import-error - +# pylint: disable=missing-docstring, import-error, yield-outside-function import factory from magic import shazam, turbogen +yield 1 def bad(generator): for item in generator: @@ -37,3 +37,11 @@ def yield_attr_nested(): def yield_expr(): for item in [1, 2, 3]: yield item # [use-yield-from] + + +def for_else_yield(gen, something): + for item in gen(): + if shazam(item): + break + else: + yield something From 85e9bc789d0133e0dd0016db4a6e91a751de102b Mon Sep 17 00:00:00 2001 From: CrazyBolillo Date: Sat, 10 Feb 2024 11:18:18 -0600 Subject: [PATCH 10/15] Do not emit use-yield-from on async func/loop It is invalid to use 'yield from' in async functions. Replacing yielding from an 'async for' loop may not be the same as using 'yield from' so the message won't be emitted for async loops as well. --- pylint/checkers/refactoring/refactoring_checker.py | 9 ++++++++- tests/functional/u/use/use_yield_from.py | 12 ++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index 500870bb93..57ca09dfb0 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -1134,12 +1134,19 @@ def visit_yield(self, node: nodes.Yield) -> None: return parent = node.parent.parent - if not isinstance(parent, nodes.For) or len(parent.body) != 1: + if ( + not isinstance(parent, nodes.For) + or isinstance(parent, nodes.AsyncFor) + or len(parent.body) != 1 + ): return if parent.target.name != node.value.name: return + if isinstance(node.frame(), nodes.AsyncFunctionDef): + return + self.add_message("use-yield-from", node.lineno, node, confidence=HIGH) @staticmethod diff --git a/tests/functional/u/use/use_yield_from.py b/tests/functional/u/use/use_yield_from.py index 3b506d535e..4717775d57 100644 --- a/tests/functional/u/use/use_yield_from.py +++ b/tests/functional/u/use/use_yield_from.py @@ -45,3 +45,15 @@ def for_else_yield(gen, something): break else: yield something + + +# yield from is not supported in async functions, so the following are fine + +async def async_for_yield(agen): + async for item in agen: + yield item + + +async def async_yield(agen): + for item in agen: + yield item From 4b73b453a6412525610dfc0ba524093ccb34ac03 Mon Sep 17 00:00:00 2001 From: CrazyBolillo Date: Sat, 10 Feb 2024 15:23:49 -0600 Subject: [PATCH 11/15] Update messages, node in message is now the loop --- doc/data/messages/u/use-yield-from/bad.py | 4 ++-- doc/user_guide/checkers/features.rst | 6 +++--- .../refactoring/refactoring_checker.py | 7 ++++--- tests/functional/u/use/use_yield_from.py | 20 +++++++++---------- tests/functional/u/use/use_yield_from.txt | 10 +++++----- 5 files changed, 24 insertions(+), 23 deletions(-) diff --git a/doc/data/messages/u/use-yield-from/bad.py b/doc/data/messages/u/use-yield-from/bad.py index a322b0fa64..012bc42f46 100644 --- a/doc/data/messages/u/use-yield-from/bad.py +++ b/doc/data/messages/u/use-yield-from/bad.py @@ -1,3 +1,3 @@ def bad_yield_from(generator): - for item in generator: - yield item # [use-yield-from] + for item in generator: # [use-yield-from] + yield item diff --git a/doc/user_guide/checkers/features.rst b/doc/user_guide/checkers/features.rst index 61982121c5..96c980039f 100644 --- a/doc/user_guide/checkers/features.rst +++ b/doc/user_guide/checkers/features.rst @@ -797,9 +797,6 @@ Refactoring checker Messages Emitted when a boolean condition can be simplified to a constant value. :simplify-boolean-expression (R1709): *Boolean expression may be simplified to %s* Emitted when redundant pre-python 2.5 ternary syntax is used. -:use-yield-from (R1737): *Consider directly using 'yield from' instead* - Emitted when yielding from a loop can be replaced by yielding from the - iterator directly. :consider-using-in (R1714): *Consider merging these comparisons with 'in' by using '%s %sin (%s)'. Use a set instead if elements are hashable.* To check if a variable is equal to one of many values, combine the values into a set or tuple and check if the variable is contained "in" it instead of @@ -909,6 +906,9 @@ Refactoring checker Messages :unnecessary-comprehension (R1721): *Unnecessary use of a comprehension, use %s instead.* Instead of using an identity comprehension, consider using the list, dict or set constructor. It is faster and simpler. +:use-yield-from (R1737): *Use 'yield from' directly instead of yielding each element one by one* + Yielding directly from the iterator is faster and arguably cleaner code than + yielding each element in the loop. :use-a-generator (R1729): *Use a generator instead '%s(%s)'* Comprehension inside of 'any', 'all', 'max', 'min' or 'sum' is unnecessary. A generator would be sufficient and faster. diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index 57ca09dfb0..c69dea8e9a 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -484,9 +484,10 @@ class RefactoringChecker(checkers.BaseTokenChecker): "The value can be accessed directly instead.", ), "R1737": ( - "Consider directly using 'yield from' instead", + "Use 'yield from' directly instead of yielding each element one by one", "use-yield-from", - "Emitted when yielding from a loop can be replaced by yielding from the iterator directly.", + "Yielding directly from the iterator is faster and arguably cleaner code than yielding each element " + "in the loop.", ), } options = ( @@ -1147,7 +1148,7 @@ def visit_yield(self, node: nodes.Yield) -> None: if isinstance(node.frame(), nodes.AsyncFunctionDef): return - self.add_message("use-yield-from", node.lineno, node, confidence=HIGH) + self.add_message("use-yield-from", node=parent, confidence=HIGH) @staticmethod def _has_exit_in_scope(scope: nodes.LocalsDictNodeNG) -> bool: diff --git a/tests/functional/u/use/use_yield_from.py b/tests/functional/u/use/use_yield_from.py index 4717775d57..2ccbb6d77e 100644 --- a/tests/functional/u/use/use_yield_from.py +++ b/tests/functional/u/use/use_yield_from.py @@ -5,13 +5,13 @@ yield 1 def bad(generator): - for item in generator: - yield item # [use-yield-from] + for item in generator: # [use-yield-from] + yield item def out_of_names(): - for item in turbogen(): - yield item # [use-yield-from] + for item in turbogen(): # [use-yield-from] + yield item def good(generator): @@ -25,18 +25,18 @@ def yield_something(): def yield_attr(): - for item in factory.gen(): - yield item # [use-yield-from] + for item in factory.gen(): # [use-yield-from] + yield item def yield_attr_nested(): - for item in factory.kiwi.gen(): - yield item # [use-yield-from] + for item in factory.kiwi.gen(): # [use-yield-from] + yield item def yield_expr(): - for item in [1, 2, 3]: - yield item # [use-yield-from] + for item in [1, 2, 3]: # [use-yield-from] + yield item def for_else_yield(gen, something): diff --git a/tests/functional/u/use/use_yield_from.txt b/tests/functional/u/use/use_yield_from.txt index 59a4be1477..fd77d31eb3 100644 --- a/tests/functional/u/use/use_yield_from.txt +++ b/tests/functional/u/use/use_yield_from.txt @@ -1,5 +1,5 @@ -use-yield-from:9:8:9:18:bad:Consider directly using 'yield from' instead:HIGH -use-yield-from:14:8:14:18:out_of_names:Consider directly using 'yield from' instead:HIGH -use-yield-from:29:8:29:18:yield_attr:Consider directly using 'yield from' instead:HIGH -use-yield-from:34:8:34:18:yield_attr_nested:Consider directly using 'yield from' instead:HIGH -use-yield-from:39:8:39:18:yield_expr:Consider directly using 'yield from' instead:HIGH +use-yield-from:8:4:9:18:bad:Use 'yield from' directly instead of yielding each element one by one:HIGH +use-yield-from:13:4:14:18:out_of_names:Use 'yield from' directly instead of yielding each element one by one:HIGH +use-yield-from:28:4:29:18:yield_attr:Use 'yield from' directly instead of yielding each element one by one:HIGH +use-yield-from:33:4:34:18:yield_attr_nested:Use 'yield from' directly instead of yielding each element one by one:HIGH +use-yield-from:38:4:39:18:yield_expr:Use 'yield from' directly instead of yielding each element one by one:HIGH From b9ce18e98b1ed8a5a3343f314bb0f75edec3b154 Mon Sep 17 00:00:00 2001 From: CrazyBolillo Date: Sat, 10 Feb 2024 15:34:01 -0600 Subject: [PATCH 12/15] Minor touchup to message description --- doc/user_guide/checkers/features.rst | 2 +- pylint/checkers/refactoring/refactoring_checker.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/user_guide/checkers/features.rst b/doc/user_guide/checkers/features.rst index 96c980039f..7dce5c43b8 100644 --- a/doc/user_guide/checkers/features.rst +++ b/doc/user_guide/checkers/features.rst @@ -908,7 +908,7 @@ Refactoring checker Messages set constructor. It is faster and simpler. :use-yield-from (R1737): *Use 'yield from' directly instead of yielding each element one by one* Yielding directly from the iterator is faster and arguably cleaner code than - yielding each element in the loop. + yielding each element one by one in the loop. :use-a-generator (R1729): *Use a generator instead '%s(%s)'* Comprehension inside of 'any', 'all', 'max', 'min' or 'sum' is unnecessary. A generator would be sufficient and faster. diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index c69dea8e9a..14b8adbc27 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -487,7 +487,7 @@ class RefactoringChecker(checkers.BaseTokenChecker): "Use 'yield from' directly instead of yielding each element one by one", "use-yield-from", "Yielding directly from the iterator is faster and arguably cleaner code than yielding each element " - "in the loop.", + "one by one in the loop.", ), } options = ( From 886afb40c618dc245a639619a8ddda7eaa255cee Mon Sep 17 00:00:00 2001 From: CrazyBolillo Date: Sat, 10 Feb 2024 23:05:08 -0600 Subject: [PATCH 13/15] Explain "why" in details.rst --- doc/data/messages/u/use-yield-from/details.rst | 14 ++++++++++++++ doc/data/messages/u/use-yield-from/related.rst | 1 + 2 files changed, 15 insertions(+) create mode 100644 doc/data/messages/u/use-yield-from/details.rst create mode 100644 doc/data/messages/u/use-yield-from/related.rst diff --git a/doc/data/messages/u/use-yield-from/details.rst b/doc/data/messages/u/use-yield-from/details.rst new file mode 100644 index 0000000000..824f6f5a0b --- /dev/null +++ b/doc/data/messages/u/use-yield-from/details.rst @@ -0,0 +1,14 @@ +:code:`yield from` can be thought of as removing the intermediary (your for loop) between the function caller and the +requested generator. This enables the caller to directly communicate with the generator (e.g. :code:`send()`). +This communication is not possible when manually yielding each element one by one in a loop. + +PEP 380 describes the possibility of adding optimizations specific to :code:`yield from`, as of the time of writing, +it can't be confirmed that such optimizations have been implemented. The following snippet still shows that while +probably negligible, a performance improvement can exist simply because of the removed overhead. + +.. code-block:: sh + + $ python3 -m timeit "def yield_from(): yield from range(100)" "for _ in yield_from(): pass" + 100000 loops, best of 5: 2.44 usec per loop + $ python3 -m timeit "def yield_loop():" " for item in range(100): yield item" "for _ in yield_loop(): pass" + 100000 loops, best of 5: 2.49 usec per loop diff --git a/doc/data/messages/u/use-yield-from/related.rst b/doc/data/messages/u/use-yield-from/related.rst new file mode 100644 index 0000000000..826aca35f2 --- /dev/null +++ b/doc/data/messages/u/use-yield-from/related.rst @@ -0,0 +1 @@ +- `PEP 380 `_ From 2a99dac52b27a85d7c3f5ae751fb5fdb765146cf Mon Sep 17 00:00:00 2001 From: CrazyBolillo Date: Sat, 10 Feb 2024 23:23:06 -0600 Subject: [PATCH 14/15] Improve details.rst redaction (i think) --- doc/data/messages/u/use-yield-from/details.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/doc/data/messages/u/use-yield-from/details.rst b/doc/data/messages/u/use-yield-from/details.rst index 824f6f5a0b..b1a309a16f 100644 --- a/doc/data/messages/u/use-yield-from/details.rst +++ b/doc/data/messages/u/use-yield-from/details.rst @@ -1,10 +1,10 @@ :code:`yield from` can be thought of as removing the intermediary (your for loop) between the function caller and the -requested generator. This enables the caller to directly communicate with the generator (e.g. :code:`send()`). +requested generator. This enables the caller to directly communicate with the generator (e.g. using :code:`send()`). This communication is not possible when manually yielding each element one by one in a loop. -PEP 380 describes the possibility of adding optimizations specific to :code:`yield from`, as of the time of writing, -it can't be confirmed that such optimizations have been implemented. The following snippet still shows that while -probably negligible, a performance improvement can exist simply because of the removed overhead. +PEP 380 describes the possibility of adding optimizations specific to :code:`yield from`. It looks like said +implementations have not been implemented as of the time of writing. Even without said optimizations, the following +snippets shows that :code:`yield from` is marginally faster. .. code-block:: sh From 2a06726fe88bcdf2adc793e630e988ad538516bf Mon Sep 17 00:00:00 2001 From: CrazyBolillo Date: Sat, 10 Feb 2024 23:29:03 -0600 Subject: [PATCH 15/15] Last nits to details.rst --- doc/data/messages/u/use-yield-from/details.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/data/messages/u/use-yield-from/details.rst b/doc/data/messages/u/use-yield-from/details.rst index b1a309a16f..037248a872 100644 --- a/doc/data/messages/u/use-yield-from/details.rst +++ b/doc/data/messages/u/use-yield-from/details.rst @@ -2,9 +2,9 @@ requested generator. This enables the caller to directly communicate with the generator (e.g. using :code:`send()`). This communication is not possible when manually yielding each element one by one in a loop. -PEP 380 describes the possibility of adding optimizations specific to :code:`yield from`. It looks like said -implementations have not been implemented as of the time of writing. Even without said optimizations, the following -snippets shows that :code:`yield from` is marginally faster. +PEP 380 describes the possibility of adding optimizations specific to :code:`yield from`. It looks like they +have not been implemented as of the time of writing. Even without said optimizations, the following snippet shows +that :code:`yield from` is marginally faster. .. code-block:: sh