ruff

ruff copied to clipboard

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.

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?

For what it's worth, the Django project is against adding type hints, at least in the near future.

Yep, but I wonder why a RUF rule should really be forcing type hints at all.

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

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.

Thank you for all the feedback here :)

A couple misc. reactions, from reading through the thread:

• This rule is admittedly "pedantic". I wish we had a way to express "pedantic" rules, and we will in the future, but for now, I intentionally made this its own rule code, independent of the dataclass version, so that it could be selectively disabled for projects in which it doesn't apply. (I do consider that a valid workaround for the issues raised thus far.)
• We should omit Pydantic models from this rule, that seems straightforward.
• On whether we should suggest typing.ClassVar: the question for me is whether the critique here is about the rule, or the suggestion. In the Django case, should we not be flagging those attributes at all, or should we not be suggesting typing.ClassVar (and instead suggest something else, like using immutable data structures)?
• Related to the above: I'm not opposed to confining this check to classes with at least one typed attribute, but the thing is, it's still problematic (in the sense of the rule) to use a mutable default in those contexts -- the thing we're linting against is still present. (typing.ClassVar at least gives you clear semantics around how the field is meant to behave, and a way to enforce those semantics via a type checker, which is why it's presented as a possible solution.)

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

I also wanted to ask here, when you have a class variable as an "immutable" constant for that class, what should you do? Should you still annotate with this? Is there some other annotation here, as you probably don't want to have mutable class variables in most cases?

In the Django case, should we not be flagging those attributes at all, or should we not be suggesting typing.ClassVar (and instead suggest something else, like using immutable data structures)?

To me, it makes sense that a typing rule should be suggesting typing.ClassVar, while a bugbear-like rule should be suggesting inmutable data structures (even if Django uses mutable ones extensively). There is no way for ruff (or any other tool) to automatically decide what you meant, and so this would be two rules, or one rule with two possible fixes.

As my final 2c, the problem to me is not that the rule is pedantic per se, it is that it’s basically a typing rule, when many projects use typing as an opt-in thing (or not at all).

When and if ruff re-classifies its rules, it might become more clear. As it stands the RUF set of rules are basically whatever, so it makes sense that rules like this one end up there. Disabling is a perfectly OK workaround.

Thanks a lot for giving it a thought!

(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.

I've just had this fire on an Ansible plugin module which does not have any type annotations. For now I'll disable RUF012 and hope that #5275 resolves it.

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.

Yeah makes sense. Also happy to add other libs to the exemption if it’d be helpful.

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?

[...] In the Django case, should we not be flagging those attributes at all, or should we not be suggesting typing.ClassVar (and instead suggest something else, like using immutable data structures)? [...]

In the Django and its (database) model case it's not valid, as there's class descriptors doing a bit of magic there. As such the ClassVar is only valid for 1 case, when there's no instance of a Model. Though if you have a Model instance, the attribute contains the python value of what's stored in a database.

I found Django migrations are an area where ruff and mypy might not agree as far as this rule is concerned.

Sample 1

class Migration(migrations.Migration):
    dependencies = [...]
    operations = [...]

ruff isn't satisfied:

RUF012 Mutable class attributes should be annotated with typing.ClassVar

Sample 2

class Migration(migrations.Migration):
    dependencies: ClassVar[list[tuple[str, str]]] = [...]

    operations: ClassVar[list[Operation]] = [...]

ruff is satisfied, but not mypy:

error: Cannot override instance variable (previously declared on base class "Migration") with class variable [misc]

Sample 3

class Migration(migrations.Migration):
    dependencies: list[tuple[str, str]] = [...]

    operations: list[Operation] = [...]

mypy is fine with it, ruff insists:

RUF012 Mutable class attributes should be annotated with typing.ClassVar

How does Mypy behave if you use : Sequence[...] instead of : list[...]? That would signal to Ruff that the type is immutable.

@charliermarsh it behaves as desired. I hadn't considered Sequence before. Thanks for the tip!

@alexei - No prob! I should probably add this to the rule docs.

@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 -- We can definitely detect this for subclasses defined in the same file, and I'll make that change now. Unfortunately we can't yet support it for classes defined across files.

Every usage I have are in different file and even different python packages all together so it sounds like a combination of both detection and a config setting would be needed in my case.

It would be nice if we could somehow reuse an existing setting here. It's not quite identical to runtime-evaluated-base-classes though 🤔

I ran into this for a beanie.Document class where the MRO does include pydantic (my guess is ruff doesn't see the subclass):

>>> Document.__mro__
(<class 'beanie.odm.documents.Document'>, <class 'lazy_model.parser.new.LazyModel'>, <class 'pydantic.main.BaseModel'>, <class 'beanie.odm.interfaces.setters.SettersInterface'>, <class 'beanie.odm.interfaces.inheritance.InheritanceInterface'>, <class 'beanie.odm.interfaces.find.FindInterface'>, <class 'beanie.odm.interfaces.aggregate.AggregateInterface'>, <class 'beanie.odm.interfaces.getters.OtherGettersInterface'>, <class 'object'>)

So I think for now, i will need to add in a #noqa: RUF012 for those lines that have a problem at the moment, as I believe there's no way to configure to "teach" ruff about the Document class here.

(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.

I find a similiar observation for:

some_attribute = ClassVar[dict[str, str]]

class MyExample:

    subclass_field: some_attribute = {}

here is RUF012 appears, but I expect not to be, which might be more convenient if you inherit over several classes:

class MyExample2:

    subclass_field: some_attribute = {}
class MyExample3:

    subclass_field: some_attribute = {}

I find a similiar observation for:

some_attribute = ClassVar[dict[str, str]]

class MyExample:

    subclass_field: some_attribute = {}

I think this might be because it uses type alias.

Yes, but is it bad to use alias, respectively, own type definitions to avoid code redundancy?

• If so, using some_attribute = ClassVar[dict[str, str]] should cause already an alert?

No, it's not bad but just a limitation of Ruff which will be resolved with the ongoing red knot project.