WIP: Fix most mypy issues #343

Avasam · 2025-03-12T16:29:54Z

No description provided.

distutils/sysconfig.py

distutils/tests/test_check.py

distutils/util.py

Avasam · 2025-03-12T16:36:36Z

distutils/cmd.py

+        # TODO: Raise a more descriptive error when cmd_obj is None ?
+        cmd_obj = cast(Command, self.distribution.get_command_obj(command, create))


Decision to be taken here.

My instinct - get_command_obj should raise the error and always return Command... unless there's a case where None is legitimately expected, in which case this should be wrapped in a helper to raise that error and set the expectation.

I just did a quick scan through the distutils and setuptools codebase and there's not a single call to get_finalized_command(..., create=False). Maybe the create flag is just cruft that can be eliminated.

I opened an exploratory PR deprecating (unused, but not removed, for backward compatibility) the create arg: #377

…le-mypy

Avasam · 2025-04-03T03:39:18Z

mypy:

distutils/_modified.py:95: error: Value of type variable "_SourcesT" of "newer_pairwise" cannot be "Iterable[str | bytes | PathLike[str] | PathLike[bytes]]" [type-var]

pyright:

distutils/_modified.py:95: Argument of type "(sources: Iterable[str | bytes | PathLike[str] | PathLike[bytes]], target: str | bytes | PathLike[str] | PathLike[bytes], missing: Literal['error', 'ignore', 'newer'] = "error") -> bool" cannot be assigned to parameter "newer" of type "(_SourcesT@newer_pairwise, _TargetsT@newer_pairwise) -> bool" in function "newer_pairwise"

@jaraco I can't help but feel this may be revealing an actual issue? Or at least an incorrect assumption.

newer_pairwise's newer param should be a function accepting a single item as the first parameter. But newer_group expects an iterable as the first parameter.
It's possible this was never raised as an issue because a str is iterable (so each character would be compared as a path, and found as inexistant)

…le-mypy

…into re-enable-mypy

Avasam · 2025-05-04T16:58:31Z

I split up all individual mypy code fixes into their own PRs for ease of review.
The newer_pairwise issue mentioned at #343 (comment) still needs to be looked at.

jaraco

Leaving an initial review. I've gone over most of this before I realized that portions have been extracted elsewhere.

jaraco · 2025-10-17T14:06:37Z

distutils/cmd.py

+        # TODO: Raise a more descriptive error when cmd_obj is None ?
+        cmd_obj = cast(Command, self.distribution.get_command_obj(command, create))


My instinct - get_command_obj should raise the error and always return Command... unless there's a case where None is legitimately expected, in which case this should be wrapped in a helper to raise that error and set the expectation.

jaraco · 2025-10-17T14:12:34Z

distutils/cmd.py

+        # TODO: Raise a more descriptive error when cmd_obj is None ?
+        cmd_obj = cast(Command, self.distribution.get_command_obj(command, create))


I just did a quick scan through the distutils and setuptools codebase and there's not a single call to get_finalized_command(..., create=False). Maybe the create flag is just cruft that can be eliminated.

jaraco · 2025-10-17T14:16:05Z

distutils/dir_util.py

+    if not dry_run:
+        try:
+            name.mkdir(mode=mode, parents=True, exist_ok=True)
+        except OSError as exc:
+            raise DistutilsFileError(f"could not create '{name}': {exc.args[-1]}")


I really don't love how this code gets more nested. What is it about mypy that's forcing different logic here?

This one could be ignored on the line itself without being unsafe.

It's an unused expression, both mypy and pyright warn here because of the lack of assignement, it "looks like" a possible mistake because the result of the expression is not used for anything. But it's just a conditional execution pattern.

It's definitely more of a lint than a type error.

https://github.com/microsoft/pylance-release/blob/main/docs/diagnostics/reportUnusedExpression.md

https://github.com/microsoft/pyright/blob/main/docs/configuration.md#user-content-reportunusedexpression

https://mypy.readthedocs.io/en/latest/error_code_list.html#code-func-returns-value

FWIW, #334 / #335 would remove the condition anyway.

jaraco · 2025-10-17T14:20:15Z

distutils/fancy_getopt.py

            raise DistutilsArgError(msg)

        for opt, val in opts:
+            value: int | str = val


This is gross. val and value are the same thing. Mypy is forcing some idiosyncratic code. There's got to be a better way (cast opts.__iter__ to the expected types?).

Actually, this may be resolved in mypy 1.18: https://mypy.readthedocs.io/en/stable/changelog.html#flexible-variable-definitions-update

Before 1.18 the flag wasn't stabilized and called allow-redefinition-new (I wasn't aware of its existence either)

Edit: No it doesn't help here because of the difference of scope

There's this related request python/mypy#15988
Which would allow val: str | int = val which is much safer than a cast.

As for casting, I could do

# Unsafely casting to avoid using a different variable # python/mypy#15988 could allow `val: str | int = val` for opt, val in cast("list[tuple[str, int | str]]", opts):

jaraco · 2025-10-17T14:24:01Z

distutils/sysconfig.py

+        assert isinstance(cc, str)
+        assert isinstance(cxx, str)
+        assert isinstance(cflags, str)
+        assert isinstance(ccshared, str)
+        assert isinstance(ldshared, str)
+        assert isinstance(ldcxxshared, str)
+        assert isinstance(ar_flags, str)
+        assert isinstance(shlib_suffix, str)


I'm not sure these assertions will always pass. There have been some bugs around these being undefined in unusual environments. Maybe it makes sense to catch these conditions early. Do we have good reason to believe that non-string values returned here would never be viable (would be breaking anyway)?

(same comment in the split PR, for tracking: #366 (comment))

jaraco · 2025-10-17T14:27:31Z

distutils/tests/unix_compat.py

+grp: ModuleType | None = None
+pwd: ModuleType | None = None
 try:
    import grp
    import pwd
 except ImportError:
-    grp = pwd = None
+    pass


I don't love the asymmetry of this syntax, but if this is the new Pythonic style, I guess it's acceptable. Is it?
It's starting to feel like we need a meta language to express Python imports. It would be so much nicer if we could simply express:

opt_import grp opt_import pwd

There's definitely much to improve when it comes to static type checkers and conditional imports... (especially with ImportError/ModuleNotFoundError)

Hopefully ty and/or pyrefly will come along with improved semantics and force existing checkers to improve there.

~~It's possible that this can be solved using mypy 1.18's allow-redefinition~~
Edit: No it doesn't help here.

jaraco · 2025-10-17T14:28:17Z

distutils/tests/unix_compat.py

 except ImportError:
-    grp = pwd = None
+    pass


I'd be tempted to use contextlib.suppress(ImportError) to eliminate the no-op block... except that requires a prior import, which may lead to problems. Also, I give it a 50% chance that mypy recognizes that approach.

Avasam · 2025-10-17T17:12:47Z

@jaraco Thanks, I took the time to go through your review even though most things here have been split-up in smaller self-contained PRs. (we can always link back to discussions here)

I think most of your comments/concern are extracted to #368, so you can probably ignore that one for now.

As for the failing CI, there's a related issue opened at setuptools: pypa/setuptools#5094

Re-enable mypy and fix most issues

ed56168