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

A006 and A002 both trigger for lambda's #14135

Closed
jaap3 opened this issue Nov 6, 2024 · 2 comments · Fixed by #14144
Closed

A006 and A002 both trigger for lambda's #14135

jaap3 opened this issue Nov 6, 2024 · 2 comments · Fixed by #14144
Assignees
Labels
bug Something isn't working

Comments

@jaap3
Copy link
Contributor

jaap3 commented Nov 6, 2024

This example triggers two violations (with preview enabled), is this intentional?

lambda id: id + 1
$ ruff check --isolated --preview --select A test.py

test.py:1:8: A002 Argument `id` is shadowing a Python builtin
  |
1 | lambda id: id + 1
  |        ^^ A002
  |

test.py:1:8: A006 Lambda argument `id` is shadowing a Python builtin
  |
1 | lambda id: id + 1
  |        ^^ A006
  |
$ ruff --version
ruff 0.7.2
@dylwil3 dylwil3 added bug Something isn't working needs-decision Awaiting a decision from a maintainer labels Nov 6, 2024
@dylwil3
Copy link
Collaborator

dylwil3 commented Nov 6, 2024

Nice catch! It looks like flake8-builtins distinguishes between (named) functions and lambdas. We should match that behavior and maybe update the documentation on A002 and A006.

My only hesitation is that this would be a breaking change for a stable rule - so if people were running the checker with preview off and catching this shadowing on lambdas, they would no longer catch it if we matched flake8-builtins.

Not sure the best protocol for handling this - @MichaReiser / @AlexWaygood ?

@AlexWaygood
Copy link
Member

Great question. When we expand the scope of a stable rule, we generally put the new behaviour in preview mode for a couple of months before promoting it to stable. When we reduce the scope of a rule, it's more of a judgement call.

I can see the argument that this might be a breaking change for some people -- but it definitely feels like a bug to me to have two rules complain about the exact same error. So I'd be inclined to ship the behaviour change straight into stable for this one, so that users can benefit from the bugfix even if they haven't selected preview = true (most don't).

@dylwil3 dylwil3 removed the needs-decision Awaiting a decision from a maintainer label Nov 6, 2024
@dylwil3 dylwil3 changed the title A005 and A002 both trigger for lambda's A006 and A002 both trigger for lambda's Nov 7, 2024
@dylwil3 dylwil3 self-assigned this Nov 7, 2024
dylwil3 added a commit that referenced this issue Nov 7, 2024
…owing (A002)` (#14144)

Flake8-builtins provides two checks for arguments (really, parameters)
of a function shadowing builtins: A002 checks function definitions, and
A006 checks lambda expressions. This PR ensures that A002 is restricted
to functions rather than lambda expressions.

Closes #14135 .
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

Successfully merging a pull request may close this issue.

3 participants