Investigate false negatives for ``unsupported-binary-operation``

[DudeNr33]

Following the discussion here:

Originally posted by @DanielNoord in #6353 (comment)

unsupported-binary-operation was previously disabled due to too many false positives.
Maybe we can investigate if we can re-enable it?

[omarandlorraine]

Have we got a regression test file for (at least some of the) false positives?

[DanielNoord]

I don't think so. If you want to look into this you could (for example) use some of the projects we have in our primer and see what happens with:
pylint --disable=all --enable=unsupported-binary-operation after having re-enabled the checker. That might show whether there are too many false positives still.

[jacobtylerwalls]

Actually, I think this issue is wrong -- the check is enabled currently, see _detect_unsupported_alternative_union_syntax and #4256

[jacobtylerwalls]

Clearly a cleanup is needed to prevent confusion and remove the todos.

[DanielNoord]

I think the message is partially emitted. There is visit_binop method which seems to do something, but there is also a _visit_binop and a _visit_augassign both of which call _check_binop_errors. Those two methods are decorated with @only_required_for_messages("unsupported-binary-operation") but never called.

We probably need to check if we should indeed look at AugAssign and whether there is something in _check_binop_errors that _detect_unsupported_alternative_union_syntax does not do.

[jacobtylerwalls]

_check_binop_errors doesn't exist anymore :D

I think we can just remove the TODOs, there are tests for += in unsupported_binary_operation.py

[DanielNoord]

https://github.com/PyCQA/pylint/blob/0a4204fd7555cfedd43f43017c94d24ef48244a5/pylint/checkers/typecheck.py#L1921-L1929

🤔

[jacobtylerwalls]

oh my word. the things that happen when you investigate an issue by deleting the routine you're talking about. ☕

[jacobtylerwalls]

The code there looks very similar to visit_unary_op() which should also be investigated and cleaned up.

[DanielNoord]

https://github.com/PyCQA/pylint/blob/0a4204fd7555cfedd43f43017c94d24ef48244a5/pylint/checkers/typecheck.py#L1921-L1929

🤔

[jacobtylerwalls]

oh my word. the things that happen when you investigate an issue by deleting the routine you're talking about. ☕

[jacobtylerwalls]

The code there looks very similar to visit_unary_op() which should also be investigated and cleaned up.

[DudeNr33]

I have trouble following the discussion.
For some cases, unsupported-binary-operation is emitted even with the current implementation, yes, but that is a lot less than originally intended:

If you check tests/functional/a/alternative/alternative_union_syntax_error.py, there are many examples where this message should trigger, but does not. Things like simple 1 + '2' etc.

Regarding the false positives: I re-enabled this check locally. Running the functional tests, two false positives that immediately pop up are:

• old style string formatting: "%d" % 1 (e.g. in functional test bad_string_format_type)
• instance attributes with type hints (simplified from dataclass_typecheck):

@dataclass
class MyClass:
    attr1: int

print(1 + obj.attr1)

I think there is value in this check, but as @DanielNoord suggested we should definitely run it against some bigger projects first and try to find as many false positives as possible first.
I think this should not be too difficult for most parts: this message should never be raised in properly tested code, as any real violations would lead to a TypeError at runtime.

[jacobtylerwalls]

Sorry for muddying the waters. I read the issue to be claiming that the message wasn't being emitted at all. That wasn't true, so I chimed in because I thought there was a misunderstanding (I was certainly not understanding!). And at a glance I couldn't even see any check logic being implemented. Looking just now I see that the responsibility for finding the errors is in astroid, and they're just passed up for this pair of commented-out methods to add messages for. Got it now. Sorry again!

If you check tests/functional/a/alternative/alternative_union_syntax_error.py, there are many examples where this message should trigger, but does not. Things like simple 1 + '2' etc.

Would you be able to point to a couple examples? I looked and nothing jumped out, since almost every line in the file emits the message. I just want to understand.

[Removed example that was run incorrectly.] @DudeNr33 is correct that 1 + '2' should emit a message. Still think it's worth listing the other ones we want to catch here, since I wasn't seeing them in the file.

[DudeNr33]

No problem @jacobtylerwalls, the opening post was not exactly what one would called a detailed description. 😄

When I run

pylint --disable=all --enable=unsupported-binary-operation tests/functional/u/unsupported/unsupported_binary_operation.py

I don't see any message. So it's more or less the whole file.

So for binary operations:

1 + "a" # [unsupported-binary-operation]
1 - [] # [unsupported-binary-operation]
1 * {} # [unsupported-binary-operation]
1 / collections # [unsupported-binary-operation]
1 ** (lambda x: x) # [unsupported-binary-operation]
{} * {} # [unsupported-binary-operation]
{} - {} # [unsupported-binary-operation]
{} | {} # [unsupported-binary-operation]
{} >> {} # [unsupported-binary-operation]
[] + () # [unsupported-binary-operation]
() + [] # [unsupported-binary-operation]
[] * 2.0 # [unsupported-binary-operation]
() * 2.0 # [unsupported-binary-operation]
2.0 >> 2.0 # [unsupported-binary-operation]

And augmented assignments like this:

TTT = 1
TTT += [] # [unsupported-binary-operation]

[jacobtylerwalls]

Ah, we were looking at different files. I think you said alternative_union_syntax_error.py above, but now I follow. NBD!

[Mi-La]

Hi,
I have test case which fires false-positive for unsupported-binary-operation.

class TestObject:
    def __init__(self, value):
        self._value = value

    def __or__(self, other):
        return TestObject(self._value | other._value)

    @property
    def value(self):
        return self._value

    class Values:
        ONE = None
        TWO = None

TestObject.Values.ONE = TestObject(1)
TestObject.Values.TWO = TestObject(2)

obj = TestObject.Values.ONE | TestObject.Values.TWO # fires here

Note that without the inner class Values pylint doesn't fire. It also doesn't fire when default assignments to None is ommited (i.e. without ONE= None, etc.)

Note that it fires with Python 3.8.16, while it works well with Python 3.11! (with same versions of astroid and pylint.

Configuration:
Pyhton: 3.8.16
Astroid: 2.15.0
Pylint: 2.16.3

pylint --disable=all --enable=unsupported-binary-operation unsupported_binary_op.py