Re-enable ANN2 for setuptools #4709

Avasam · 2024-10-28T22:00:34Z

Summary of changes

Follow-up to #4504, works towards #2345
I didn't include long/complex overloads in this PR, that may come later

This has some overlap with #4711, but they can be merged in either order.

Pull Request Checklist

Changes have tests (existing type-checking and runtime tests)
News fragment added in newsfragments/. (no public facing changes)
(See documentation for details)

Avasam · 2024-10-28T22:00:56Z

setuptools/__init__.py

@@ -226,7 +226,7 @@ def reinitialize_command(
    ) -> _Command:
        cmd = _Command.reinitialize_command(self, command, reinit_subcommands)
        vars(cmd).update(kw)
-        return cmd
+        return cmd  # pyright: ignore[reportReturnType] # pypa/distutils#307


Ref.: pypa/distutils#307

Avasam · 2024-10-28T22:02:35Z

setuptools/command/build_py.py

-            outfile = str(Path(outfile).resolve())
-        return super().copy_file(
+            outfile = str(Path(outfile).resolve())  # type: ignore[assignment] # Re-assigning a str when outfile is StrPath is ok
+        return super().copy_file(  # pyright: ignore[reportReturnType] # pypa/distutils#309


Ref.: pypa/distutils#309

Avasam · 2024-10-28T22:09:35Z

setuptools/glob.py

-def has_magic(s):
+def has_magic(s: str | bytes) -> bool:
    if isinstance(s, bytes):
-        match = magic_check_bytes.search(s)
+        return magic_check_bytes.search(s) is not None
    else:
-        match = magic_check.search(s)
-    return match is not None
+        return magic_check.search(s) is not None


mypy will declare the first match as typed Math[bytes] | None, so the second assignement doesn't respect that typing, this is the same issue as python/mypy#10736 AFAICT

Either the variable needs to be explicitely typed as Match[bytes] | Match[str] | None, or we simply don't assign.

Avasam · 2024-10-28T22:11:21Z

setuptools/depends.py

-    def get_version(self, paths=None, default: str = "unknown"):
+    def get_version(
+        self, paths=None, default: str | int = "unknown"
+    ) -> str | int | None:


get_version, get_module_constant, and extract_constant can return Any (because of getattr on _imp.get_module and code.co_consts is typed as tuple[Any, ...]), but I'm not sure that's within intended usage ? Should that maybe be runtime validated ?

Yeah, this module is a bit annoying...

It looks very "vestigial", not very used in the setuptools code-base itself. But is exported as part of the API.

get_module_constant and extract_constant seem to be very likely returning something compatible with the return values of ast.parse_literal and possibly other objects that can be disassembled directly from Python bytecode. But I guess Any is the best thing we can do about that.

get_version is returning None | Literal["default"] | T if self.format is defined as Callable[..., T] or Any otherwise. But I don't think it is worth to add any form of validation here, we want to avoid breaking this API (potentially it is a good candidate for deprecation or moving to a different 3rd-party package).

Ok so essentially Any, but I changed it to at least give some hints.

Avasam · 2024-10-30T17:07:52Z

setuptools/command/bdist_egg.py

@@ -455,7 +455,7 @@ def make_zipfile(
    """
    import zipfile

-    mkpath(os.path.dirname(zip_filename), dry_run=dry_run)
+    mkpath(os.path.dirname(zip_filename), dry_run=dry_run)  # type: ignore[arg-type] # python/mypy#18075


Ref: python/mypy#18075

Avasam · 2024-10-31T14:06:56Z

setuptools/command/build_py.py

        # Overwrite base class to allow using links
        if link:
            infile = str(Path(infile).resolve())
-            outfile = str(Path(outfile).resolve())
-        return super().copy_file(
+            outfile = str(Path(outfile).resolve())  # type: ignore[assignment] # Re-assigning a str when outfile is StrPath is ok


This may or may not also be related to python/mypy#18075

setuptools/_path.py

abravalheri · 2024-10-31T14:32:37Z

setuptools/depends.py

-    def get_version(self, paths=None, default: str = "unknown"):
+    def get_version(
+        self, paths=None, default: str | int = "unknown"
+    ) -> str | int | None:


Yeah, this module is a bit annoying...

It looks very "vestigial", not very used in the setuptools code-base itself. But is exported as part of the API.

get_module_constant and extract_constant seem to be very likely returning something compatible with the return values of ast.parse_literal and possibly other objects that can be disassembled directly from Python bytecode. But I guess Any is the best thing we can do about that.

get_version is returning None | Literal["default"] | T if self.format is defined as Callable[..., T] or Any otherwise. But I don't think it is worth to add any form of validation here, we want to avoid breaking this API (potentially it is a good candidate for deprecation or moving to a different 3rd-party package).

abravalheri · 2024-10-31T18:05:53Z

setuptools/dist.py

@@ -52,6 +51,9 @@
 if TYPE_CHECKING:
    from typing_extensions import TypeAlias

+    from pkg_resources import Distribution as _pkg_resources_Distribution


It is good this is inside a if TYPE_CHECKING block. Thank you!

Ideally we want to avoid importing pkg_resources in runtime because of the performance hit.

It is only acceptable to "lazily" import pkg_resources on deprecated code paths (but never eagerly).

abravalheri · 2024-10-31T18:24:23Z

I don't know if it was a false negative but apparently after merging this in top of the previous merges we have a mypy error:

https://github.com/pypa/setuptools/actions/runs/11617414532/job/32352587471#step:9:71

Maybe there is a new version of mypy?

@Avasam, do you have any info?

Avasam · 2024-10-31T21:31:14Z

I don't know if it was a false negative but apparently after merging this in top of the previous merges we have a mypy error:

[…]

@Avasam, do you have any info?

I can take a look at it tomorrow. I'm omw to a Halloween party 🎃

If needed for other PRs you can suppress the error temporarily (or suppress for entire file if it only happens on one side of the 3.12 distutils boundary)

It's possible that two PRs individually didn't trigger new errors, but together do. And it wouldn't be caught by CI if the branches weren't updated (and there was no conflict forcing an update)

Avasam commented Oct 28, 2024

View reviewed changes

Avasam force-pushed the ANN2-autofixes branch 3 times, most recently from 63a7dd0 to abca822 Compare October 28, 2024 22:27

Avasam mentioned this pull request Oct 28, 2024

setuptools ANN201 autofixes for fully untyped functions #4711

Merged

2 tasks

Avasam force-pushed the ANN2-autofixes branch from abca822 to 7a1006c Compare October 30, 2024 17:07

Avasam commented Oct 30, 2024

View reviewed changes

Avasam mentioned this pull request Oct 30, 2024

Bump setuptools to 75.3.* python/typeshed#12928

Merged

Avasam force-pushed the ANN2-autofixes branch from 7a1006c to 3ee8f10 Compare October 31, 2024 13:57

Avasam commented Oct 31, 2024

View reviewed changes

abravalheri reviewed Oct 31, 2024

View reviewed changes

Re-enable ANN2 for setuptools

f160c70

Avasam force-pushed the ANN2-autofixes branch from 3ee8f10 to f160c70 Compare October 31, 2024 14:49

abravalheri reviewed Oct 31, 2024

View reviewed changes

abravalheri merged commit 39179c1 into pypa:main Oct 31, 2024
24 checks passed

Avasam deleted the ANN2-autofixes branch October 31, 2024 21:36

Avasam mentioned this pull request Nov 1, 2024

Fix mypy failure after multiple merges #4726

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-enable ANN2 for setuptools #4709

Re-enable ANN2 for setuptools #4709

Avasam commented Oct 28, 2024 •

edited

Loading

Avasam Oct 28, 2024

Avasam Oct 28, 2024

Avasam Oct 28, 2024

Avasam Oct 28, 2024 •

edited

Loading

abravalheri Oct 31, 2024

Avasam Oct 31, 2024

Avasam Oct 30, 2024

Avasam Oct 31, 2024

abravalheri Oct 31, 2024

abravalheri Oct 31, 2024

abravalheri commented Oct 31, 2024

Avasam commented Oct 31, 2024 •

edited

Loading

Re-enable ANN2 for setuptools #4709

Re-enable ANN2 for setuptools #4709

Conversation

Avasam commented Oct 28, 2024 • edited Loading

Summary of changes

Pull Request Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avasam Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abravalheri commented Oct 31, 2024

Avasam commented Oct 31, 2024 • edited Loading

Avasam commented Oct 28, 2024 •

edited

Loading

Avasam Oct 28, 2024 •

edited

Loading

Avasam commented Oct 31, 2024 •

edited

Loading