E731 auto-fix is too aggressive

[adampauls]

Consider the following code:

@dataclass
class Foo:
    my_lambda: Callable[[], int]


def func(arg: Foo, flag: bool):
    my_lambda: Callable[[], int]
    if flag:
        my_lambda = arg.my_lambda
    else:
        my_lambda = lambda: 5  # noqa: E731

    return my_lambda()

Ruff will autofix the noqa line, but that introduces a bug, at least I think it does. Certainly pyright complains.

EDIT:
After the autofix, the code is

@dataclass
class Foo:
    my_lambda: Callable[[], int]


def func(arg: Foo, flag: bool) -> int:
    my_lambda: Callable[[], int]
    if flag:
        my_lambda = arg.my_lambda
    else:
        def my_lambda():
            return 5

    return my_lambda()

[zanieb]

@adampauls can you please include the code after the autofix and the pyright complaint?

[adampauls]

Done

[zanieb]

@adampauls that fix looks fine to me and runs correctly

> func(Foo(my_lambda=lambda: 1), True)
1

> func(Foo(my_lambda=lambda: 1), False)
5

What's the bug / pyright complaint?

[adampauls]

Pyright says

error: Declaration "my_lambda" is obscured by a declaration of the same name

I guess you can argue that pyright is issuing an incorrect error here, but to my eyes the code becomes worse after the fix here. I had imagined that the ruff rule/fix was spiritually targeted towards the case where assigning a lambda is just uglier syntax for a def, whereas here, "assigning a lambda" really is a meaningful assignment to an existing symbol instead of declaration of a new one. If this behavior is intended, feel free to close and I'll just ignore the rule.

[charliermarsh]

This might be a case where there could be another suggestion for refactoring the code. Is it possible to share a more realistic snippet?

[adampauls]

Here's a link to the original code

[charliermarsh]

Thanks :) Hmm, perhaps we could avoid flagging E731 when the variable already exists as an annotation? E.g., based on the partial_parse_builder: Callable[[Datum], PartialParse].

[adampauls]

FWIW I don't even mind the lint error, I like opinionated linters. I just don't like opinionated autofixers, since you might not even realize that code was changed behind the scenes if you're running it over lots of code. If it were me, I would disable the autofix if the variable is assigned or declared anywhere other than the place where it is assigned to a lambda (before or after).

[charliermarsh]

I think that's reasonable. We could mark it as a manual fix in that case.

[adampauls]

Thanks!