Skip to content

stubtest: Reduce false-positive errors on runtime type aliases #13116

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

Merged
merged 20 commits into from
Jul 28, 2022

Conversation

AlexWaygood
Copy link
Member

Description

Stubtest doesn't do very well at dealing with type aliases that exist at runtime. This PR fixes that.

Fixes #13114. Fixes #12821.

Test Plan

Several tests added.

@hauntsaninja hauntsaninja self-assigned this Jul 13, 2022
@AlexWaygood
Copy link
Member Author

This PR depends on #13101, since it uses typing_extensions.get_origin, which only works on 3.7+.

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks pretty good. I made a couple minor changes.

One quick thing: maybe it's worth checking full names? It does seem like the most common issue here would be cases where the stub has the name right but the qualified name wrong. We could do a quick {"typing": "collections.abc"}.get(mod_name, mod_name) to normalise.

@AlexWaygood
Copy link
Member Author

Thanks, this looks pretty good. I made a couple minor changes.

Thanks for the cleanups — they all look like changes for the better 😊

One quick thing: maybe it's worth checking full names? It does seem like the most common issue here would be cases where the stub has the name right but the qualified name wrong. We could do a quick {"typing": "collections.abc"}.get(mod_name, mod_name) to normalise.

Yeah, if it's just typing/collections.abc, it's not much of a problem. But what about e.g. StringIO, which is in io.pyi in typeshed, but has _io as its __module__ at runtime. And what if somebody's using a Python implementation where the stdlib io module imports StringIO from _pyio instead of _io? We'd want an object by the runtime name of _io.StringIO or _pyio.StringIO to be compatible with typeshed's io.StringIO. This principle could apply to an arbitrary number of hypothetical third-party modules, as well as to all the modules in the stdlib with C-accelerator modules.

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. I remember running into some of this stuff when I tried to get stubtest to check MRO. I had something that had a couple heuristics and worked okay, but it was icky enough that I never merged it.

That said, all hope is not lost. We could try the fullname match (adjusting for typing / collections.abc). And if it's not a match, we can fallback to the current behaviour of the structural check yield from verify(stub_target.type, runtime, object_path).

It's a nice optimisation over master, avoids false positives from typing constructs, avoids duplicate errors in the common case, avoids false positives for cases where the runtime itself gets aliased (class _A: ...; A = _A; A.__name__ = _A), keeps true positives for bad fully qualified aliases.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Jul 18, 2022

Here's the output if I run typeshed's stubtest_stdlib.py on my Windows PC, on Python 3.10, with this patch checked out:

+note: unused allowlist entry sqlite3.dbapi2.Binary.__contains__
+note: unused allowlist entry sqlite3.Binary.__contains__
+note: unused allowlist entry _socket.error.characters_written
+note: unused allowlist entry builtins.EnvironmentError.characters_written
+note: unused allowlist entry builtins.IOError.characters_written
+note: unused allowlist entry dbm.dumb.error.characters_written
+note: unused allowlist entry os.error.characters_written
+note: unused allowlist entry select.error.characters_written
+note: unused allowlist entry socket.error.characters_written
+note: unused allowlist entry builtins.WindowsError.characters_written
+note: unused allowlist entry winreg.error.characters_written

This is good. _socket.error, builtins.EnvironmentError, builtins.IOError, dbm.dumb.error, os.error, select.error, socket.error, builtins.WindowsError and winreg.error are all aliases to builtins.OSError. sqlite3.dbapi2.Binary and sqlite3.Binary are both aliases to builtins.memoryview.

There's no need for stubtest to emit duplicate errors for all of these aliases, given that it already emits an error for OSError.characters_written and memoryview.__contains__. With this patch, stubtest is able to detect that the fullnames are the same, and can therefore pass on emitting the duplicate errors.

@AlexWaygood AlexWaygood requested a review from hauntsaninja July 18, 2022 22:07
@AlexWaygood AlexWaygood requested a review from hauntsaninja July 28, 2022 00:12
Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay all green! One last nit, do we know for sure that runtime.__module__ + runtime.__qualname__ == stub.fullname? I'm realising I'm not actually sure what stub.fullname is for all the nested cases.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Jul 28, 2022

Yay all green! One last nit, do we know for sure that runtime.__module__ + runtime.__qualname__ == stub.fullname? I'm realising I'm not actually sure what stub.fullname is for all the nested cases.

With this patch checked out, I applied this diff to mypy/stubtest.py locally:

diff --git a/mypy/stubtest.py b/mypy/stubtest.py
index 96f6aa5af..1d65a6af1 100644
--- a/mypy/stubtest.py
+++ b/mypy/stubtest.py
@@ -1055,6 +1055,8 @@ def verify_typealias(
             runtime_name = runtime_origin.__qualname__
         except AttributeError:
             runtime_name = getattr(runtime_origin, "__name__", MISSING)
+        if runtime_origin.__qualname__ != runtime_origin.__name__:
+            print(f'{runtime_origin.__module__}.{runtime_origin.__qualname__}: {stub_origin.fullname}')
         if isinstance(runtime_name, str):
             runtime_module: object = getattr(runtime_origin, "__module__", MISSING)
             if isinstance(runtime_module, str):

And then ran pytest ./mypy/test/teststubtest.py. The printed output was:

test_module.Foo.Bar: test_module.Foo.Bar

So I think we're good!

@hauntsaninja hauntsaninja merged commit ff6fbe8 into python:master Jul 28, 2022
@hauntsaninja
Copy link
Collaborator

Hooray! Thank you

@AlexWaygood AlexWaygood deleted the stubtest-alias branch July 28, 2022 20:09
@AlexWaygood
Copy link
Member Author

Wooooo 😃🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stubtest: Problem with runtime type aliases stubtest false positive with runtime type alias for Callable
2 participants