[ruff] add fix safety section (RUF033)

[VascoSch92]

The PR add the fix safety section for rule RUF033 (#15584 ).

It seems that the fix was always unsafe, see #13192. An example of unsafely can be found in the PR description of #13192 or in the next section.

Unsafety example

I took as an example the case

from dataclasses import dataclass, InitVar

@dataclass
class User:
    name: str

    def __post_init__(self, greeting: str = "Hello"):
        print(f"{greeting}, {self.name}!")

@dataclass
class UserAfterFix:
    name: str
    greeting: InitVar[str] = "Hello"

    def __post_init__(self, greeting: str):
        print(f"{greeting}, {self.name}!")

User('Alice')  # print `Hello, Alice`
UserAfterFix('Alice') # raise an error

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[VascoSch92]

Hey @dylwil3

I believe the PR is ready for review.

[dylwil3]

I could not reproduce the exception being raised in the example in your PR summary (but maybe I've done something wrong?), nor could I see why the example fix in the original PR was unsafe. The example in the docs for the rule of a suggested change would be an unsafe fix since it changes runtime behavior, but we don't actually offer an autofix in the situation where we would be overwriting a default like that. So I don't really see yet why this fix is unsafe (is it?).

[dylwil3]

I don't really follow this as a justification - if the user didn't want this, they wouldn't select this lint rule to begin with. Does the fix ever cause a change in runtime behavior? Is it too likely that there are false positives, or are there known false positives? That's the sort of thing we're looking for here.

[dscorbett]

The fix is unsafe because, although switching to InitVar is usually correct, it is incorrect when the parameter is not meant to be a field exposed in the public API, or when the value should be shared between all instances. The examples below use a defaulted parameter as a class-level cache; the correct fix would be to switch to ClassVar.

Here is an example where the fix changes runtime behavior:

$ cat >ruf033_1.py <<'# EOF'
from dataclasses import dataclass, field
@dataclass
class Foo:
    name: str
    serial_number: int = field(init=False)
    def __post_init__(self, cache: dict[str, int] = {}) -> None:
        self.serial_number = cache.setdefault(self.name, len(cache))
print(Foo("A").serial_number)
print(Foo("B").serial_number)
print(Foo("A").serial_number)
try:
    print(Foo("A", cache={"A": 10}).serial_number)
except TypeError:
    pass
else:
    print("Error: `cache` should not be accessible in the API")
# EOF

$ python ruf033_1.py
0
1
0

$ ruff --isolated check ruf033_1.py --select RUF033 --unsafe-fixes --fix
Found 1 error (1 fixed, 0 remaining).

$ python ruf033_1.py                                                    
0
1
0
10
Error: `cache` should not be accessible in the API

Here is another:

$ cat >ruf033_2.py <<'# EOF'
from dataclasses import dataclass, field
@dataclass
class Foo:
    name: str
    serial_number: int = field(init=False)
    def __post_init__(self, *, cache: dict[str, int] = {}) -> None:
        self.serial_number = cache.setdefault(self.name, len(cache))
print(Foo("A").serial_number)
print(Foo("B").serial_number)
print(Foo("A").serial_number)
# EOF

$ python ruf033_2.py
0
1
0

$ ruff --isolated check ruf033_2.py --select RUF033 --unsafe-fixes --fix
Found 1 error (1 fixed, 0 remaining).

$ python ruf033_2.py
Traceback (most recent call last):
  File "ruf033.py", line 9, in <module>
    print(Foo("A").serial_number)
          ^^^^^^^^
  File "<string>", line 4, in __init__
TypeError: Foo.__post_init__() takes 1 positional argument but 2 were given

[VascoSch92]

I could not reproduce the exception being raised in the example in your PR summary (but maybe I've done something wrong?), nor could I see why the example fix in the original PR was unsafe. The example in the docs for the rule of a suggested change would be an unsafe fix since it changes runtime behavior, but we don't actually offer an autofix in the situation where we would be overwriting a default like that. So I don't really see yet why this fix is unsafe (is it?).

Ok, strange... It works on ruff playground, i.e., the rule is triggered.

I had in mind the same reason of @dscorbett but I could not explain it so clear ;-)

I will change the section accordingly.

[dylwil3]

Ok, strange... It works on ruff playground, i.e., the rule is triggered.

I agree that the rule is triggered, but when you actually run the code it does not raise an exception (both before and after the fix).

[dylwil3]

Thank you! (and thanks @dscorbett !)