ruff: RUF012 triggers many false positives (are they really? they are correct) in some projects

Hello 👋🏽

In this PR RUF012 was extended to apply to non-dataclass classes.

This has the (maybe desirable, maybe not, but was surprising to me) effect of forcing typing for a RUF rule, in places where you would not necessarily use typing (if you did not want to).

For instance, in Django Rest Framework (from their docs):

class AccountSerializer(serializers.ModelSerializer):
    class Meta:
        model = Account
        fields = ['id', 'account_name', 'users', 'created']  # RUF012

Or in Django itself:

class Customer(models.Model):
    first_name = models.CharField(max_length=100)
    last_name = models.CharField(max_length=100)

    class Meta:
        indexes = [
            models.Index(fields=["last_name", "first_name"]),  # RUF012
            models.Index(fields=["first_name"], name="first_name_idx"),  # RUF012
        ]

You could argue that’s good, but at least to me, it’s different from dataclasses, since in there you are required to use typing to even use them, so they are opt-in even before you use ruff. You could also argue both Django and Django Rest Framework should be recommending tuples instead.

Isn’t this too trigger happy for a RUF rule? Does it make sense to split it from the dataclass one?

EDIT: Actually, it is a separated code already 🤦🏽

In any case, feel free to close if this is just what it is! Ignoring the rule is easy for these kind of projects.

About this issue

Original URL
State: open
Created a year ago
Reactions: 6
Comments: 23 (10 by maintainers)

Commits related to this issue

Allow `typing.Final` for `mutable-class-default annotations` (`RUF012`) (#5274) ## Summary See: https://github.com/astral-sh/ruff/issues/5243. — committed to astral-sh/ruff by charliermarsh a year ago
discable RUF012 see https://github.com/astral-sh/ruff/issues/5243 — committed to xi/django-mfa3 by xi 10 months ago
disable RUF012 see https://github.com/astral-sh/ruff/issues/5243 — committed to xi/django-mfa3 by xi 10 months ago
Avoid `mutable-class-default` violations for Pydantic subclasses (#9187) Only applies to subclasses defined within the same file, as elsewhere. See: https://github.com/astral-sh/ruff/issues/5243#... — committed to astral-sh/ruff by charliermarsh 6 months ago

Most upvoted comments

(To be clear: I’m looking for more feedback here, including on the questions above. Thanks all!)

One of my use cases is the following:

from typing import ClassVar


class SomeAbstractBaseClass:

    some_attribute: ClassVar[dict[str, str]]


class SomeConcreteNewClass(SomeAbstractBaseClass):

    some_attribute = {"1": "2"}

This triggers RUF012 on the subclass, where I would have expected this to pass.

g-as on Jun 21, 2023

Adding to the false positives, this is also flagging pydantic models which function similar to dataclasses in how they are defined but actually create new instances of objects when instantiating the class.

from pydantic import BaseModel

class MyExample(BaseModel):

    foo: dict[str, str] = {}

In this example, foo is flagged with RUF012: Mutable class attributes should be annotated with typing.ClassVar but pydantic creates a new instance of the empty dict each time the class is instantiated. It’s not well documented if at all (https://github.com/pydantic/pydantic/discussions/3877) but is how it functions.

ITProKyle on Jun 21, 2023

We should omit Pydantic models from this rule, that seems straightforward.

I’d suggest a setting that lets you whitelist certain ancestor classes, with maybe default set to Pydantic.BaseModel or whatever. Since there are multiple libs where the “problem” syntax is part of a DSL.

I am not sure if this is possible yet, but it might be with the new import resolver.

smackesey on Jul 4, 2023

For what it’s worth, the Django project is against adding type hints, at least in the near future. You can read more in this PR and a technical board statement. This might be why it doesn’t recommend the ClassVar type annotation (and annotations in general).

For balance, however, the typed-django project also hasn’t gone all-in with ClassVar as suggested in this PR (though a quick search of the codebase shows ClassVar is still used in some places).

Is it perhaps worth resolving a balance of only flagging a violation when type annotations are already used to annotate a variable?

tjkuson on Jun 21, 2023

@charliermarsh, I appreciate the fix in #5274 to omit pydantic models however, it does not omit classes that subclass user defined models.

from pydantic import BaseModel


class MyBaseModel(BaseModel):
    
    def __bool__(self) -> bool:
        """pydantic.BaseModel does not implement __bool__."""
        return bool(self.model_dump(exclude_unset=True))


class MyExample(MyBaseModel):

    subclass_field: dict[str, str] = {}  # RUF012

If this is not something that ruff can determine on it’s own, an option like runtime-evaluated-base-classes for this case would be an acceptable solution.

ITProKyle on Dec 18, 2023

Related suggestion, have you considered a new category for “Ruff Warnings” (RUW?)? I think for things like RUF012 it could be nice to put them in a new category so that people could opt in to these kinds of lints separate from the other lints in RUF?

johnthagen on Jul 6, 2023

Thanks for this! I’m a little busy this morning but I’ll get back to it later today and share my perspective 😃

charliermarsh on Jun 21, 2023