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.

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

Add additional Unpack/kwargs conformance test cases #1918

Merged
merged 20 commits into from
Feb 6, 2025

Conversation

yangdanny97
Copy link
Contributor

These two examples from the typing spec don't appear to have corresponding cases in the conformance suite:

EXAMPLE 1

The reverse situation where the destination callable contains **kwargs: Unpack[TypedDict] and the source callable doesn’t contain **kwargs should be disallowed. This is because, we cannot be sure that additional keyword arguments are not being passed in when an instance of a subclass had been assigned to a variable with a base class type and then unpacked in the destination callable invocation

https://typing.readthedocs.io/en/latest/spec/callables.html#source-contains-kwargs-and-destination-doesn-t

EXAMPLE 2

Therefore, kwargs hinted with an unpacked TypedDict can only be passed to another function if the function to which unpacked kwargs are being passed to has **kwargs in its signature as well, because then additional keywords would not cause errors at runtime during function invocation. Otherwise, the type checker should generate an error.

https://typing.readthedocs.io/en/latest/spec/callables.html#passing-kwargs-inside-a-function-to-another-function

This PR adds 2 new test cases and updates the generated conformance output.

Pyre's output is super fishy here, it emits an error message on line 138, but it's not what I would expect. If we agree that these tests should be added, I'll dig into Pyre's behavior some more.

Example 2 I have some questions about. At a glance it seems reasonable, but I thought it was really weird to specify this requirement just for unpacked kwargs, but not typed dicts in other argument positions (which should still be susceptible to these issues due to structural typing). For example:

def func7(*, v1: int, v3: str, v2: str = "") -> None: ...

def func9(x: TD2, **kwargs: Unpack[TD2]) -> None:
    func7(**kwargs)  # not OK
    func7(**x)  # OK according to the spec, but not safe

Copy link
Collaborator

@erictraut erictraut left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Overall, it looks good. Just a few minor changes needed for consistency with existing tests and test results.

conformance/tests/callables_kwargs.py Show resolved Hide resolved
Copy link
Collaborator

@erictraut erictraut left a comment

Choose a reason for hiding this comment

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

LGTM

@erictraut
Copy link
Collaborator

@yangdanny97, this PR looks good to me. If you think it's complete, I can merge it.

@erictraut
Copy link
Collaborator

@yangdanny97, looking at this closer, I agree with you that Example 2 seems inconsistent. It doesn't make any sense that there should be a difference between the two cases in your code snippet above. I'm not sure what to do about this. Implementing this behavior inconsistently will require some ugly special-casing within pyright's logic — and presumably in the other type checkers as well.

Rather than add Example 2 to the conformance tests, perhaps we should amend the typing spec to remove this inconsistency. Thoughts? If that sounds good to you, please update the PR to remove Example 2, and then either you or I can start a discussion in the typing forum to propose changing the spec.

@yangdanny97
Copy link
Contributor Author

yangdanny97 commented Feb 6, 2025

I had a look at Pyre's implementation. I think the error emitted for the second example is a different bug, and Pyre doesn't implement the desired behavior of banning unpacked kwargs in this situation.

I'll update the results to mark Pyre as partial here, and then we should be good to go.

Edit: just saw your earlier response

@yangdanny97
Copy link
Contributor Author

@yangdanny97, looking at this closer, I agree with you that Example 2 seems inconsistent. It doesn't make any sense that there should be a difference between the two cases in your code snippet above. I'm not sure what to do about this. Implementing this behavior inconsistently will require some ugly special-casing within pyright's logic — and presumably in the other type checkers as well.

Rather than add Example 2 to the conformance tests, perhaps we should amend the typing spec to remove this inconsistency. Thoughts? If that sounds good to you, please update the PR to remove Example 2, and then either you or I can start a discussion in the typing forum to propose changing the spec.

Sure, I can update the PR to remove the second example until we sort out the spec. Ideally we have this check apply to all typed dict spreads, or none of them.

@yangdanny97
Copy link
Contributor Author

@erictraut erictraut merged commit 53daf3c into python:main Feb 6, 2025
4 checks passed
facebook-github-bot pushed a commit to facebook/pyre-check that referenced this pull request Feb 6, 2025
Summary:
Handle a case that gave a weird error in python/typing#1918

Keyword only parameters were always treated as not having defaults, which made them incompatible with notrequired typeddict items.

Reviewed By: rchen152

Differential Revision: D69222079

fbshipit-source-id: 5f4ac60d5bc3f7fa5da4b3709f0364f21f8d94aa
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants