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

builtins-ignorelist for attributes only #6524

Closed
benjamin-kirkbride opened this issue Aug 12, 2023 · 6 comments · Fixed by #9462
Closed

builtins-ignorelist for attributes only #6524

benjamin-kirkbride opened this issue Aug 12, 2023 · 6 comments · Fixed by #9462
Assignees
Labels
needs-decision Awaiting a decision from a maintainer

Comments

@benjamin-kirkbride
Copy link

Looking at https://beta.ruff.rs/docs/rules/builtin-attribute-shadowing/#builtin-attribute-shadowing-a003: I agree with the point made here: https://stackoverflow.com/a/60423602/1342874

Specifically, I think that the following is fine, and in fact better than alternatives:

@dataclass
class Row:
    id: int
    input: str

but of course the following is not fine:

id, input = row.id, row.input

My understanding is that the builtins-ignorelist is all or nothing; it makes it so that you can us id everywhere without warning, or nowhere without warning.

My proposal is that there is a builtins-attributes-ignorelist or something similar added for this.

@charliermarsh
Copy link
Member

Why not disable the rule entirely? It's intentionally categorized as a separate rule to enable that kind of granular on/off switch.

@charliermarsh charliermarsh added the needs-info More information is needed from the issue author label Aug 13, 2023
@benjamin-kirkbride
Copy link
Author

Fair question 😅

I think there are some attributes that I would still want to list, like str, or dict or something, but I don't have a good argument for why I think that some things are okay (like id, input, class, etc) but others aren't..

@tylerlaprade
Copy link
Contributor

I ran across this as well. I really don't want type to be used as a Django model attribute because if I accidentally use type instead of self.type, my code will be completely wrong, but I won't get any warnings. But id and filter seem generally harmless for my usecases.

@charliermarsh charliermarsh added needs-decision Awaiting a decision from a maintainer and removed needs-info More information is needed from the issue author labels Aug 14, 2023
@Olegt0rr
Copy link

Olegt0rr commented Sep 5, 2023

Just get A003 Class attribute exec is shadowing a python builtin on a method.

class Foo:
    def exec(self):
        ...

It's not a shadowing, cause I still can use builtin exec.

class Foo:
    @staticmethod
    def exec():
        exec("print('it is not a shadowing')")

Foo.exec()

For me, shadowing is when an object covers the original object and confusion can occur when called.

def foo(exec: str):
    print(exec)
    ...
    # and the later you may want to call builtin exec, but you can't
    exec("print('it is a shadowing')")

foo('bar')

Also readers may mistake the attribute for the builtin is a very strange explanation.
My user has an id. And user's class should use it:

class User:
    id: int

id_ - is ugly
user_id - is redundant and ugly, especially on call: user.user_id

What's wrong with reader, who can mess id() and user.id?

@charliermarsh
Copy link
Member

@Olegt0rr - That is working as intended but it's a strange rule. You can see an example of what it's intended to catch here:

class C:
    @staticmethod
    def list() -> None:
        pass

    @staticmethod
    def repeat(value: int, times: int) -> list[int]:
        return [value] * times

The issue is that it will be shadowed if you reference the builtin within the class scope.

@charliermarsh
Copy link
Member

PR up that should make this rule far more useful by only flagging actual shadowed reads (like in the example above) rather than the method definitions (which will almost always be fine): #9462

charliermarsh added a commit that referenced this issue Jan 11, 2024
…9462)

## Summary

This PR attempts to improve `builtin-attribute-shadowing` (`A003`), a
rule which has been repeatedly criticized, but _does_ have value (just
not in the current form).

Historically, this rule would flag cases like:

```python
class Class:
    id: int
```

This led to an increasing number of exceptions and special-cases to the
rule over time to try and improve it's specificity (e.g., ignore
`TypedDict`, ignore `@override`).

The crux of the issue is that given the above, referencing `id` will
never resolve to `Class.id`, so the shadowing is actually fine. There's
one exception, however:

```python
class Class:
    id: int

    def do_thing() -> id:
        pass
```

Here, `id` actually resolves to the `id` attribute on the class, not the
`id` builtin.

So this PR completely reworks the rule around this _much_ more targeted
case, which will almost always be a mistake: when you reference a class
member from within the class, and that member shadows a builtin.

Closes #6524.

Closes #7806.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-decision Awaiting a decision from a maintainer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants