Skip to content

Import from collections.abc wherever possible #7635

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 6 commits into from
Apr 18, 2022
Merged

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Apr 16, 2022

We still can't do collections.abc imports in builtins (it still causes mypy crashes).

Also, there were lots of pytype crashes the last time I tried doing this (#6688). I'm not sure where pytype stands on this now.

@AlexWaygood
Copy link
Member Author

Script I used:
import ast
import re
import subprocess
import sys
from collections import defaultdict
from itertools import chain
from operator import attrgetter
from pathlib import Path
from typing import NamedTuple


class DeleteableImport(NamedTuple):
    old: str
    replacement: str


STUBS_SUPPORTING_PYTHON_2 = frozenset(
    path.parent for path in Path("stubs").rglob("METADATA.toml") if "python2 = true" in path.read_text().splitlines()
)


FORBIDDEN_BUILTIN_TYPING_IMPORTS = frozenset({"List", "FrozenSet", "Set", "Dict", "Tuple"})

# AbstractSet intentionally omitted from this list -- special-cased
IMPORTED_FROM_COLLECTIONS_ABC_NOT_TYPING = frozenset(
    {
        "ByteString",
        "Collection",
        "Container",
        "ItemsView",
        "KeysView",
        "Mapping",
        "MappingView",
        "MutableMapping",
        "MutableSequence",
        "MutableSet",
        "Sequence",
        "ValuesView",
        "Iterable",
        "Iterator",
        "Generator",
        "Hashable",
        "Reversible",
        "Sized",
        "Coroutine",
        "AsyncGenerator",
        "AsyncIterable",
        "AsyncIterator",
        "Awaitable",
        "Callable",
    }
)

# The values in the mapping are what these are called in `collections`
IMPORTED_FROM_COLLECTIONS_NOT_TYPING = {
    "Counter": "Counter",
    "Deque": "deque",
    "DefaultDict": "defaultdict",
    "OrderedDict": "OrderedDict",
    "ChainMap": "ChainMap",
}


def fix_bad_syntax(path: Path) -> None:
    if (
        "@python2" in path.parts
        or (Path("stubs/protobuf/google/protobuf") in path.parents and str(path).endswith("_pb2.pyi"))
        or any(directory in path.parents for directory in STUBS_SUPPORTING_PYTHON_2)
    ):
        return

    print(f"Attempting to convert {path} to new syntax.")

    with open(path) as f:
        stub = f.read()

    lines = stub.splitlines()
    tree = ast.parse(stub)
    imports_to_delete = {}
    imports_to_add = []
    classes_from_typing = set()
    import_linenos = set()

    class BadImportFinder(ast.NodeVisitor):
        def visit_Import(self, node: ast.Import):
            import_linenos.add(node.lineno)

        def visit_ImportFrom(self, node: ast.ImportFrom) -> None:
            import_linenos.add(node.lineno)

            if node.module != "typing":
                return

            bad_builtins_classes_in_this_import = set()
            bad_collections_classes_in_this_import = set()
            bad_collections_abc_classes_in_this_import = set()

            for cls in node.names:
                if cls.name in FORBIDDEN_BUILTIN_TYPING_IMPORTS:
                    bad_builtins_classes_in_this_import.add(cls)
                elif cls.name in IMPORTED_FROM_COLLECTIONS_NOT_TYPING:
                    bad_collections_classes_in_this_import.add(cls)
                elif cls.name in IMPORTED_FROM_COLLECTIONS_ABC_NOT_TYPING and path not in {
                    Path("stdlib/_collections_abc.pyi"),
                    Path("stdlib/builtins.pyi"),
                }:
                    bad_collections_abc_classes_in_this_import.add(cls)

            bad_classes_in_this_import = (
                bad_builtins_classes_in_this_import
                | bad_collections_classes_in_this_import
                | bad_collections_abc_classes_in_this_import
            )

            if not bad_classes_in_this_import:
                return

            classes_from_typing.update(cls.name for cls in bad_classes_in_this_import)
            new_import_list = [cls for cls in node.names if cls not in bad_classes_in_this_import]

            if not new_import_list:
                if path == Path("stdlib/csv.pyi"):
                    imports_to_delete[node.lineno - 1] = DeleteableImport(
                        old=ast.unparse(node), replacement="_DictReadMapping = dict"
                    )
                elif path != Path("stdlib/collections/__init__.pyi"):
                    imports_to_delete[node.lineno - 1] = DeleteableImport(old=ast.unparse(node), replacement="")
            elif node.lineno == node.end_lineno:
                imports_to_delete[node.lineno - 1] = DeleteableImport(
                    old=ast.unparse(node),
                    replacement=ast.unparse(ast.ImportFrom(module="typing", names=new_import_list, level=0)),
                )
            else:
                for cls in node.names:
                    if cls in bad_classes_in_this_import:
                        imports_to_delete[cls.lineno - 1] = DeleteableImport(
                            old=f"{cls.name}," if cls.asname is None else f"{cls.name} as {cls.asname},", replacement=""
                        )

            if bad_collections_classes_in_this_import:
                imports_to_add.append(
                    ast.unparse(
                        ast.ImportFrom(
                            module="collections",
                            names=[
                                ast.alias(name=IMPORTED_FROM_COLLECTIONS_NOT_TYPING[cls.name], asname=cls.asname)
                                for cls in sorted(bad_collections_classes_in_this_import, key=attrgetter("name"))
                            ],
                            level=0,
                        )
                    )
                )

            if bad_collections_abc_classes_in_this_import and path != Path("stdlib/collections/__init__.pyi"):
                imports_to_add.append(
                    ast.unparse(
                        ast.ImportFrom(
                            module="collections.abc",
                            names=sorted(bad_collections_abc_classes_in_this_import, key=attrgetter("name")),
                            level=0,
                        )
                    )
                )

    BadImportFinder().visit(tree)

    if not classes_from_typing:
        return

    for lineno, (old_syntax, new_syntax) in imports_to_delete.items():
        lines[lineno] = lines[lineno].replace(old_syntax, new_syntax)

    first_import_lineno = min(import_linenos) - 1

    for new_import in imports_to_add:
        lines[first_import_lineno:first_import_lineno] = [new_import]

    try:
        new_tree = ast.parse("\n".join(lines))
    except SyntaxError:
        print(path)
    else:
        lines_with_bad_syntax = defaultdict(list)

        class OldSyntaxFinder(ast.NodeVisitor):
            def visit_Subscript(self, node: ast.Subscript) -> None:
                if isinstance(node.value, ast.Name) and node.value.id in (
                    classes_from_typing & (FORBIDDEN_BUILTIN_TYPING_IMPORTS | {"Deque", "DefaultDict"})
                ):
                    lines_with_bad_syntax[node.lineno - 1].append(node.value.id)
                self.generic_visit(node)

        OldSyntaxFinder().visit(new_tree)

        for i, cls_list in lines_with_bad_syntax.items():
            for cls in cls_list:
                lines[i] = re.sub(rf"(\W){cls}\[", rf"\1{cls.lower()}[", lines[i])

    if Path("stubs\typed-ast\typed_ast") in path.parents:
        lines.remove("import typing")

    new_stub = "\n".join(lines) + "\n"

    if path == Path("stdlib/plistlib.pyi"):
        new_stub = new_stub.replace("_Dict", "dict")

    with open(path, "w") as f:
        f.write(new_stub)


def main() -> None:
    for path in chain(Path("stdlib").rglob("*.pyi"), Path("stubs").rglob("*.pyi")):
        fix_bad_syntax(path)

    print("\n\nSTARTING ISORT...\n\n")
    subprocess.run([sys.executable, "-m", "isort", "."])

    print("\n\nSTARTING BLACK...\n\n")
    subprocess.run([sys.executable, "-m", "black", "."])


if __name__ == "__main__":
    main()

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Apr 16, 2022

@rchen152, any idea what's causing these pytype errors? :)

Ran pytype with 2553 pyis, got 7 errors.
stdlib/_compression.pyi (3.9): AssertionError
stdlib/_socket.pyi (3.9): AssertionError
stdlib/_typeshed/__init__.pyi (3.9): AssertionError
stdlib/dataclasses.pyi (3.9): AssertionError
stdlib/email/headerregistry.pyi (3.9): AssertionError
stdlib/functools.pyi (3.9): AssertionError
stdlib/inspect.pyi (3.9): AssertionError

I've reverted all changes to these modules, but pytype is giving me AssertionErrors for these modules nonetheless.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@AlexWaygood AlexWaygood marked this pull request as ready for review April 16, 2022 12:37
Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

I briefly spot checked this and all looked good. This is good to go, once the pytype problems are sorted out. Maybe it would help to reduce the PR by doing third-party stubs first?

@AlexWaygood
Copy link
Member Author

Maybe it would help to reduce the PR by doing third-party stubs first?

Good idea!

@AlexWaygood
Copy link
Member Author

Third-party stubs have now been codemodded in #7637, so this PR now only touches the stdlib.

@github-actions

This comment has been minimized.

9 similar comments
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@JelleZijlstra
Copy link
Member

I did some debugging and the pytype errors are because array.array and types.MappingProxyType inherit from collections.abc classes. Can you change those back to inherit from typing.MutableSequence and typing.Mapping?

@JelleZijlstra
Copy link
Member

I don't know why pytype fails there though, I just put enough print statements in until I figured out where the errors were coming from.

JelleZijlstra added a commit that referenced this pull request Apr 17, 2022
This was the first step to figure out the issue in #7635. The stack trace was still pretty mysterious, but it helps more than just "AssertionError".
@AlexWaygood
Copy link
Member Author

I don't know why pytype fails there though, I just put enough print statements in until I figured out where the errors were coming from.

Thank you very much!! I'm useless when it comes to debugging pytype failures, since I'm on Windows, and pytype doesn't support Windows currently :(

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member Author

Can you change those back to inherit from typing.MutableSequence and typing.Mapping?

Looks like that did the trick :D

@AlexWaygood AlexWaygood marked this pull request as draft April 18, 2022 09:35
@AlexWaygood AlexWaygood marked this pull request as ready for review April 18, 2022 09:57
@AlexWaygood
Copy link
Member Author

Looks like there are three classes that we can't import from collections.abc in builtins.pyi without mypy crashing:

stdlib\codecs.pyi:56: error: INTERNAL ERROR -- Please try using mypy master on Github:
https://mypy.readthedocs.io/en/stable/common_issues.html#using-a-development-mypy-build
Please report a bug at https://github.com/python/mypy/issues
version: 0.942
Traceback (most recent call last):
  File "C:\Users\alexw\coding\typeshed\tests\mypy_test.py", line 358, in <module>
    main()
  File "mypy\semanal.py", line 5220, in accept
  File "mypy\nodes.py", line 1155, in accept
  File "mypy\semanal.py", line 2090, in visit_assignment_stmt
  File "mypy\semanal.py", line 2522, in process_type_annotation
  File "mypy\semanal.py", line 5318, in anal_type
  File "mypy\types.py", line 685, in accept
  File "mypy\typeanal.py", line 167, in visit_unbound_type
  File "mypy\typeanal.py", line 235, in visit_unbound_type_nonoptional
  File "mypy\typeanal.py", line 354, in try_analyze_special_unbound_type
  File "mypy\typeanal.py", line 853, in analyze_literal_type
  File "mypy\typeanal.py", line 913, in analyze_literal_param
  File "mypy\typeanal.py", line 1064, in named_type_with_normalized_str
  File "mypy\typeanal.py", line 1071, in named_type
AssertionError:
stdlib\codecs.pyi:56: : note: use --pdb to drop into pdb

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@AlexWaygood
Copy link
Member Author

Does anybody want to do another spot check, or shall I merge? :)

@srittau srittau merged commit 97a74bc into python:master Apr 18, 2022
@AlexWaygood AlexWaygood deleted the 585 branch April 18, 2022 10:52
AlexWaygood added a commit to PyCQA/flake8-pyi that referenced this pull request Apr 18, 2022
A regression test for python/typeshed#7635

Refs #46 (but doesn't quite close it).

Changes made:
* Expand Y027 to cover all objects in typing that are aliases to objects in collections.abc, except for AbstractSet
* Add new Y038 error code to forbid import AbstractSet from typing
* Change the error message in Y023 to make it clear that imports from collections.abc are now preferred to imports from typing.
* Refactor Y023 logic in pyi.py so as to quell flake8 from complaining that _check_import_or_attribute was getting too complex.
* Some small refactorings of test files to make them compatible with the new checks.
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.

3 participants