[preview] FURB118 can be unsafe for methods

[henryiii]

If you run with preview set, then the FURB118 check makes an unsafe replacement, you can see me rolling back the change wntrblm/nox@a91dbe2 to make the tests pass, the difference in behavior can be shown with this MWE:

>>> class A:
...     pass
...  
>>> A.__eq__ = lambda a, b: a == b
>>> A() == A()
Traceback (most recent call last):
  File "<python-input-5>", line 1, in <module>
    A() == A()
  File "<python-input-4>", line 1, in <lambda>
    A.__eq__ = lambda a, b: a == b
                            ^^^^^^
  File "<python-input-4>", line 1, in <lambda>
    A.__eq__ = lambda a, b: a == b
                            ^^^^^^
  File "<python-input-4>", line 1, in <lambda>
    A.__eq__ = lambda a, b: a == b
                            ^^^^^^
  [Previous line repeated 988 more times]
RecursionError: maximum recursion depth exceeded
>>> import operator
>>> A.__eq__ = operator.eq
>>> A() == A()
Traceback (most recent call last):
  File "<python-input-8>", line 1, in <module>
    A() == A()
TypeError: eq expected 2 arguments, got 1

[AlexWaygood]

Thanks for opening the issue. FURB118 has been something of a problematic rule for us in terms of the number of issues we've had regarding it :/

Here's a slightly smaller repro for the issue here, that makes it slightly clearer how the suggested change can break working code:

>>> import operator
>>> class Spam:
...     foo = lambda self, other: self == other
... 
>>> class Eggs:
...     foo = operator.eq
... 
>>> Spam().foo(Spam())
False
>>> Eggs().foo(Eggs())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: eq expected 2 arguments, got 1

It seems like the operator functions are always unsuitable for use as method definitions (whether monkeypatched or otherwise), because they don't have __get__ methods in the same way that user-defined functions do. When you access the foo method on an instance of Spam in my REPL example, you don't actually get the foo function; behind the scenes, foo.__get__ is called and you get back a MethodType instance; this is why you don't need to pass in self explicitly in most method calls. But when you access the foo method on an instance of Eggs, you get back the original operator.eq function instead of a MethodType instance, because operator.eq has no __get__ method to call.

The next question is what we can do about this. I think there are a few things we can do:

• Document more clearly that this rule's autofix is always unsafe. It is always categorised as an unsafe fix (you have to opt into it with --unsafe-fixes), but our docs currently emphasise that it's "usually safe". While I think it is true that the fix is usually safe, we don't actually explicitly state anywhere in our docs currently that it's always categorised as an unsafe fix; we probably should.
• We should list the missing __get__ method on the operator functions as another reason why the fix is unsafe in our docs
• We should avoid emitting the diagnostic at all for direct assignments in class bodies, like in my REPL example above

What I think we can't do, unfortunately, is avoid emitting the diagnostic in dynamic cases where you're monkey-patching a method onto a class, like the original situation you ran into in your nox PR. Possibly we might be able to do that with more type inference, but for now I think it's well beyond our capabilities to reliably detect whether the thing you're monkeypatching the function onto is a class object or not.

[henryiii]

I was under the impression it was a "safe" fix, but I reran it and it does only show up as an unsafe fix. (Aside: it would be nice if unsafe fixes were also indicated in the default printout, like [**] or something).

All three improvements sound good. Probably no rush as it's not out of preview yet.

[AlexWaygood]

(Aside: it would be nice if unsafe fixes were also indicated in the default printout, like [**] or something).

This might possibly be addressed by #7352, which is definitely something we want to do at some point!