False positive Unnecessarily calls dunder method __index__

[avylove]

Bug description

A little background. The str justify methods (rjust(), ljust(), center()) take SupportsIndex types for width rather than just straight integers, allowing you to use custom types like so.

>>> class Foo:
...     def __index__(self):
...         return 10
... 
>>> 'abcd'.rjust(Foo())
'      abcd'

If you subclass str and re-implement these methods in a compatible manner, you use width.__index__() instead of using width directly as an integer. With Pylint 2.14.0 this now raises unnecessary-dunder-call which seems intended for sequences, which have an index() method, rather than SupportsIndex types which don't.

A simple example:

"""Example of false positive for Unnecessarily calls dunder method __index__"""

import typing

class MyString(str):
    """Custom str implementation"""

    def rjust(self, width: typing.SupportsIndex, fillchar: str = ' '):
        """Override of str.rjust"""
        width = width.__index__()

Configuration

No response

Command used

pylint index_test.py

Pylint output

************* Module index_test
Desktop/index_test.py:10:16: C2801: Unnecessarily calls dunder method __index__. Use index method. (unnecessary-dunder-call)

-----------------------------------
Your code has been rated at 7.50/10

Expected behavior

Should be a clean run. That said, I understand this is a corner case and may be difficult to detect.

Pylint version

pylint 2.14.0
astroid 2.11.5
Python 3.10.4 (main, Mar 25 2022, 00:00:00) [GCC 12.0.1 20220308 (Red Hat 12.0.1-0)]

OS / Environment

Fedora 36

Additional dependencies

No response

[Pierre-Sassoulas]

@jpy-git do you want to triage that one ? :)

[avylove]

Now that I look at it, since Python 3.8, which is the version SupportsIndex was introduced, int(Foo()) works, so that might be preferred to Foo().__index__(). It's still a different situation than the one the message is intended for.

[clavedeluna]

Brainstorming fix ideas. Would one fix potentially be that when a call to __index__(), to determine that the class is inheriting from str and not output the unncessary dunder call? Or would that potentially lead to a bunch of false negatives?

[avylove]

@clavedeluna, str only comes into play here because some str methods require arguments that include the __index__() method.

This issue with the current message is the __index__() and index() methods are not related to each other. index() is used to find the index of the first occurrence of a value in a sequence. This is limited to sequences. __index__() is used to convert an object to an integer. Any object can implement __index__()

The main question we need to answer is what is the preferred way to invoke object.__index__()?

There are three ways I've found:

1. Directly foo.__index__() Downside is this calls a magic method directly In this case, this check should ignore all uses of __index__().

2. Through the operator module operator.index(foo) In this case the message should be updated

3. Indirectly, through int() int(foo) If __int__() isn't defined, it will fall back to __index__(), and often __index__() is mapped to __int__(), but this makes assumptions that aren't necessarily true, so I think this is the wrong approach. I only include it here because it is common.

@Pierre-Sassoulas, do you think this should this be ignored, reference operator.index(), or something else?

[Pierre-Sassoulas]

I investigated a little and I'm wondering if it's not working as intended. @avylove I guess my question is, why would you use __index__ directly instead of one of bin, oct, hex, complex, int or float depending on what you need ?

[avylove]

It's definitely not correct as it is, because the message instructs the user to use index() which is unrelated.

int() is probably the closest, but the usecase is slightly different. __index__() and __int__() are not guaranteed to return the same value.

>>> class Foo:
...     def __index__(self):
...             return 10
...     def __int__(self):
...             return 5
...
>>> int(Foo())
5
>>> operator.index(Foo())
10

[Pierre-Sassoulas]

All right, then the suggested function should be operator.index instead of index, if we want to keep the spirit of the check right ?

[avylove]

It makes sense to me. I just wasn't sure where you were on suggestions that required an import. But I think that's the best option. It gives the user an alternative to using the dunder method directly and they can always disable the check for that line if they don't want to use the suggestion.

[Pierre-Sassoulas]

Yeah recommending to add an import is not ideal. I'm hesitating between that and allowing __index__ call in order to favor false negative vs false positive. What's your opinion @jacobtylerwalls ?

[jacobtylerwalls]

Let's just relax the check to allow __index__ calls. The spirit of the check as I took it was to replace obvious things like __len__ with len, not deal with this can of worms. :D