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

B023 is emitted when closure is defined inside a loop and uses a variable from the same loop #7847

Open
Guilherme-Vasconcelos opened this issue Oct 7, 2023 · 5 comments
Labels
bug Something isn't working

Comments

@Guilherme-Vasconcelos
Copy link
Contributor

Code snippet that reproduces the bug

The following snippet emits B023 (function-uses-loop-variable):

def make_squared_list(size: int) -> list[int]:
    result = []

    for i in range(size):
        square = i**2

        def _append_to_result() -> None:
            # Function definition does not bind loop variable `square` Ruff[B023](https://docs.astral.sh/ruff/rules/function-uses-loop- 
            # variable)
            # (variable) square: int
            result.append(square)

        _append_to_result()

    return result

I have found out that adding a nonlocal <variable> declaration followed by an assignment to itself makes the error go away, i.e. change the closure _append_to_result to this:

def _append_to_result() -> None:
    # No longer emits any warnings.
    nonlocal square
    square = square
    result.append(square)

Command invoked

$ poetry run ruff ./ruff_bug.py

Current ruff settings

Relevant sections from pyproject.toml:

[tool.ruff]
select = [
    "F",
    "E",
    "W",
    "I",
    "N",
    "UP",
    "S",
    "BLE",
    "B",
    "A",
    "C4",
    "SIM",
    "PERF",
    "RUF"
]
line-length = 88
target-version = "py311"
ignore = [
    "S101",
    "B011",
]

[tool.ruff.pycodestyle]
ignore-overlong-task-comments = true

Current ruff version

$ poetry run ruff --version
ruff 0.0.291
@zanieb
Copy link
Member

zanieb commented Oct 7, 2023

Thanks for the well written issue :) this does look like a bug, even if I move return the function and run it in a new scope the variable is indeed bound

def make_squared_list(size: int) -> list[int]:
    result = []

    for i in range(size):
        square = i**2

        def _append_to_result() -> None:
            # Function definition does not bind loop variable `square` Ruff[B023](https://docs.astral.sh/ruff/rules/function-uses-loop- 
            # variable)
            # (variable) square: int
            print(square)
            result.append(square)

        _append_to_result()

    return _append_to_result

make_squared_list(3)()
0
1
4
4

@zanieb zanieb added the bug Something isn't working label Oct 7, 2023
@charliermarsh
Copy link
Member

charliermarsh commented Oct 8, 2023

(Confirmed that flake8-bugbear also flags these cases. So it's not a bug in our re-implementation. We should still fix if we can.)

@charliermarsh
Copy link
Member

Hmm, I looked at some bugbear issues, and it does seem like this is valid to flag in some cases.

For example, this prints [1, 1] instead of [0, 1]. Note that the function defined in the loop is called after the loop, rather than within each iteration:

def make_squared_list(size: int) -> list[int]:
    result = []

    functions = []
    for i in range(size):
        square = i**2

        def _append_to_result() -> None:
            # Function definition does not bind loop variable `square` Ruff[B023](https://docs.astral.sh/ruff/rules/function-uses-loop- 
            # variable)
            # (variable) square: int
            result.append(square)

        functions.append(_append_to_result)

    for function in functions:
        function()

    return result


print(make_squared_list(2))

See PyCQA/flake8-bugbear#402.

@beauxq
Copy link

beauxq commented Aug 29, 2024

I think this false positive could be eliminated without risking any false negatives.

We should be able to detect whether the only thing that is done with the function is calling it within the same loop.
If that is the case, it shouldn't be reported.
If anything else is done with the function besides calling it in the same loop, the error should be reported.


    for i in range(size):
        square = i**2

        def _append_to_result() -> None:
            result.append(square)

        _append_to_result()
    return result

Nothing is done besides calling it in the same loop, so it should not be reported.


    for i in range(size):
        square = i**2

        def _append_to_result() -> None:
            result.append(square)

        _append_to_result()
    return _append_to_result

Something is done besides calling it in the same loop (returning it), so it should be reported.


    for i in range(size):
        square = i**2

        def _append_to_result() -> None:
            result.append(square)

        functions.append(_append_to_result)

Something is done besides calling it in the same loop (passing it to another function), so it should be reported.


In case someone wonders why someone would want to define a function only to call it within the same loop, the biggest reason I do it is code organization.

  • It organizes a section of code together and gives it a name.
  • It creates a new scope so temp variables don't pollute the name space.
  • It is collapsible in a code editor.

In one place I ran into this false positive, there were 5 variables captured by the function.
Having to bind all of them would make a bunch of noise that would make it harder to read.
noqa comments on all the usages would also make it harder to read. And I don't want to risk a false negative somewhere else by ignoring the whole file. Ignoring the rule also brings the risk that someone else could come later and change it to use the function outside the loop, and the problem wouldn't be caught.

@pankajp
Copy link

pankajp commented Sep 10, 2024

I think @beauxq is right and ruff should be able to detect the first case where the only thing being done with the function is calling it within the same loop.
I just had to disable B023 checks in my config as we have numerous such uses

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants