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

stubtest: Detect abstract properties mismatches #13647

Merged
merged 4 commits into from
Sep 14, 2022

Conversation

sobolevn
Copy link
Member

Closes #13646

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

It would be good to also add tests for abstract read-write properties, as well as abstract read-only properties. Read-write properties have a different internal representation to read-only properties -- read-only properties are nodes.FuncDef objects, but read-write properties are nodes.OverloadedFuncDef objects

mypy/stubtest.py Outdated Show resolved Hide resolved
mypy/stubtest.py Outdated Show resolved Hide resolved
mypy/stubtest.py Outdated Show resolved Hide resolved
@AlexWaygood
Copy link
Member

I just tried to see what impact this would have on typeshed, but this got in the way:

@sobolevn
Copy link
Member Author

@AlexWaygood do you have time to try again? :)

@AlexWaygood
Copy link
Member

I will!

@AlexWaygood
Copy link
Member

The diff looks pretty good! Here's the new hits on my machine (on Windows, using Python 3.10.6):

diff --git a/old.txt b/new.txt
index ac8937a9..3f60a744 100644
--- a/old.txt
+++ b/new.txt
@@ -1,9 +1,63 @@
+error: importlib.abc.Traversable.name is inconsistent, runtime property is abstract but stub is not
+Stub: at line 180 in file stdlib\importlib\abc.pyi
+def (self: importlib.abc.Traversable) -> builtins.str
+Runtime:
+<abc.abstractproperty object at 0x0000021F9CBBE440>
+
 error: locale names exported from the stub do not correspond to the names exported at runtime. This is probably due to an inaccurate `__all__` in the stub or things being missing from the stub.
 Stub: in file stdlib\locale.pyi
 Names exported in the stub but not at runtime: ['LC_MESSAGES']
 Runtime:
 Names exported at runtime but not in the stub: []

+error: typing.IO.closed is inconsistent, runtime property is abstract but stub is not
+Stub: at line 647 in file stdlib\typing.pyi
+def (self: typing.IO[AnyStr`1]) -> builtins.bool
+Runtime:
+<property object at 0x0000021F9C981E90>
+
+error: typing.IO.mode is inconsistent, runtime property is abstract but stub is not
+Stub: at line 641 in file stdlib\typing.pyi
+def (self: typing.IO[AnyStr`1]) -> builtins.str
+Runtime:
+<property object at 0x0000021F9C981DF0>
+
+error: typing.IO.name is inconsistent, runtime property is abstract but stub is not
+Stub: at line 643 in file stdlib\typing.pyi
+def (self: typing.IO[AnyStr`1]) -> builtins.str
+Runtime:
+<property object at 0x0000021F9C981E40>
+
+error: typing.TextIO.buffer is inconsistent, runtime property is abstract but stub is not
+Stub: at line 694 in file stdlib\typing.pyi
+def (self: typing.TextIO) -> typing.BinaryIO
+Runtime:
+<property object at 0x0000021F9C9822F0>
+
+error: typing.TextIO.encoding is inconsistent, runtime property is abstract but stub is not
+Stub: at line 696 in file stdlib\typing.pyi
+def (self: typing.TextIO) -> builtins.str
+Runtime:
+<property object at 0x0000021F9C982340>
+
+error: typing.TextIO.errors is inconsistent, runtime property is abstract but stub is not
+Stub: at line 698 in file stdlib\typing.pyi
+def (self: typing.TextIO) -> Union[builtins.str, None]
+Runtime:
+<property object at 0x0000021F9C982480>
+
+error: typing.TextIO.line_buffering is inconsistent, runtime property is abstract but stub is not
+Stub: at line 700 in file stdlib\typing.pyi
+def (self: typing.TextIO) -> builtins.int
+Runtime:
+<property object at 0x0000021F9C9824D0>
+
+error: typing.TextIO.newlines is inconsistent, runtime property is abstract but stub is not
+Stub: at line 702 in file stdlib\typing.pyi
+def (self: typing.TextIO) -> Any
+Runtime:
+<property object at 0x0000021F9C982520>
+
 note: unused allowlist entry sqlite3.dbapi2.Binary.__contains__
 note: unused allowlist entry sqlite3.Binary.__contains__
 note: unused allowlist entry enum.Enum.name
@@ -33,4 +87,4 @@ note: unused allowlist entry typing.SupportsInt.__init__
 note: unused allowlist entry typing.SupportsRound.__init__
 note: unused allowlist entry builtins.WindowsError.characters_written
 note: unused allowlist entry winreg.error.characters_written
-Found 30 errors (checked 482 modules)
+Found 39 errors (checked 482 modules)

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM!

@sobolevn
Copy link
Member Author

Awesome! I will double check the output later tomorrow, send a PR to typeshed with new changes, and merge this :)

Thanks a lot for your time and help!

@sobolevn
Copy link
Member Author

I don't quite like this one:

+error: importlib.abc.Traversable.name is inconsistent, runtime property is abstract but stub is not
+Stub: at line 180 in file stdlib\importlib\abc.pyi
+def (self: importlib.abc.Traversable) -> builtins.str
+Runtime:
+<abc.abstractproperty object at 0x0000021F9CBBE440>

@AlexWaygood
Copy link
Member

AlexWaygood commented Sep 13, 2022

I don't quite like this one:

+error: importlib.abc.Traversable.name is inconsistent, runtime property is abstract but stub is not
+Stub: at line 180 in file stdlib\importlib\abc.pyi
+def (self: importlib.abc.Traversable) -> builtins.str
+Runtime:
+<abc.abstractproperty object at 0x0000021F9CBBE440>

Arguably that should be fixed in CPython, rather than typeshed or mypy. abc.abstractproperty isn't really supported by mypy, but it's been deprecated since Python 3.3.

@hauntsaninja
Copy link
Collaborator

Agreed on fixing in CPython. I made a PR, but no one reviewed it and it eventually got closed for being stale

@sobolevn
Copy link
Member Author

I've made a new one!
I am sometimes a bit sad of how many good PRs are becoming stale due to the fact that there's not enough time to review them 😞

@AlexWaygood
Copy link
Member

The diff looks pretty good! Here's the new hits on my machine (on Windows, using Python 3.10.6):

Here's a slightly hacky script to produce these diffs, btw:

import subprocess
import sys
import tempfile
from pathlib import Path
from typing import Any


def run(*args: str, check: bool = True, **kwargs: Any) -> str:
    result = subprocess.run(list(args), check=check, capture_output=True, text=True, **kwargs)
    return result.stdout or ""


def run_stubtest(*, typeshed_dir: str) -> str:
    """This function is basically copied from typeshed's stubtest_stdlib.py."""

    allowlist_dir = Path(typeshed_dir, "tests", "stubtest_allowlists")
    version_allowlist = f"py{sys.version_info.major}{sys.version_info.minor}.txt"
    platform_allowlist = f"{sys.platform}.txt"
    combined_allowlist = f"{sys.platform}-py{sys.version_info.major}{sys.version_info.minor}.txt"

    cmd = [
        sys.executable,
        "-m",
        "mypy.stubtest",
        "--check-typeshed",
        "--custom-typeshed-dir",
        typeshed_dir,
        "--allowlist",
        str(allowlist_dir / "py3_common.txt"),
        "--allowlist",
        str(allowlist_dir / version_allowlist),
    ]
    if (allowlist_dir / platform_allowlist).exists():
        cmd += ["--allowlist", str(allowlist_dir / platform_allowlist)]
    if (allowlist_dir / combined_allowlist).exists():
        cmd += ["--allowlist", str(allowlist_dir / combined_allowlist)]
    if sys.version_info < (3, 10):
        # As discussed in https://github.com/python/typeshed/issues/3693, we only aim for
        # positional-only arg accuracy for the latest Python version.
        cmd.append("--ignore-positional-only")
    return run(*cmd, check=False)


def checkout_and_sync_with_master() -> None:
    run("git", "checkout", "master")
    run("git", "pull", "upstream", "master")
    run("git", "push")


def get_stubtest_diff(*, pr_url: str, typeshed_dir: str) -> str:
    print("Checking out and syncing mypy master...")
    checkout_and_sync_with_master()
    print("Running stubtest using mypy master...")
    old_results = run_stubtest(typeshed_dir=typeshed_dir)
    print("Checking out and syncing the PR...")
    run("gh", "pr", "checkout", pr_url)
    print("Running stubtest using the PR...")
    new_results = run_stubtest(typeshed_dir=typeshed_dir)
    print("Getting the diff...")
    with tempfile.TemporaryDirectory() as td:
        old, new = Path(td, "old.txt"), Path(td, "new.txt")
        for path, result in [(old, old_results), (new, new_results)]:
            path.write_text(result)
        diff_results = run("git", "diff", "--no-index", str(old), str(new), check=False)
    print("Cleaning up...")
    checkout_and_sync_with_master()
    return "\n".join(filter(None, diff_results.splitlines()))


def main() -> None:
    import argparse

    parser = argparse.ArgumentParser()
    parser.add_argument("pr", type=str, help="URL to a stubtest PR to check")
    parser.add_argument(
        "--typeshed-dir", type=str, required=True, help="Path to a local clone of typeshed"
    )
    parser.add_argument(
        "--output-file", type=str, help="File to write output to (writes to stdout if not set)"
    )
    args = parser.parse_args()

    diff = get_stubtest_diff(pr_url=args.pr, typeshed_dir=args.typeshed_dir)

    if args.output_file:
        with open(args.output_file, "w") as f:
            f.write(diff)
        print(f"Success! The diff has been written to {args.output_file!r}")
    else:
        print(diff)


if __name__ == "__main__":
    main()

@sobolevn sobolevn merged commit 7c14fee into python:master Sep 14, 2022
@sobolevn
Copy link
Member Author

Refs python/typeshed#8726

@sobolevn
Copy link
Member Author

Thanks everyone!

@JelleZijlstra
Copy link
Member

@sobolevn and @hauntsaninja Feel free to ping me if you need a CPython PR reviewed.

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: Abstract properties are not detected
4 participants