[pylint] add fix safety section (PLW3301)

[yunchipang]

parent: #15584
issue: #16163

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[ntBre]

This isn't really a fix safety issue, this is a bug with an open issue that needs to be fixed.

I went through the git blame on this one, and I'm not really sure it actually needs to be unsafe. It was first converted from Fix::unspecified to Fix::suggested in #5251, which became Fix::sometimes_applies, which became Fix::unsafe_edit. I didn't see any specific discussion of the fix being unsafe, so I think it may have just been the more conservative option.

All that to say, it's not clear to me why this rule would need to be unsafe, at least from its description and development history. We even check for comment ranges and avoid a fix entirely if comments are present, so let's hold off on this for now.

[dscorbett]

The min fix is unsafe when __lt__ is not commutative. The max fix is unsafe when __gt__ is not commutative.

$ cat >plw3301.py <<'# EOF'
print(min(2.0, min(float("nan"), 1.0)))
print(max(1.0, max(float("nan"), 2.0)))
# EOF

$ python plw3301.py
2.0
1.0

$ ruff check --isolated --select PLW3301 plw3301.py --unsafe-fixes --fix
Found 2 errors (2 fixed, 0 remaining).

$ python plw3301.py
1.0
2.0

[yunchipang]

@ntBre @dscorbett thanks so much for the input! I've updated the docs, though I'm not exactly sure, referencing the above case, is this rule always or sometimes unsafe? In general, what's makes a rule always/sometimes unsafe?

thanks a lot.

[ntBre]

Ah great catch as always, @dscorbett! I was hoping you might see this one.

I may have mentioned this on another PR, but the way I check for safety is to search the file for safe_edit. If you find cases of Fix::safe_edit and Fix::unsafe_edit, then the rule is sometimes safe. If you only find Fix::unsafe_edit, as I believe is the case here, the rule is always unsafe. You'll have to be slightly more careful with your search range in files with multiple rules, but the general strategy still applies.

[ntBre]

Thanks, this is looking good.

[ntBre]

I'd probably phrase this more like

This fix is always unsafe because it can change the program's behavior in cases where the underlying dunder methods (__lt__ for min and __gt__ for max) are not commutative, as is the case for floats, for example:

[dscorbett]

“Commutative” is the wrong word: I meant “associative”. Sorry for the confusion. Actually, I’m not sure associativity is precisely what is necessary and sufficient here. It’s probably best to avoid the technical jargon unless someone proves it’s correct.

[yunchipang]

@nefrob @dscorbett thanks for the reviews! I’ve adjusted the wording slightly based on my understanding—hope it’s clearer and aligns with the intent. Let me know if there’s any part that could be further improved!

[ntBre]

Suggested change

	/// This fix is always unsafe and may change the program's behavior when expanding
	/// `min()` or `max()` calls. The underlying dunder methods (`__lt__` for `min` and
	/// `__gt__` for `max`) are not associative, particularly when dealing with special
	/// floating-point values like `NaN` (Not a Number).
	/// This fix is always unsafe and may change the program's behavior for
	/// types without full equivalence relations, such as float comparisons involving `NaN`.

I think associativity is the correct property, but this is another possibility that I borrowed from the Rust PartialEq docs. I think we could also phrase it as "types without a total ordering" or something. Maybe associative is close enough to stick with that 😅

[ntBre]

Looks good, thanks! I just pushed one small change putting the section back at the end.