New message `use-yield-from` #9419

crazybolillo · 2024-02-08T02:35:19Z

Type of Changes

	Type
✓	✨ New feature

Description

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.

crazybolillo · 2024-02-08T02:35:59Z

I may be missing some test cases and also wonder what the appropriate confidence level should be. I don't think it should be UNDEFINED. Any review is appreciated.

DanielNoord · 2024-02-08T07:45:48Z

I may be missing some test cases and also wonder what the appropriate confidence level should be. I don't think it should be UNDEFINED. Any review is appreciated.

confidence.HIGH would be appropriate here. As you can see you will need to update some of the existing test cases for this to work.

Pierre-Sassoulas

Looks great, thank you ! The two new messages in pylint make sense and are not false positives. I think we need to fix those two violations and fix the tests to be able to run the primer and see how it fares on more diverse python projects (we do not run the primer if other tests fails).

crazybolillo · 2024-02-09T01:26:35Z

So I think unit tests should be fixed. Also found some cases I had not considered and improved the checker in general. Still I think I need to do a little bit more work to handle cases like:

for item in [1, 2, 3]:
    yield item

crazybolillo · 2024-02-09T01:27:15Z

Also realized I need to add a docs section for the message. Will work on that as well.

codecov · 2024-02-09T01:32:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (659a32f) 95.80% compared to head (2a06726) 95.81%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #9419   +/-   ##
=======================================
  Coverage   95.80%   95.81%           
=======================================
  Files         173      173           
  Lines       18776    18788   +12     
=======================================
+ Hits        17989    18001   +12     
  Misses        787      787

Files	Coverage Δ
pylint/checkers/refactoring/refactoring_checker.py	`98.26% <100.00%> (+0.02%)`	⬆️

crazybolillo · 2024-02-10T05:25:55Z

I tried to add more logic to obtain a human-friendly name for other sort of iterators in emitted messages but I thought the simplicity of simply not showing it outweighed the extra-code, after all the documentation good/bad examples make it very clear and it is not a very complex message (IMO).

Therefore the message now just suggests use 'yield from'. This makes the actual code to implement the code short and straight to the point. Let me know if you agree on this or prefer to show the iterator's name in the emitted message.

Also, I ran my branch over pylint source code and it did not emit use-yield-from messages. Maybe I am missing something? Or what were the two errors that were mentioned?

DanielNoord

Very nice! One last nit.

DanielNoord · 2024-02-10T13:00:54Z

tests/functional/u/use/use_yield_from.py

@@ -0,0 +1,39 @@
+# pylint: disable=missing-docstring, import-error


Could you add

Suggested change

# pylint: disable=missing-docstring, import-error

yield 1

Although it is a syntax error I would like to check that pylint doesn't crash on it.

Pierre-Sassoulas

Looks great, the coverage and primers aren't working though. Could you try to rebase on upstream main ?

Pierre-Sassoulas · 2024-02-10T13:15:01Z

doc/user_guide/checkers/features.rst

  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*


We have a distinction between "use" and "consider using", we should not mix both. Is there something to consider or is using yield from always better ?

This check should only emit messages on very clear cases like this one:

for x in gen: yield x

And in those cases I think yield from is always better. I see this message as similar to use-dict-literal.

jacobtylerwalls · 2024-02-10T14:23:57Z

Looks great, the coverage and primers aren't working though.

It's because there hasn't been a commit on main in five days, and all of the primer caches were evicted due to space constraints. I just manually reran the primer / main job, so when it finishes, we can try these ones again.

Pierre-Sassoulas · 2024-02-10T14:27:20Z

It's because there hasn't been a commit on main in five days,

Damn, it's the first slow week since forever, we never encountered this issue before 😄 (it was a 'pytest' week for me). Thanks for the explanation Jacob.

jacobtylerwalls · 2024-02-10T14:50:23Z

Looks like a FP in black.

jacobtylerwalls · 2024-02-10T16:26:28Z

I'm not fluent with async for (see https://github.com/django/django/blob/3580b47ed31ec85ae89b13618f36bb463e97acc8/django/test/client.py#L132). Would that be a behavior change if changed to simply 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 pylint-dev#9229.

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.

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.

The corresponding good/bad examples have been added and the message description touched up a bit.

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.

crazybolillo · 2024-02-10T16:43:36Z

I'm not fluent with async for (see https://github.com/django/django/blob/3580b47ed31ec85ae89b13618f36bb463e97acc8/django/test/client.py#L132). Would that be a behavior change if changed to simply yield from?

Good catch, I would not consider myself fluent either but in this case yield from is not even supported inside async functions, so I will work on handling that case.

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.

crazybolillo · 2024-02-10T18:26:20Z

So I handled the async case and all primer messages seem valid to me.

Pierre-Sassoulas

Thank you for fixing the async case, the primer looks very promising now !

pylint/checkers/refactoring/refactoring_checker.py

Pierre-Sassoulas · 2024-02-10T19:44:32Z

pylint/checkers/refactoring/refactoring_checker.py

+        "R1737": (
+            "Consider directly using 'yield from' instead",
+            "use-yield-from",
+            "Emitted when yielding from a loop can be replaced by yielding from the iterator directly.",


Suggested change

"Emitted when yielding from a loop can be replaced by yielding from the iterator directly.",

"yielding from a loop is slower than yielding from the iterator directly.",

(? not sure if actually true all the time, but I suppose it's the case. At least it's going to be more elegant and understandable than the loop.)

I know almost all our messages talk about the "when" it's raised, when the "why" it's raised is a lot more interesting.

Yes, I had thought about stating possible perf improvements but was not sure whether to say "possibly faster" or just state it. I guess we can go with this because it certainly won't be any slower. Besides explaining it is faster I also mentioned the cleaner code part.

Sometime it's surprising, it would be better to add something in related.rst Found https://copyprogramming.com/howto/in-python-is-faster-using-yield-from-than-yield-in-a-loop it would be better if it's in depth and by someone like a python core dev but I didn't find this in a reasonable time.

pylint/checkers/refactoring/refactoring_checker.py

tests/functional/u/use/use_yield_from.txt

crazybolillo · 2024-02-10T21:36:04Z

Applied the requested changes.

Pierre-Sassoulas

Great, thank you. Let me know what you think of the article I found for related.rst, or if you found something more convincing. This is going to be a nice addition to 3.1.0 !

Changes done

crazybolillo · 2024-02-11T05:14:09Z

So I could not find anything too convincing regarding the performance of yield from. I added PEP 380 to the related links, it has a section on "optimizations". However I found this post on stack overflow that states the optimizations have not been implemented (at least of python 3.6):

https://stackoverflow.com/questions/43591542/does-cpython-implement-the-mentioned-optimizations-from-pep-380

I don't trust my CPython reading abilities so I can't confirm whether they have been implemented on newer versions. I did play around with timeit to compare the performance between the two approaches, I added the snippet to details.rst.

As we know measuring performance is not that easy but I think we can say that yield from is at least as fast if not a little bit faster on occasions.

github-actions · 2024-02-11T05:53:22Z

🤖 Effect of this PR on checked open source code: 🤖

Effect on astroid:
The following messages are now emitted:

use-yield-from:
Use 'yield from' directly instead of yielding each element one by one
https://github.com/pylint-dev/astroid/blob/35dba66b7739b8fc0b25e409ca07da9a62d9ee34/astroid/manager.py#L405

Effect on home-assistant:
The following messages are now emitted:

use-yield-from:
Use 'yield from' directly instead of yielding each element one by one
https://github.com/home-assistant/core/blob/3441b93c5c1c639dd879a67bc5f82ca125f2cd3a/homeassistant/components/stream/worker.py#L424

Effect on music21:
The following messages are now emitted:

use-yield-from:
Use 'yield from' directly instead of yielding each element one by one
https://github.com/cuthbertLab/music21/blob/0927e39231a8a748a78f0f6499dd246942fed0e4/music21/common/objects.py#L70
use-yield-from:
Use 'yield from' directly instead of yielding each element one by one
https://github.com/cuthbertLab/music21/blob/0927e39231a8a748a78f0f6499dd246942fed0e4/music21/test/treeYield.py#L69
use-yield-from:
Use 'yield from' directly instead of yielding each element one by one
https://github.com/cuthbertLab/music21/blob/0927e39231a8a748a78f0f6499dd246942fed0e4/music21/test/treeYield.py#L77
use-yield-from:
Use 'yield from' directly instead of yielding each element one by one
https://github.com/cuthbertLab/music21/blob/0927e39231a8a748a78f0f6499dd246942fed0e4/music21/test/treeYield.py#L98
use-yield-from:
Use 'yield from' directly instead of yielding each element one by one
https://github.com/cuthbertLab/music21/blob/0927e39231a8a748a78f0f6499dd246942fed0e4/music21/tree/core.py#L522
use-yield-from:
Use 'yield from' directly instead of yielding each element one by one
https://github.com/cuthbertLab/music21/blob/0927e39231a8a748a78f0f6499dd246942fed0e4/music21/tree/core.py#L526
use-yield-from:
Use 'yield from' directly instead of yielding each element one by one
https://github.com/cuthbertLab/music21/blob/0927e39231a8a748a78f0f6499dd246942fed0e4/music21/stream/iterator.py#L511

Effect on pytest:
The following messages are now emitted:

use-yield-from:
Use 'yield from' directly instead of yielding each element one by one
https://github.com/pytest-dev/pytest/blob/7690a0ddf15785f0af0e9a4cf8524379c53cd061/src/_pytest/_py/path.py#L163
use-yield-from:
Use 'yield from' directly instead of yielding each element one by one
https://github.com/pytest-dev/pytest/blob/7690a0ddf15785f0af0e9a4cf8524379c53cd061/src/_pytest/_py/path.py#L170

Effect on pandas:
The following messages are now emitted:

use-yield-from:
Use 'yield from' directly instead of yielding each element one by one
https://github.com/pandas-dev/pandas/blob/862d4318b4d0a1f052afd796901bcc4038761f2c/pandas/core/arrays/masked.py#L336

This comment was generated for commit 2a06726

DanielNoord

Thank you, a nice addition!

Pierre-Sassoulas

Great new checker, one of the main new features of 3.1.0 🎉

Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label Feb 8, 2024

Pierre-Sassoulas reviewed Feb 8, 2024

View reviewed changes

crazybolillo requested a review from Pierre-Sassoulas February 10, 2024 05:25

DanielNoord reviewed Feb 10, 2024

View reviewed changes

Pierre-Sassoulas requested changes Feb 10, 2024

View reviewed changes

jacobtylerwalls mentioned this pull request Feb 10, 2024

[primer] Allow primer comment job to recreate venv #9427

Merged

This comment has been minimized.

Sign in to view

jacobtylerwalls mentioned this pull request Feb 10, 2024

Avoid some inference in no-name-from-module #9428

Merged

crazybolillo added 9 commits February 10, 2024 10:27

New message use-yield-from

04dba4f

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 pylint-dev#9229.

Update confidence level for 'use-yield-from'

bf33b16

Disable use-yield-from on unrelated tests

4fd42d8

Add documentation

5f7ba3b

The corresponding good/bad examples have been added and the message description touched up a bit.

Add message to bad.py

6301df2

Update doc files

7197d97

Fix FP, add more test cases

a389d3a

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.

crazybolillo force-pushed the i9229-crazybolillo branch from 29eb146 to a389d3a Compare February 10, 2024 16:27

This comment has been minimized.

Sign in to view

Do not emit use-yield-from on async func/loop

85e9bc7

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.

This comment has been minimized.

Sign in to view

crazybolillo requested review from DanielNoord and Pierre-Sassoulas February 10, 2024 18:26

Pierre-Sassoulas previously requested changes Feb 10, 2024

View reviewed changes

crazybolillo added 2 commits February 10, 2024 15:23

Update messages, node in message is now the loop

4b73b45

Minor touchup to message description

b9ce18e

crazybolillo requested a review from Pierre-Sassoulas February 10, 2024 21:35

This comment has been minimized.

Sign in to view

Pierre-Sassoulas reviewed Feb 10, 2024

View reviewed changes

Pierre-Sassoulas added this to the 3.1.0 milestone Feb 10, 2024

Explain "why" in details.rst

886afb4

crazybolillo requested a review from Pierre-Sassoulas February 11, 2024 05:14

crazybolillo added 2 commits February 10, 2024 23:23

Improve details.rst redaction (i think)

2a99dac

Last nits to details.rst

2a06726

DanielNoord approved these changes Feb 11, 2024

View reviewed changes

Pierre-Sassoulas approved these changes Feb 11, 2024

View reviewed changes

Pierre-Sassoulas merged commit 18013eb into pylint-dev:main Feb 11, 2024

		@@ -0,0 +1,39 @@
		# pylint: disable=missing-docstring, import-error

	"Emitted when yielding from a loop can be replaced by yielding from the iterator directly.",
	"yielding from a loop is slower than yielding from the iterator directly.",

Uh oh!

New message use-yield-from #9419

New message use-yield-from #9419

Uh oh!

Conversation

crazybolillo commented Feb 8, 2024

Type of Changes

Description

Uh oh!

crazybolillo commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanielNoord commented Feb 8, 2024

Uh oh!

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

crazybolillo commented Feb 9, 2024

Uh oh!

crazybolillo commented Feb 9, 2024

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

crazybolillo commented Feb 10, 2024

Uh oh!

DanielNoord left a comment

Choose a reason for hiding this comment

Uh oh!

DanielNoord Feb 10, 2024

Choose a reason for hiding this comment

Uh oh!

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

Pierre-Sassoulas Feb 10, 2024

Choose a reason for hiding this comment

Uh oh!

crazybolillo Feb 10, 2024

Choose a reason for hiding this comment

Uh oh!

jacobtylerwalls commented Feb 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pierre-Sassoulas commented Feb 10, 2024

Uh oh!

This comment has been minimized.

jacobtylerwalls commented Feb 10, 2024

Uh oh!

jacobtylerwalls commented Feb 10, 2024

Uh oh!

crazybolillo commented Feb 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

crazybolillo commented Feb 10, 2024

Uh oh!

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Pierre-Sassoulas Feb 10, 2024

Choose a reason for hiding this comment

Uh oh!

crazybolillo Feb 10, 2024

Choose a reason for hiding this comment

Uh oh!

Pierre-Sassoulas Feb 10, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

crazybolillo commented Feb 10, 2024

Uh oh!

This comment has been minimized.

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

New message `use-yield-from` #9419

New message `use-yield-from` #9419

crazybolillo commented Feb 8, 2024 •

edited

Loading

codecov bot commented Feb 9, 2024 •

edited

Loading

jacobtylerwalls commented Feb 10, 2024 •

edited

Loading

crazybolillo commented Feb 10, 2024 •

edited

Loading