mypy cannot typecheck event classes decorated with @attrs_event

[wetmore]

Description of the problem

Due to a known bug/missing feature in the mypy plugin for attrs, mypy cannot correctly infer keyword arguments for a class which is decorated with @attrs_event, or inherits from a class decorated with @attrs_event.

The relevant issues on mypy: python/mypy#5406
Two related issues: python-attrs/attrs#594, python/mypy#6239

There are workarounds provided in the issues. However, as a quick fix, it's possible to just replace all instances of @attrs_event with @attr.s(slots=True, kw_only=kw_only, frozen=True), after which mypy seems to typecheck fine. Since the @attrs_event decorator is not exported from fbchat anyway, this seems like an acceptable fix,

While this project doesn't seem to use mypy, it would be nice to fix this so that other projects that use fbchat can use mypy. It's a shame having type hints in fbchat that can't be leveraged by dependants.

For now, I can just workaround with # type: ignore, but this means I can't get the benefit of typechecking for any events I subclass from those provided by fbchat.

Code to reproduce

import fbchat
import attr

@attr.s
class MyEvent(fbchat.MessageEvent):
    def makeMyEvent(self, event: fbchat.MessageEvent):
        return MyEvent(thread=event.thread, author=event.author, message=event.message, at=event.at)

Traceback

Unexpected keyword argument "thread" for "MyEvent"mypy(error)
Unexpected keyword argument "author" for "MyEvent"mypy(error)
Unexpected keyword argument "message" for "MyEvent"mypy(error)
Unexpected keyword argument "at" for "MyEvent"mypy(error)

Environment information

• Python version 3.8.3
• fbchat version 2.0.0a2

[madsmtm]

I've been wanting to enable mypy, but haven't for the same reason. But in my testing, using @attr.s(slots=True, kw_only=kw_only, frozen=True) directly didn't work either, mypy couldn't handle the dynamic kw_only parameter, has this been fixed? If so, I'll gladly accept a PR that changes these!

Same with @attrs_default

No matter what, we could also just drop Python 3.5 support, and then we wouldn't have to generate the conditional kw_only parameter, though I'd rather wait until it reaches EOL in september, see this table

[wetmore]

So for my use case, I was just talking about using mypy on a project which depends on fbchat, not using mypy on fbchat itself. I performed my tests by editing the code for fbchat in my project's venv. I tested using 1) kw_only=True and 2) kw_only=kw_only, and in both cases my project's code would typechecked after making those changes. In both cases I also cleared my mypy cache before typechecking.

I just repeated test 2) by cloning fbchat from github, changing @attrs_event to @attr.s(slots=True, kw_only=kw_only, frozen=True), and creating the following test.py in the fbchat root:

# Test creating an event
def copyMessageEvent(self, event: fbchat.MessageEvent):
    return fbchat.MessageEvent(
        thread=event.thread, author=event.author, message=event.message, at=event.at
    )


# Test creating an event with incorrect args (should not type check)
bad_message = fbchat.MessageEvent(
    thread="not a thread",
    author="not a user",
    message="not a message",
    at="not a datetime",
)


# Test inheriting
@attr.s
class MyEvent(fbchat.MessageEvent):
    def makeMyEvent(self, event: fbchat.MessageEvent):
        return MyEvent(
            thread=event.thread, author=event.author, message=event.message, at=event.at
        )

I committed the changes to the event code, so I can test mypy on test.py with or without my changes applied.

Without my changes, I get the "Unexpected keyword argument..." errors for each use of fbchat.MessageEvent(...) or MyEvent(...).

With my changes, I no longer get type errors for copyMessageEvent and makeMyEvent, and I do get the expected type error for each bad argument passed to fbchat.MessageEvent in bad_message.

Further investigations into kw_only=kw_only

I do get more mypy errors for the files I changed, because of the use of kw_only=kw_only. But I'm guessing when mypy typechecks third-party imports, it only collects type information necessary for typechecking and ignores any errors it finds in the third-party code. mypy doesn't need to parse kw_only in order to collect the necessary type info to typecheck test.py. mypy parsing kw_only is only necessary to reject the following code:

def copyMessageEvent_no_kw(self, event: fbchat.MessageEvent):
    return fbchat.MessageEvent(event.author, event.thread, event.message, event.at)

Before my changes, this does not typecheck, failing with "Too many arguments for "MessageEvent"".

After my changes, this typechecks, but since kw_only evaluates to True, it shouldn't. This code will fail at runtime, because fbchat.MessageEvent doesn't accept positional arguments. If I replace all the occurrences of kw_only=kw_only to kw_only=True in the events code, then mypy will correctly reject the code, so that's what mypy needs to be able to parse kw_only for

So I guess there is a tradeoff here:

• Keep @attrs_event: Code using kwargs when creating events or classes which inherit from them will not typecheck, but code creating events without using kwargs will correctly throw an error with mypy.

• Replace @attrs_event: Code using kwargs when creating events or classes which inherit from them will typecheck, but code creating events without using kwargs will not throw an error with mypy when it should.

In either situation it's possible to create a runtime error because of faulty typechecking. However, I think it's worth it to replace @attrs_event, because it's less likely someone will try to create an event without using kwargs, since that's not used in any examples.

Sorry this got so long, and thanks for your time maintaining this package!