Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unable to assign a function a method #2427

Closed
rowillia opened this issue Nov 10, 2016 · 23 comments · Fixed by #14570
Closed

Unable to assign a function a method #2427

rowillia opened this issue Nov 10, 2016 · 23 comments · Fixed by #14570
Labels
bug mypy got something wrong false-positive mypy gave an error on correct code needs discussion priority-1-normal

Comments

@rowillia
Copy link
Contributor

For example:

from typing import Any, Optional


class Foo(object):
    def __init__(self):
        # type: () -> None
        self.x = 0

    def get_x(self, a=None, b=None):
        # type: (Optional[int], Optional[int]) -> int
        return self.x + (a or 0) + (b or 0)


def get_x_patch(self, *args, **kwargs):
    #  type: (Foo, *Any, **Any) -> int
    return 42


Foo.get_x = get_x_patch

Results:

test_method_assignment.py:19: error: Cannot assign to a method

This assignment should be legal as any call to get_x will be able to call get_x_patch. It looks like 3ce8d6a explicitly disallowed all method assignments, but there's not a ton of context behind it.

I know monkeypatching is generally frowned upon, but is unfortunately a very popular part of Python. I can always mark those lines as ignored, but I'd rather be able to test that the patch is compatible with the underlying method with mypy.

@refi64
Copy link
Contributor

refi64 commented Nov 10, 2016

I swear, this is a duplicate, but I can't find the issue # yet...

@rowillia
Copy link
Contributor Author

@kirbyfan64 Yeah...I poked around and couldn't find anything. Happy to close this if it is!

@gvanrossum
Copy link
Member

I think @sixolet might be interesting in this...

@JukkaL
Copy link
Collaborator

JukkaL commented Nov 10, 2016

The mypy callable type representation isn't expressive enough to to check assignments to methods precisely. In particular, at least bound methods and unbound function objects should be treated differently. Consider this example:

class A:
    def f(self): pass
    def g(self): pass

def h(self): pass

A.f = h   # rvalue has one argument (unbound)
A().f()  # okay
A.f = A().g  # rvalue has no arguments (bound)
A().f()  # okay 

When we have value with an annotated callable type, such as Callable[[A], None], mypy can't decide whether this is a bound or unbound function method/function. Static methods and class methods might complicate this further.

I'm pretty sure this is already broken in other contexts, but we may want to resolve this eventually. Some random ideas:

  1. Internally keep track whether a callable is bound so that we can do more precise checking. This would work for expressions with inferred types. For values explicitly annotated with a Callable type, the value of the flag would be unknown and these couldn't be used for monkey patching (unless the argument types are ...).
  2. Like (1), but make some assumptions about annotated Callable types. Either assume that they are unbound, or optimistically assume that they have a compatible 'unboundedness' for any particular context. This is unsafe. This is basically how things work right now, though perhaps not consistently.
  3. Add syntax for specifying callables that are always bound or unbound. We'd likely need three different variants: either bound or unbound (likely spelled just Callable), bound (for example, BoundMethod) and unbound (for example, UnboundCallable).

Option (3) doesn't seem worth the added complexity, to be honest, as it's always possible to fall back to Callable[..., X].

@gvanrossum
Copy link
Member

So is the problem that when we see

class C:
    f = None  # type: Callable[[int, int], int]

we don't know whether that defines an instance variable or a class variable? Maybe we can use ClassVar (introduced by PEP 526 into the typing module)? E.g.

class C:
    f = None  # type: ClassVar[Callable[[int, int], int]]

and if ClassVar is not used assume f refers to an instance variable.

Of course initializations inside __init__ are unambiguous. And so are method definitions (with or without @staticmethod or @classmethod).

I do think mypy ought to be fully aware of bound and unbound methods.

But perhaps the original problem is due to something else? The immediate problem seems to be that we don't try to match *args, **kwds against a=None, b=None?

@ilevkivskyi
Copy link
Member

Isn't this a duplicate of #708?

But maybe it makes sense to keep this open, since this issue contains some additional discussion.

@gvanrossum
Copy link
Member

Not really -- IIUC this seems about monkey-patching a class, whereas #708 is about assigning to function attributes.

@Jym77
Copy link

Jym77 commented Apr 25, 2018

A case where I keep running into that issue is when writing unit tests and trying to replace methods with MagicMock().

Typically, class Foo is defined and tested somewhere and class FooBar uses (an instance of) Foo, but in order to unit test FooBar I don't really need/want to make actual calls to Foo methods (which can either take a long time to compute, or require some setup (eg, networking) that isn't here for unit test, …) So, I heavily Mock() the methods which allow to test that the correct calls are issued and thus test FooBar.

It does feel bad to add a bunch a # type: ignore on all these mocks :-(

@JukkaL
Copy link
Collaborator

JukkaL commented Apr 25, 2018

I prefer setattr over using # type: ignore. Mypy won't complain about it. This is something we could discuss in the common issues section in the docs.

@JukkaL JukkaL added the false-positive mypy gave an error on correct code label May 19, 2018
@ikelos
Copy link

ikelos commented Apr 5, 2019

What's the state of this (about monkey patching a method)? It seems like it needed discussion, has that happened offline?

@JukkaL
Copy link
Collaborator

JukkaL commented Apr 5, 2019

The has been no progress recently. The workarounds discussed above (setattr or # type: ignore) are still the recommended ways to deal with this.

@ikelos
Copy link

ikelos commented Apr 5, 2019

Cool, thanks for the update. 5:)

@compwron
Copy link

I think that I am running into this. The code that causes the mypy error is FileDownloader.download = classmethod(lambda a, filename: open(f'tests/fixtures/{filename}', 'rb'))
The error is error: Cannot assign to a method
version is mypy==0.620

@dpinol
Copy link

dpinol commented Oct 23, 2019

Hi, any progress on this?

@jbcpollak
Copy link

In my case I'm not even monkey-patching (at least, I don't feel like it is), I'm trying to take a function as a parameter of init and use it as a wrapper. Like this (note simplified example, so it might not make entire sense):

Adapter = Callable[[S_co], T_contra]

class AdaptingRunner():
    adapter: Adapter
    runner: Runner[T_contra]

    def __init__(self, adapter: Adapter, runner: Runner[T_contra]):
        self.adapter = adapter  # Error here
        self.runner = runner

    def run(self, obj: S_co):
        return self.runner.run(self.adapter(ob))

If I remove adapter: Adapter, everything is fine, but if I declare it, then I get the referenced error.

askhade pushed a commit to onnx/onnx that referenced this issue Jan 19, 2022
* mypy: update to 0.760, remove protobuf stubs

- We can remove the stubs in favor of types-protobuf package
- mypy==0.600 doesn't install properly for me, updating to a somewhat newer version that does install but doesn't change much logic otherwise

Signed-off-by: Chris Hua <[email protected]>

* mypy: use setattr

per python/mypy#2427

Signed-off-by: Chris Hua <[email protected]>

* mypy: fix type signatures

Signed-off-by: Christopher Hua <[email protected]>
@iainelder
Copy link

I prefer setattr over using # type: ignore. Mypy won't complain about it. This is something we could discuss in the common issues section in the docs.

It might silence mypy, but it's one of flakeheaven's bugbears.

B010 Do not call setattr with a constant attribute value, it is not any safer than normal property access. [flake8-bugbear]

So I still prefer to use type:ignore with a comment about what is being ignored. For example, mypy also more usefully points out when the callable signatures don't match.

deeplow added a commit to freedomofpress/dangerzone that referenced this issue Aug 4, 2022
ignore method assignment. Currently mypy cannot check this.
Related upstream issues:
  - python/mypy#2427
  - python/mypy#708
deeplow added a commit to freedomofpress/dangerzone that referenced this issue Aug 16, 2022
ignore method assignment. Currently mypy cannot check this.
Related upstream issues:
  - python/mypy#2427
  - python/mypy#708
deeplow added a commit to freedomofpress/dangerzone that referenced this issue Aug 16, 2022
ignore method assignment. Currently mypy cannot check this.
Related upstream issues:
  - python/mypy#2427
  - python/mypy#708
deeplow added a commit to freedomofpress/dangerzone that referenced this issue Aug 16, 2022
ignore method assignment. Currently mypy cannot check this.
Related upstream issues:
  - python/mypy#2427
  - python/mypy#708
deeplow added a commit to freedomofpress/dangerzone that referenced this issue Aug 17, 2022
ignore method assignment. Currently mypy cannot check this.
Related upstream issues:
  - python/mypy#2427
  - python/mypy#708
deeplow added a commit to freedomofpress/dangerzone that referenced this issue Aug 22, 2022
ignore method assignment. Currently mypy cannot check this.
Related upstream issues:
  - python/mypy#2427
  - python/mypy#708
aqk added a commit to Chia-Network/chia-blockchain that referenced this issue Oct 4, 2022
anoadragon453 added a commit to matrix-org/synapse that referenced this issue Jan 21, 2023
mypy doesn't appear to have a better way to handle this.
python/mypy#2427

Convert method assignments to Mock usages were possible.
ilevkivskyi added a commit that referenced this issue Feb 5, 2023
Fixes #2427

I also add some special logic, so that `# type: ignore[assignment]` will cover `[method-assign]`.
@ilevkivskyi
Copy link
Member

For posterity, after some offline discussions we agreed that it would be hard to find semantics here that would satisfy everyone, and instead there will be a dedicated error code for this case. If you don't want mypy to complain about assignments to methods, use --disable-error-code=method-assign (starting mypy 1.1.0)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong false-positive mypy gave an error on correct code needs discussion priority-1-normal
Projects
None yet
Development

Successfully merging a pull request may close this issue.