Improve path-related type hints for setuptools.Extension() and distutils.CCompiler()

[agriyakhetarpal]

Description

This PR is coming via python/mypy#18107 (comment) post #12928. I think this should maintain compatibility since Sequence is covariant. In particular, the type annotation of the sources parameter in setuptools.Extension.__init__ has been from list[StrPath] to Sequence[StrPath] list[str] | list[StrPath]. setuptools.Extension shouldn't need a mutable sequence.

I also added a few test cases, but this is one of my first pull requests to typeshed and towards a typing-related project in general (plus I'm not well-versed with type theory) and I'm unsure if these changes are correct – please feel free to guide me towards making these changes in the correct manner, push changes to my branch directly, or close the PR in case it ends up not being needed. Thank you!

Closes python/mypy#18107

Additional context

xref: python/mypy#18107, pyodide/pyodide#5160

[agriyakhetarpal]

I see, that gets us back to the original issue.

[agriyakhetarpal]

I guess I need to change CCompiler.compile()'s stubs, too, but I don't know how invasive that change will be.

[Avasam]

list's invariance really bites us here. But I think going with generics should solve all issues (quick sanity check:

When appropriate, could you also keep the same methods in setuptools and stdlib's distutils in sync ?

• Extension class in setuptools
• CCompoiler.compile method in distutils

I'm working towards syncing setuptools._distutils no longer being necessary pypa/setuptools#4689

[Avasam]

If that annotation can be loosened to Sequence it definitely should be, if not it should take list[str] | list[StrPath]. PR to typeshed welcome!

sources argument of CCompiler.sources is used as an Iterable at https://github.com/pypa/distutils/blob/251797602080fa4c3a25016c6794179daceb6003/distutils/ccompiler.py#L962-L965 and its length is checked at https://github.com/pypa/distutils/blob/251797602080fa4c3a25016c6794179daceb6003/distutils/ccompiler.py#L352-L357
Sized iterables are mostly sequences:

Suggested change

	sources: list[str] | list[StrPath],
	sources: Sequence[StrPath],

Btw the exact same change can be applied to stdlib's distutils at

typeshed/stdlib/distutils/ccompiler.pyi

Lines 64 to 66 in 1ae7e61

	def compile(
	self,
	sources: list[str],

[Avasam]

There's a runtime check to explicitly str or os.PathLike at https://github.com/pypa/distutils/blob/251797602080fa4c3a25016c6794179daceb6003/distutils/extension.py#L109-L115, but the invariance of lists make this extremely annoying to type for all common use-case:

def foo(bar: list[str] | list[PathLike[str]] | list[Path] | list[StrPath]): ...

a: list[str]
b: list[PathLike[str]]
c: list[Path]
d: list[StrPath]
_ = foo(a)
_ = foo(b)
_ = foo(c)
_ = foo(d)

Suggested change

	sources: list[str] | list[StrPath],
	sources: list[str] | list[PathLike[str]] | list[Path] | list[StrPath],

I think a better long-term solution would be for pypa/distutils to stop explicitly checking for a list here, especially since the sources argument is already coerced into a list for Extension.sources

Then there's the question of Extension.sources being overly restrictive with a PathLike (which is a very barebones ABC !)
This could probably be solved with a Generic that defaults to StrPath. I'd imagine this class would have 5 generic params to cover all path-related arguments.

[agriyakhetarpal]

Yes, if Extension.sources retrieves a list anyway, requiring a list in the input seems like it's just an unnecessary restriction. The long-term solution with generics is interesting and makes (somewhat) sense to me, though – would you like to see me try a follow-up PR to pypa/distutils with that approach sometime soon?

[Avasam]

I don't think any change to pypa/distutils is necessary to make this class generic in stubs.

Whilst it's currently not pretty, I'd be fine doing it as a follow-up and keeping this PR as-is to restore functionality to projects that were relying on the previous annotations.

I'm curious as to what other maintainers think though. Is the current state of this PR acceptable as a stop-gap with a follow-up? Or should we make it generic now? Other thoughts? (CCing @hauntsaninja from conversation in python/mypy#18107 )

[JelleZijlstra]

I'd personally prefer to avoid the generic. Generics are confusing for users who use the class in a type annotation, because they need to figure out what the generic parameter refers to. That's easy for something where the generic nature is really inherent to the class (like with list or other collections), but for this class, the generic parameter probably doesn't make a huge difference to most users. It may be better to go with a more general type like Sequence to improve usability.

[JelleZijlstra]

Also I wrote this before I saw @Avasam's comment above. I think the current state of this PR is OK; the one thing that gives me pause is the list[PathLike[str]] | list[str] ... annotation. Type checkers aren't terribly likely to infer list[PathLike[str]] as the type for a list that the user provides, so that type isn't very usable in practice. We could consider using Sequence instead, but then we'd get false negatives if a user passes a tuple or other sequence. Ideally we'd solve this in the runtime of setuptools so it accepts any Sequence and not just list, but in the meantime what the PR currently does seems like a good solution.

[Avasam]

I feel like this shows a current issue where you can't have a list of Path

[Avasam]

setuptools.Extensions.__init__ still has sources as list[StrPath] in your branch, this shows that passing inline lists, the type-checkers are able to infer all these as being list[StrPath]

Here's a small snippet to demonstrate:

from __future__ import annotations
from os import PathLike
from pathlib import Path


def loose(foo: list[str] | list[PathLike[str]] | list[Path] | list[StrPath]): ...
def str_or_strpath(foo: list[str] | list[StrPath]): ...
def restrictive(foo: list[StrPath]): ...


def _(a: list[str], b: list[PathLike[str]], c: list[Path], d: list[StrPath]) -> None:
    loose(a)
    loose(b)
    loose(c)
    loose(d)
    restrictive(a) # Argument 1 to "restrictive" has incompatible type "list[str]"; expected "list[Union[str, PathLike[str]]]"
    restrictive(b) # Argument 1 to "restrictive" has incompatible type "list[PathLike[str]]"; expected "list[Union[str, PathLike[str]]]"
    restrictive(c) # Argument 1 to "restrictive" has incompatible type "list[Path]"; expected "list[Union[str, PathLike[str]]]"
    restrictive(d)
    restrictive(["file1.c", "file2.c"])  # list of str(s)
    restrictive([Path("file1.c"), Path("file2.c")])  # list of Path(s)
    restrictive([Path("file1.c"), "file2.c"])  # mixed str(s) and Path(s)
    str_or_strpath(a)
    str_or_strpath(b) # Argument 1 to "str_or_strpath" has incompatible type "list[PathLike[str]]"; expected "Union[list[str], list[Union[str, PathLike[str]]]]"
    str_or_strpath(c) # Argument 1 to "str_or_strpath" has incompatible type "list[Path]"; expected "Union[list[str], list[Union[str, PathLike[str]]]]"
    str_or_strpath(d)
    str_or_strpath(["file1.c", "file2.c"])  # list of str(s)
    str_or_strpath([Path("file1.c"), Path("file2.c")])  # list of Path(s) # Argument 1 to "str_or_strpath" has incompatible type "list[Path]"; expected "Union[list[str], list[Union[str, PathLike[str]]]]"
    str_or_strpath([Path("file1.c"), "file2.c"])  # mixed str(s) and Path(s) # Argument 1 to "str_or_strpath" has incompatible type "list[object]"; expected "Union[list[str], list[Union[str, PathLike[str]]]]"

[agriyakhetarpal]

Please let me know if there's anything more I can or need to address here. Thanks for the review so far!

[agriyakhetarpal]

Question: should we just use Sequence[StrPath] like we tried before to make CPython happy?

[github-actions]

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

[Avasam]

Question: should we just use Sequence[StrPath] like we tried before to make CPython happy?

For the method argument? I think Jelle's answer sums it up well:

We could consider using Sequence instead, but then we'd get false negatives if a user passes a tuple or other sequence. Ideally we'd solve this in the runtime of setuptools so it accepts any Sequence and not just list, but in the meantime what the PR currently does seems like a good solution.

So I'd leave it as-is, open a PR at https://github.com/pypa/distutils to change the runtime check to any Iterable (non-str, the "not str" check is necessary because a str is a valid Iterable[str] and Sequence[str], see python/mypy#11001), then update it back in typeshed.

[agriyakhetarpal]

Thanks for the feedback. I opened a PR in pypa/distutils here: pypa/distutils#311 which allows this behaviour.

[Avasam]

Thanks for your contribution!

This fixes an issue for mypy itself and Jelle seems to approve of the change as a stop-gap measure until the situation is improved upstream: pypa/distutils#311

in the meantime what the PR currently does seems like a good solution.

So I'll merge and if any other issue come up feel free to open a follow-up PR 😃