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.

Sign up for GitHub

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

Jump to bottom

RUF052: Don't flag assignments to private function arguments #14968

Open
aiudirog opened this issue Dec 14, 2024 · 2 comments · May be fixed by #15376
Open

RUF052: Don't flag assignments to private function arguments #14968

aiudirog opened this issue Dec 14, 2024 · 2 comments · May be fixed by #15376
Assignees
@dylwil3
Labels
accepted Ready for implementation help wanted Contributions especially welcome rule Implementing or modifying a lint rule

Comments

@aiudirog
Copy link

This is a follow up to #14796: attempting to assign a private function parameter still gets flagged in Ruff v0.8.3 (playground)

My primary use case for private parameters is when I need to track what I've seen when recursing through a potentially circular graph:

class Node:

    connected: list[Node]

    def recurse(self, *, _seen: set[Node] | None = None):
        if _seen is None:
            _seen = set()  # Local dummy variable `_seen` is accessed (RUF052)
        elif self in _seen:
            return
        _seen.add(self)
        for other in self.connected:
            other.recurse(_seen=_seen)

In most cases, I wouldn't want the caller to have to seed the _seen param or even know that it exists, as imo, that's just an implementation detail.
@dylwil3 dylwil3 added the rule Implementing or modifying a lint rule label Dec 14, 2024
@AlexWaygood AlexWaygood added help wanted Contributions especially welcome accepted Ready for implementation labels Dec 14, 2024
@dylwil3 dylwil3 self-assigned this Dec 14, 2024
@dylwil3
Copy link
Collaborator

dylwil3 commented Dec 14, 2024 •
edited
Loading

Incidentally, in this example the rule is not complaining about the binding for the function parameter, but rather about the rebinding in the if-statement. So I think we need to add some logic that skips if the offending binding is a re-binding of a private parameter declaration.

Unrelated - would it be more helpful if the diagnostic pointed to the problematic use rather than the binding?

@aiudirog
Copy link
Author

Incidentally, in this example the rule is not complaining about the binding for the function parameter, but rather about the rebinding in the if-statement. So I think we need to add some logic that skips if the offending binding is a re-binding of a private parameter declaration.

Yeah on 0.8.2 the rule flagged both the function definition and the reassignment in the body separately, so that seems like the appropriate solution.

Unrelated - would it be more helpful if the diagnostic pointed to the problematic use rather than the binding?

That might just flip it to error on _seen.add(self) and, if there's a lot of usages in the same function, could output a lot of messages

@alexfikl alexfikl mentioned this issue Dec 15, 2024
Merged
@dylwil3 dylwil3 linked a pull request Jan 9, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dylwil3 dylwil3

Labels
accepted Ready for implementation help wanted Contributions especially welcome rule Implementing or modifying a lint rule
Projects
None yet
Milestone
No milestone
3 participants
@aiudirog @dylwil3 @AlexWaygood