A way to prevent MyPy from implictly checking the __r<magic method>__

[arnav-jain1]

Feature

When type checking an overloaded magic method, mypy will implicitly reverse the operands and check it with the reversed operands. A flag to prevent this would be very helpful

Pitch

Some operations are not commutative and checking the reverse may end up with a different result causing confusion and errors. I am making a plugin and when checking a * b, whether or not it fails, it will also check b * a. This is incredibly annoying as if the former fails, it will check the latter. If the latter is successful, it will overwrite the failure. Being able to prevent this, either via a flag or some other way would be super nice, especially when writing plugins.

[JelleZijlstra]

This sounds like you are saying mypy incorrectly typechecks operators like + in some cases. Could you give a self-contained example to clarify where the bug is?

[arnav-jain1]

Yeah, messing with it more I think I may have discovered a bug.
I have a plugin like this that takes in the sum of 2 ints and returns a literal type of the sum.

from mypy.types import LiteralType
from mypy.plugin import Plugin

def type_add(ctx, *args):
    lhs = ctx.type
    rhs = ctx.arg_types[0][0]
    print(lhs)
    print(rhs)
    ret = lhs.value + rhs.value
    # ctx.api.fail("test", ctx.context)
    return LiteralType(ret, fallback=ctx.api.named_generic_type('builtins.int', []))

TYPE_DICT = {
    'builtins.int.__add__': type_add
}

class TestPlugin(Plugin):

    def get_method_hook(self, fullname):
        print(fullname)
        return TYPE_DICT.get(fullname, None)

def plugin(version: str):
    return TestPlugin

The input is essentially 3 + 4

When the ctx.api.fail is commented out. The result is to be expected:

builtins.int.__add__
builtins.int.__add__
Literal[3]
Literal[4]
Success: no issues found in 1 source file

When the ctx.api.fail is used, MyPy attempts to check it multiple times. Half of which are with the LHS and RHS flipped

builtins.int.__add__
builtins.int.__add__
Literal[3]
Literal[4]
builtins.int.__add__
builtins.int.__add__
Literal[4]
Literal[3]
builtins.int.__add__
builtins.int.__add__
Literal[3]
Literal[4]
builtins.int.__add__
builtins.int.__add__
Literal[4]
Literal[3]
test_add.py:6: error: test  [misc]
Found 1 error in 1 file (checked 1 source file)

From what I can tell, the flipping part is intended (and I would like to disable it) but I am not sure why it is checking it an extra time. If you would like to see the full code, it is right here: https://github.com/arnav-jain1/mypy_bug
Let me know if you need any more info. I can also recreate the bug in a more useful setting where the operation is non-commutative to show what I mean.

[A5rocks]

I am not sure why it is checking it an extra time

I would recommend checking the stack trace! IIRC there's a thing where if an expression fails we check whether it would work in an await for the suggestion so maybe something like that is the cause.

[arnav-jain1]

While that does make sense, it is still getting called with the lhs and rhs flipped, which I would like to prevent or ignore. Is there any way to do so?

[sterliakov]

Happens here:

mypy/mypy/checkexpr.py

Lines 4097 to 4114 in c274971

	for name, method, obj, arg in variants:
	with self.msg.filter_errors(save_filtered_errors=True) as local_errors:
	result = self.check_method_call(op_name, obj, method, [arg], [ARG_POS], context)
	if local_errors.has_new_errors():
	errors.append(local_errors.filtered_errors())
	results.append(result)
	else:
	if isinstance(obj, Instance) and isinstance(
	defn := obj.type.get_method(name), OverloadedFuncDef
	):
	for item in defn.items:
	if (
	isinstance(item, Decorator)
	and isinstance(typ := item.func.type, CallableType)
	and bind_self(typ) == result[1]
	):
	self.chk.check_deprecated(item.func, context)
	return result

If the plugin produces any errors, we discard that variant and go try __radd__ instead. __radd__ shouldn't be handled by plugin (but actually is due to name confusion, see below). Both calls result in an error, so we go further and return Any. This is by design, but I'm not sure it is correct.

L4099 is definitely a bug: check_method_call is passed op_name (original operator method's name), not name (loop variable gathered in variants), so the plugin is triggered for int.__add__ while we're checking a call of int.__radd__.

[sterliakov]

And the logic of checking __radd__ is sound, this indeed works by design. We only check simple calls with fixed args there, so any errors do really mean "foo of foo + bar says it doesn't want to be added to bar". By convention, foo should return NotImplemented and not raise any error, so that other side has a say. If bar says it's perfectly addable to foo, there's nothing to check further: this call is valid.

Since there's no standard way to statically denote "this op method is non-cooperative and raises for unsupported arguments" as of now, mypy implements the typical case as the default one. If you need to support that scenario using a plugin... well, we need to fix the wrong method name (PRs welcome!), and you probably need to handle __radd__ in plugin code to generate an error. If you post your actual use case, we may be able to suggest something else.

The fix is as easy as

diff --git a/mypy/checkexpr.py b/mypy/checkexpr.py
index e7c2cba3f..9308164de 100644
--- a/mypy/checkexpr.py
+++ b/mypy/checkexpr.py
@@ -4096,7 +4096,7 @@ class ExpressionChecker(ExpressionVisitor[Type], ExpressionCheckerSharedApi):
         results = []
         for name, method, obj, arg in variants:
             with self.msg.filter_errors(save_filtered_errors=True) as local_errors:
-                result = self.check_method_call(op_name, obj, method, [arg], [ARG_POS], context)
+                result = self.check_method_call(name, obj, method, [arg], [ARG_POS], context)
             if local_errors.has_new_errors():
                 errors.append(local_errors.filtered_errors())
                 results.append(result)

and a test may involve a custom plugin (see test-data/unit/check-custom-plugin.test for how it's done) that produces different errors for __add__ and __radd__ so that both are visible in the output.

[arnav-jain1]

Gotcha thank you! I will take a look and submit a PR

[arnav-jain1]

I made the change but when I was testing it against with this plugin (previous Github repo has this too) I had a question

from mypy.types import LiteralType
from mypy.plugin import Plugin

def type_add(ctx, *args):
    lhs = ctx.type
    rhs = ctx.arg_types[0][0]
    print(lhs, rhs)
    ret = lhs.value + rhs.value
    ctx.api.fail("test", ctx.context)
    return LiteralType(ret, fallback=ctx.api.named_generic_type('builtins.int', []))

class TestPlugin(Plugin):

    def get_method_hook(self, fullname):
        if fullname == 'builtins.int.__add__':
            return type_add
        return None

def plugin(version: str):
    return TestPlugin

When running it with the change, the output is

builtins.int.__add__
builtins.int.__add__
Literal[3] Literal[4]
builtins.int.__radd__
Success: no issues found in 1 source file

add is called twice and then radd is called so it is differentiating it but since there is no method for radd, there is no error so it treats it as no error. Should I leave it like this and require the plugin creator to have a radd method along with an add method (could potentially break old code, don't know though) or should I change the code so that add and radd are handled separately and if add has an error it will return the error (this I might need guidance on if there is an "elegant" non hacky way to do it) or any other way that you guys suggest.

[sterliakov]

Ohh. No, please don't change this existing logic, it's sound - an error in one method is now consistently interpreted as a sign that the next one should be tried instead. There's no elegant way to only filter plugin-generated errors AFAIC.

This level of breaking compat is acceptable IMO (most likely there's no single plugin in wild checking there dunders, or we'd have received a report earlier) - we'll post a notice in plugin changes issue and call it a day. If someone really needs to prevent using such additions, the plugin update is trivial and arguably the plugin code was incorrect if it didn't handle __radd__ before - that's a mypy bug that __radd__ name wasn't passed properly:)

[arnav-jain1]

Gotcha. I am working on creating the test case and submitting the PR but I ran into a small issue. After making the changes, I created a test case with this as the plugin

from mypy.types import LiteralType, AnyType, TypeOfAny
from mypy.plugin import Plugin

def type_add(ctx, *args):
    lhs = ctx.type
    rhs = ctx.arg_types[0][0]
    ret = lhs.value + rhs.value
    ctx.api.fail("test", ctx.context)
    return AnyType(TypeOfAny.from_error)

def type_radd(ctx, *args):
    lhs = ctx.type
    rhs = ctx.arg_types[0][0]
    ret = lhs.value + rhs.value
    return LiteralType(ret, fallback=ctx.api.named_generic_type('builtins.int', []))


class TestPlugin(Plugin):

    def get_method_hook(self, fullname):
        if fullname == 'builtins.int.__add__':
            return type_add
        if fullname == 'builtins.int.__radd__':
            return type_radd
        return None

def plugin(version: str):
    return TestPlugin

and this test:

from typing import Literal

op1: Literal[3] = 3
op2: Literal[4] = 4
c = op1 + op2
reveal_type(c) # N: Revealed type is "Literal[7]"

check-custom-plugin.test:

[case magicMethodReverse]
# flags: --config-file tmp/mypy.ini
from typing import Literal

op1: Literal[3] = 3
op2: Literal[4] = 4
c = op1 + op2
reveal_type(c) # N: Revealed type is "Literal[7]"

[file mypy.ini]
\[mypy]
plugins=<ROOT>/test-data/unit/plugins/magic_method.py

For whatever reason, whenever I use pytest -k magicMethodReverse, the function hooks for the test case are:

__add__
__add__

Meaning it is not flipping the operands.
When I call it locally just using the https://github.com/arnav-jain1/mypy_bug repo (which is literally the same code) with python -m mypy --config-file ..\\mypy_bug\\mypy.ini ..\\mypy_bug\\test.py I get the correct hooks:

__add__
__radd__

I am printing these hooks from mypy code itself (temporarily, to figure out why the test is failing), not the plugin, so I am not sure why there is a difference in the output.
Mypy code:

        for name, method, obj, arg in variants:
            with self.msg.filter_errors(save_filtered_errors=True) as local_errors:
                print(name)
                result = self.check_method_call(name, obj, method, [arg], [ARG_POS], context)

This code is also on my Github.

I have never used pytest before, so it might be a pytest thing that I am messing up. But I am not sure why that would be the case or how to fix it. Thank you for your help!

[sterliakov]

It isn't a pytest thing, but our complicated test setup:)

Every test can optionally end with

[builtins fixtures/some_file.pyi]

If you add such a line, it will treat the provided file as a replacement for builtins.pyi. Otherwise it will use another tiny fixture that only contains a few symbols (try isinstance(c, int) - the test will likely fail saying that isinstance is not defined). That's done to make our tests marginally faster (builtins aren't that small!)

So, in that stub int doesn't have __radd__. You may want to specift

[builtins fixtures/ops.pyi]

to your test (all those fixtures live in test-data/unit/fixtures, so you can have a look at what exactly is provided by each one).

[arnav-jain1]

Thank you for your help, hopefully the PR goes through soon.

[A5rocks]

I recommend leaving this issue open, and writing "Fixes <issue link>" in the initial PR comment (you can edit it in). This way, this issue will automatically close when that PR is merged! And if the PR is closed, this issue is still open so others can see it's still a problem.

[arnav-jain1]

Gotcha, my bad.