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

Trigger violation on mutable values and function calls for non-dataclass attribute defaults #4053

Closed
adampauls opened this issue Apr 20, 2023 · 6 comments
Labels
question Asking for support or clarification

Comments

@adampauls
Copy link
Contributor

This is a feature request. I see in this PR that there is some recent work in flagging mutable and function call defaults for dataclass attributes. Is there a plan to enable this check for non-dataclass attributes as well? Right now, no Python tooling I know of catches this bug:

import dataclasses

class NonDataClass:
    my_list: list[int] = dataclasses.field(default_factory=list)  # oops! type checks but is nonsense

    def add_item(self, item: int) -> None:
        self.my_list.append(item)

instance = NonDataClass()
instance.add_item(1)
assert instance.my_list == [1]

The problem is that dataclass.field lies about its type.

It would be great if we could check for mutable and function call defaults in class attributes because they are just problematic for non-dataclasses as they are for dataclasses. The only difference from the existing implementation would need to be that dataclasses.field should only be exempted when used inside a dataclass.

I can look into this if the work is not already planned since I think it's a simple change from what's already been done.

Thanks for the great tool!

@adampauls adampauls changed the title Trigger violation on mutable values and function calls for non-dataclass attribut defaults Trigger violation on mutable values and function calls for non-dataclass attribute defaults Apr 20, 2023
@charliermarsh
Copy link
Member

Interesting! I initially assumed that the suggestion here was to flag usages of dataclasses.field within classes that lack the @dataclass decorator, but I think you're suggesting something a bit broader: disallowing mutable defaults and function calls within non-@dataclass-decorated classes, which would by definition include usages of dataclasses.field. Is that right?

@charliermarsh charliermarsh added the question Asking for support or clarification label Apr 21, 2023
@hmc-cs-mdrissi
Copy link

I'll note some libraries do re-use/support dataclasses field for there own decorator/class. field just serves as a convenient way to hold some annotations and there are number of dataclass like libraries. I do use internal library that does,

from dataclasses import field

@configurable
class Foo:
  x = field(default_factory=list)

where default_factory list can be safe to use. It's own separate rule to disallow field in non dataclass sounds fine though as if you aren't using alternative dataclass like libraries it makes sense.

@adampauls
Copy link
Contributor Author

If it's easy, it would be great to allow a configurable list of decorators that allow data class.field, or maybe even any dataclass_transform should be exempted?

@adampauls
Copy link
Contributor Author

Is that right?

Yes, exactly.

@adampauls
Copy link
Contributor Author

Though I would settle for the more targeted warning on just dataclass.field if that's preferable!

charliermarsh added a commit that referenced this issue Jun 12, 2023
AFAIK, there is no reason to limit RUF008 to just dataclasses -- mutable
defaults have the same problems for regular classes.

Partially addresses #4053
and broken out from #4096.

---------

Co-authored-by: Micha Reiser <micha@reiser.io>
Co-authored-by: Charlie Marsh <charlie.r.marsh@gmail.com>
@charliermarsh
Copy link
Member

Closing in favor of #4390.

konstin pushed a commit that referenced this issue Jun 13, 2023
AFAIK, there is no reason to limit RUF008 to just dataclasses -- mutable
defaults have the same problems for regular classes.

Partially addresses #4053
and broken out from #4096.

---------

Co-authored-by: Micha Reiser <micha@reiser.io>
Co-authored-by: Charlie Marsh <charlie.r.marsh@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Asking for support or clarification
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants