modules as protocols doesn't type-check against the protocol

[asottile]

Bug Report

I was really excited to see #13513 merge but trying it out seems like it doesn't actually type-check things

here's a small example

# main.py
from typing import Protocol

class P(Protocol):
    def f(self) -> str: ...

import t
t_p: P = t  # expect an error here, it does not match P

# t.py
def f() -> int:
    return 6

I expect this to error because the f inside t returns int but the protocol specifies it should return str.

additionally, I think I should also be able to write something like this, but it currently errors for an unrelated reason (even when the module matches the protocol):

from typing import Protocol

class P(Protocol):
    def f(self) -> str: ...

t: P
import t  # error: Name "t" already defined on line 6

Your Environment

• Mypy version used: 0.981
• Mypy command-line flags: mypy main.py
• Mypy configuration options from mypy.ini (and other config files): n/a
• Python version used: 3.8.10

[AlexWaygood]

Cc. @ilevkivskyi

[ilevkivskyi]

This feature was not yet released. But actually the fact that there is no error means there (without this feature there should be always an error, even if it matches protocol) is an unrelated bug (and I even guess I know where).

[ilevkivskyi]

It however affects the feature as well, so I am going to look at it.

[asottile]

oh my bad, I thought that it was in the release, oops -- I will double-check on latest then

[hauntsaninja]

@ilevkivskyi I can repro on master. It looks like the issue is that types.ModuleType on typeshed has def __getattr__(self, name: str) -> Any: .... Unit tests work because of fixtures.

[AlexWaygood]

It looks like the issue is that types.ModuleType on typeshed has def __getattr__(self, name: str) -> Any: ...

That was one of my "innovations". Maybe it was a bad idea. Previous discussion in:

• Harmonise return type of builtins.__import__ and importlib.import_module typeshed#6302
• Remove ModuleType.__getattr__ typeshed#6357
• Don't use ModuleType.__getattr__ if we know module symbols #11597

[hauntsaninja]

Certainly wasn't an unreasonable thing to do. I think it's probably something we live with and work around in mypy.

If you're interested in my (somewhat off-topic) thoughts on that change:

• I agree that __import__ and import_module should have the same return type and that it should allow for dynamically typed use of the returned module.
• Between returning Any vs adding ModuleType.__getattr__ ... Any makes it more obvious to users that things are dynamically typed. ModuleType.__getattr__ lets you get better typing if you use dunders. I lean in favour of ModuleType.__getattr__, but don't feel strongly.
• Not sure how much it matters, but I think the primer is a little misleading on Harmonise return type of builtins.__import__ and importlib.import_module typeshed#6302. I spot checked two errors it removed for users, and I suspect that Don't use ModuleType.__getattr__ if we know module symbols #11597 would have re-introduced them. One was an implicit re-export and the other was a platform availability issue. Probably would look different re-running it now, with the mypy special-casing.

[ilevkivskyi]

TBH I don't like the __getattr__(). Instead there should be two types in typeshed: ModuleType and say _AnyModuleType inheriting from the former and having the __getattr__(). I see there is a PR, but the whole construction is quite fragile, since we have multiple code paths for the attribute access, and I could see how this may cause more bugs in the future.

(I will take a look at the PR still to have a back up plan)

[AlexWaygood]

TBH I don't like the __getattr__(). Instead there should be two types in typeshed: ModuleType and say _AnyModuleType inheriting from the former and having the __getattr__(). I see there is a PR, but the whole construction is quite fragile, since we have multiple code paths for the attribute access, and I could see how this may cause more bugs in the future.

That's an interesting idea. One downside I can see is that the result of reveal_type might be quite confusing for users, given that _AnyModuleType won't exist at runtime:

collections = __import__("collections")
reveal_type(collections)  # types._AnyModuleType ???

assert_type(__import__("collections"), types.ModuleType) would also presumably fail.

[ilevkivskyi]

I am going to merge #13778 to speed up things (and since I don't have strongly either way).