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.

Sign up for GitHub

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

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

Merged
merged 13 commits into from
Nov 6, 2024
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
ce78e5f
Allow `Sequence` for `setuptools.Extension` sources
agriyakhetarpal Nov 6, 2024
5e156e3
Add some test cases
agriyakhetarpal Nov 6, 2024
88f1ad0
Stick to `list[StrPath]` for `mypy_primer` checks
agriyakhetarpal Nov 6, 2024
13a04e1
Accept both `list[str]` and `list[StrPath]`
agriyakhetarpal Nov 6, 2024
7932a4e
Update `CCompiler.compile()` `sources` to allow `list[StrPath]`
agriyakhetarpal Nov 6, 2024
1a5a9fa
Merge branch 'main' into fix/loosen-setuptools-extension-sources
agriyakhetarpal Nov 6, 2024
47e149c
Add a few tests for `CCompiler().compile`
agriyakhetarpal Nov 6, 2024
97c456c
Code review: `distutils` can use `Sequence[StrPath]`
agriyakhetarpal Nov 6, 2024
477ae2a
Code review: accept all path-related args (for now)
agriyakhetarpal Nov 6, 2024
4a9141a
Experiment with more combined types
agriyakhetarpal Nov 6, 2024
f371836
Loosen `setuptools`'s sources arguments, update test
agriyakhetarpal Nov 6, 2024
b83d11e
Coherent hints for Extension class def and `__init__`
agriyakhetarpal Nov 6, 2024
ceaf190
Revert "Coherent hints for Extension class def and `__init__`"
agriyakhetarpal Nov 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions stubs/setuptools/@tests/test_cases/check_distutils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,21 @@
from __future__ import annotations

import distutils.command.sdist
from _typeshed import StrPath
from pathlib import Path
from typing import List

from setuptools._distutils.ccompiler import CCompiler

c = distutils.command.sdist.sdist

# Test CCompiler().compile with varied sources

compiler = CCompiler()
compiler.compile(sources=["file1.c", "file2.c"])

paths: List[StrPath] = [Path("file1.c"), Path("file2.c")]
compiler.compile(sources=paths)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


mixed: List[StrPath] = [Path("file1.c"), "file2.c"]
compiler.compile(sources=mixed)
8 changes: 8 additions & 0 deletions stubs/setuptools/@tests/test_cases/check_extension.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
from pathlib import Path

from setuptools import Extension

# Dummy extensions
ext1 = Extension(name="test1", sources=["file1.c", "file2.c"]) # list of str(s)
ext2 = Extension(name="test2", sources=[Path("file1.c"), Path("file2.c")]) # list of Path(s)
ext3 = Extension(name="test3", sources=[Path("file1.c"), "file2.c"]) # mixed str(s) and Path(s)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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]]]]"

2 changes: 1 addition & 1 deletion stubs/setuptools/setuptools/_distutils/ccompiler.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ class CCompiler:
def set_executables(self, **args: str) -> None: ...
def compile(
self,
sources: list[str],
sources: list[str] | list[StrPath],
Copy link
Collaborator

@Avasam Avasam Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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],

output_dir: str | None = None,
macros: list[_Macro] | None = None,
include_dirs: list[str] | None = None,
Expand Down
6 changes: 4 additions & 2 deletions stubs/setuptools/setuptools/_distutils/extension.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
from _typeshed import StrPath

class Extension:
name: str
sources: list[str]
sources: list[str] | list[StrPath]
include_dirs: list[str]
define_macros: list[tuple[str, str | None]]
undef_macros: list[str]
Expand All @@ -18,7 +20,7 @@ class Extension:
def __init__(
self,
name: str,
sources: list[str],
sources: list[str] | list[StrPath],
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

@Avasam Avasam Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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 )

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

include_dirs: list[str] | None = None,
define_macros: list[tuple[str, str | None]] | None = None,
undef_macros: list[str] | None = None,
Expand Down