Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make UP028 always fixable again #16496

Open
InSyncWithFoo opened this issue Mar 4, 2025 · 4 comments
Open

Make UP028 always fixable again #16496

InSyncWithFoo opened this issue Mar 4, 2025 · 4 comments
Labels
fixes Related to suggested fixes for violations wish Not on the current roadmap; maybe in the future

Comments

@InSyncWithFoo
Copy link
Contributor

InSyncWithFoo commented Mar 4, 2025

After #16451, UP028 is no longer always fixable. However, a different kind of fix can be offered in such cases:

for some_global, some_nonlocal in iterable:
    yield some_global, some_nonlocal
for _element in iterable:
    some_global, some_nonlocal = _element
    yield some_global, some_nonlocal

I can't think of any edge cases, other than that adding a new name to the scope might break runtime inspections:

nonlocal some_nonlocal

for some_nonlocal in iterable:
    yield some_nonlocal

print(locals())  # {'some_nonlocal': ...}
nonlocal some_nonlocal

for _element in iterable:
    some_nonlocal = _element
    yield some_nonlocal

print(locals())  # {'some_nonlocal': ..., '_element': ...}

Also consider the case where the new name is the same as an existing one:

_element = ...

for _element in iterable:
    some_global, some_nonlocal = _element
    yield some_global, some_nonlocal

print(_element)  # !!!

Generating a new, unique variable name is trivial, but it does, however, necessitate the fix being marked as unsafe to give the user a chance to review that name (along with everything else).

@MichaReiser
Copy link
Member

I consider adding such a fix low priority as this pattern should be rare. I'm okay adding it if it has low complexity.

I believe Ruff already has a functionality to introduce new names but that can fail (because of overlaps?)

@MichaReiser MichaReiser added wish Not on the current roadmap; maybe in the future fixes Related to suggested fixes for violations labels Mar 4, 2025
@InSyncWithFoo
Copy link
Contributor Author

@MichaReiser Generating a new name can never fail: Ruff just needs to check all names of the form var_{number} from 0 to infinity, stopping as soon as it finds one available (I think I did this once in a PR).

The examples above are all about global/nonlocal, but the same also applies to subscripts and attributes.

@dscorbett
Copy link

The point of UP028 is to recommend yield from where possible. The fix proposed here doesn’t use yield from so it isn’t an appropriate fix for UP028: it only fixes it in that it adds an extra statement to the loop so UP028 doesn’t apply. It could be an appropriate fix for a new style rule against assigning to globals via for targets, which is unrelated to yield.

@InSyncWithFoo
Copy link
Contributor Author

That's a much better idea. Consider this a rule request then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixes Related to suggested fixes for violations wish Not on the current roadmap; maybe in the future
Projects
None yet
Development

No branches or pull requests

3 participants