[refurb] Unsafe fixes in FURB161

[VascoSch92]

The PR fixes #16457 .

Specifically, FURB161 is marked safe, but the rule generates safe fixes only in specific cases. Therefore, we attempt to mark the fix as unsafe when we are not in one of these cases.

For instances, the fix is marked as aunsafe just in case of strings (as pointed out in the issue). Let me know if I should change something.

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[MichaReiser]

Thanks for looking into this. Could you update the rule's documentation with a ## Fif safety header and explain when the fix is safe (or isn't)? This will also make reviewing easier

[dscorbett]

This PR doesn’t fully fix #16457. The point of that issue is that FURB161 should be unsafe by default, with a few exceptions. This PR keeps it safe by default, the only exception being string literals.

[VascoSch92]

This PR doesn’t fully fix #16457. The point of that issue is that FURB161 should be unsafe by default, with a few exceptions. This PR keeps it safe by default, the only exception being string literals.

Thanks for the explanation. I approved the modification to make the rule unsafe if we can not determine if the argument of the bin method is of type int or bool

[dscorbett]

Using ResolvedPythonType::from would help catch more expressions than literals, like -1 or 1 + 2.

[ntBre]

Thanks for working on this! Overall I think this is good, but we can simplify things a bit by using a fix Applicability, and then I think we can do a better job with type inference with ResolvedPythonType::from, specifically for Expr::BinOp (cases like 1 + 2) and for Expr::UnOp (cases like -1).

If we extend the safe handling to bool, we may also be able to resolve expressions like True or False.

[ntBre]

We also want to say explicitly when it is safe, since it's not always unsafe. Maybe something like this?

Suggested change

	/// ## Fix safety
	/// This rule's fix is marked as unsafe, as the `bit_count()` method can be used when the argument
	/// to the `bin()` is an instance of a type that implements the `__index__` and `bit_count` methods.
	///
	/// ## Fix safety
	/// This rule's fix is marked as unsafe unless the argument to `bin` can be inferred as
	/// an instance of a type that implements the `__index__` and `bit_count` methods.
	///

I think we could also possibly drop the mention of __index__ since we're always just calling bit_count right? Based on my reading of the __index__ docs, it's actually a requirement for calling bin itself rather than a requirement to call bit_count as seems more relevant here.

[ntBre]

I think instead of returning a second bool here, you can just return an Applicability and use that with Fix::applicable_edit instead of branching on fix_is_safe down below.

[ntBre]

I can't highlight the line directly, but I think Expr::BooleanLiteral should also allow a safe edit, at least that's what it sounds like from the issue.

[ntBre]

This is a separate issue from the changes here, but I don't think Expr::Names need to be parenthesized. Let's not modify this here, but I'm surprised to see (x).bit_count instead of x.bit_count.

[ntBre]

This one is also a bit unfortunate because this is obviously an integer. I think this would be helped by ResolvedPythonType::from as @dscorbett suggested.