githubkit icon indicating copy to clipboard operation
githubkit copied to clipboard

Prevent `list` method from shadowing `list` in type annotations

Open sterliakov opened this issue 1 year ago • 5 comments

Currently some features do not support type checking with mypy due to invalid type hints in the library. For example, the following:

issues = gh.rest.issues.list_for_repo("python", "mypy", state="open", sort="created").parsed_data
for i in issues: ...

will produce mypy errors. A small repro to explain the problem:

class Foo:
    def list(self) -> None: ...
    
    def method(self) -> list[str]:  # E: Function "__main__.Foo.list" is not valid as a type  [valid-type]
        return [""]

Foo().method().append("")  # E: "list?[builtins.str]" has no attribute "append"  [attr-defined]

The most common solution for this problem is to use builtins.list in such cases, which I do in this PR. Unfortunately, the schema was recently updated, so there are unrelated diffs withs moved/added classes.

To prevent similar issues from appearing in future, I suggest to add a mypy (and probably pyright) check to the project CI. I will submit a PR to do so once this is merged (those changes are smaller but still overlap with codegen, because we need a few ignore comments until https://peps.python.org/pep-0747/ is approved and implemented). There are a few type errors to correct, but those are mostly simple. In particular, #176 introduced a bug: QueryParamTypes and HeaderTypes must stick to dict and list because upstream httpx declares so and explicitly checks with isinstance, so passing some other Mapping or Sequence will either result in a crash or produce unexpected results.

sterliakov avatar Jan 23 '25 20:01 sterliakov

For the builtin type shallowing problem, I need to do some check about this. This PR includes too much changes and i can not review on it. Could you please commit the necessary code only without re-generate the openapi code?

I suggest to add a mypy (and probably pyright) check to the project CI

Currently, githubkit is checked with pyright, excluding the generated code. I will add an pyright action first to check if there are other issues.

Mypy is not primary supported, it will produce many type inferencing errors in complex cases (such as githubkit's paginator).

QueryParamTypes and HeaderTypes must stick to dict and list because upstream httpx declares so and explicitly checks with isinstance

httpx defines the types here. I'm not sure which version of httpx you are using?

yanyongyu avatar Jan 24 '25 03:01 yanyongyu

Yes, sure! I'll update this tomorrow to keep (the only) non-autogen change in this PR.

Re mypy: actually after fixing autogen there were only ~10 non-strict mypy errors, all either trivial to fix or worth a type-ignore. I can push and show it after merging solution to this critical issue.

Re httpx: sorry, my bad, HeaderTypes are fine, but QueryParamTypes were changed incorrectly. httpx expects

QueryParamTypes = Union[
    "QueryParams",
    Mapping[str, Union[PrimitiveData, Sequence[PrimitiveData]]],
    List[Tuple[str, PrimitiveData]],
    Tuple[Tuple[str, PrimitiveData], ...],
    str,
    bytes,
]

either a list or a tuple of Tuple[str, PrimitiveData]. It does not allow other sequences, and here's why:

https://github.com/encode/httpx/blob/10b7295922741b91a15751029e6ad3e8e5efb9f3/httpx/_urls.py#L438

Replacing that with Sequence[Tuple[str, PrimitiveData]] is not compatible with upstream httpx.

sterliakov avatar Jan 24 '25 03:01 sterliakov

I pushed the isolated change; all other files are modified by autogen - please run it yourself in whichever way is convenient for you.

sterliakov avatar Jan 24 '25 04:01 sterliakov

I checked the example code in pyright, the shadow issue does not happen. I'm not sure why the behavior is different between pyright and mypy.

If i remove the from __future__ import annotations, pyright also complains about the shadow error. I think this may related to the postponed eval of types.

yanyongyu avatar Jan 24 '25 09:01 yanyongyu

Hm, thanks, that might indeed be a mypy issue. I filed https://github.com/python/mypy/issues/18525 and convert this to draft for now.

sterliakov avatar Jan 24 '25 18:01 sterliakov