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

Allow using TypedDict for more precise typing of **kwds #13471

Merged
merged 9 commits into from
Aug 22, 2022

Conversation

ilevkivskyi
Copy link
Member

Fixes #4441

This uses a different approach than the initial attempt, but I re-used some of the test cases from the first PR by @franekmagiera. The initial idea was to eagerly expand the signature of the function during semantic analysis, but it didn't work well with fine-grained mode and also mypy in general relies on function definition and its type being consistent (and rewriting FuncDef sounds too sketchy). So instead I add a boolean flag to CallableType to indicate whether type of **kwargs is each item type or the "packed" type.

I also add few helpers and safety net in form of a NewType(), but in general I am surprised how few places needed normalizing the signatures (because most relevant code paths go through check_callable_call() and/or is_callable_compatible()). Currently Unpack[...] is hidden behind --enable-incomplete-features, so this will be too, but IMO this part is 99% complete (you can see even some more exotic use cases like generic TypedDicts and callback protocols in test cases).

cc @JukkaL so that you are aware of this when you are back from vacation.

@github-actions

This comment has been minimized.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thank you so much! I was waiting for this feature for such a long time!


def copy_modified(
self,
self: CT,
Copy link
Member

Choose a reason for hiding this comment

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

Good candidate for Self type in a future refactoring.

def g(**kwargs: Unpack[Person]) -> int: ...

reveal_type(g) # N: Revealed type is "def (*, name: builtins.str, age: builtins.int) -> builtins.list[builtins.int]"
[builtins fixtures/dict.pyi]
Copy link
Member

@sobolevn sobolevn Aug 22, 2022

Choose a reason for hiding this comment

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

There are some other cases that I think are worth covering:

  1. When TypedDict has kwargs argument (because it is special cased in the implementation)
  2. When TypedDict has invalid chars for aguments, like T = TypedDict("T", {"@": int})
  3. When TypedDict is empty, no keys
  4. When TypedDict has a key that overrides other arguments like T = TypedDict("T", {"a": int}) and def some(a: str, **kwargs: Unpack[T]): ... (and similar cases like def some(a: str, /, **kwargs: Unpack[T]) and def some(*, a: str, **kwargs: Unpack[T]))
  5. We can also check overrides with compatible / incompatible typed dics

Copy link
Member Author

Choose a reason for hiding this comment

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

I added the tests (and also fixed error messages for incompatible overrides). Btw on non-identifier keys, Python allows them:

>>> def foo(**a):
...     print(a)
... 
>>> foo(**{"@": 1})
{'@': 1}

Note I already had a test for named argument overlap, so I added the same one for positional-only (which should be allowed).

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions
Copy link
Contributor

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

@ilevkivskyi
Copy link
Member Author

Does this PR look good now? @sobolevn @jhance

@ilevkivskyi ilevkivskyi merged commit 3973981 into python:master Aug 22, 2022
@ilevkivskyi ilevkivskyi deleted the unpack-kwargs branch August 22, 2022 23:44
jhance pushed a commit that referenced this pull request Sep 9, 2022
Fixes #4441

This uses a different approach than the initial attempt, but I re-used some of the test cases from the older PR. The initial idea was to eagerly expand the signature of the function during semantic analysis, but it didn't work well with fine-grained mode and also mypy in general relies on function definition and its type being consistent (and rewriting `FuncDef` sounds too sketchy). So instead I add a boolean flag to `CallableType` to indicate whether type of `**kwargs` is each item type or the "packed" type.

I also add few helpers and safety net in form of a `NewType()`, but in general I am surprised how few places needed normalizing the signatures (because most relevant code paths go through `check_callable_call()` and/or `is_callable_compatible()`). Currently `Unpack[...]` is hidden behind `--enable-incomplete-features`, so this will be too, but IMO this part is 99% complete (you can see even some more exotic use cases like generic TypedDicts and callback protocols in test cases).
JukkaL added a commit that referenced this pull request Dec 19, 2022
The implementation copied lots of callable types even when not using
the new feature, which was expensive. Now we only generate a copy if a
callable actually uses TypedDict types for **kwds.

This made self check 7-8% faster (when compiled with -O0).

The original implementation was in #13471.
JukkaL added a commit that referenced this pull request Dec 20, 2022
The implementation copied lots of callable types even when not using
the new feature, which was expensive. Now we only generate a copy if a
callable actually uses TypedDict types for **kwds.

This made self check 7-8% faster (when compiled with -O0).

The original implementation was in
#13471.
@shadycuz
Copy link

shadycuz commented Sep 7, 2023

@ilevkivskyi I cant seem to be able to turn this on in my config file.

I'm using TOML and I tried

enable_incomplete_features = "Unpack"
enable_incomplete_features = ["Unpack"]
enable_incomplete_features = true

I'm getting the error from a vscode mypy extention that doesn't allow me to mess with the cli arguments. I can not find any information about enable_incomplete_features in any of the mypy docs.

image

Update:
It appears that:

[tool.mypy]
enable_incomplete_feature = "Unpack"

Is the correct way to turn it on? Note its feature and not features. But then mypy daemon crashes with the following error:

...
  File "mypy/semanal.py", line 808, in visit_func_def
  File "mypy/semanal.py", line 837, in analyze_func_def
  File "mypy/typeanal.py", line 865, in visit_callable_type
  File "mypy/typeanal.py", line 1391, in anal_array
  File "mypy/typeanal.py", line 1400, in anal_type
  File "mypy/types.py", line 1792, in accept
  File "mypy/typeanal.py", line 865, in visit_callable_type
  File "mypy/typeanal.py", line 1391, in anal_array
  File "mypy/typeanal.py", line 1400, in anal_type
  File "mypy/types.py", line 933, in accept
  File "mypy/typeanal.py", line 845, in visit_unpack_type
NotImplementedError


[6] Error running mypy in /workspaces/nvcloud: the mypy daemon crashed. This is probably a bug in mypy itself, see Output panel for details. The daemon will be restarted automatically.

here is the code that is causing the issue:

class StackArgs(TypedDict):
    """The arguments that are passed to boto when creating or updating a stack.

    Args:
        StackName (str): The name of the stack to create, must be unique within the region.
        Capabilities (Sequence[CapabilityType]): A list of capabilities that the stack is granted.
        TemplateBody (Optional[str], optional): The Cloudformation template as a str. Defaults to None.
        TemplateURL (Optional[str], optional): A URL to the Cloudformation template. Defaults to None.
        Parameters (Optional[Sequence[ParameterTypeDef]], optional): The parameters to use when createing the stack.
        Defaults to None.
        Tags (Optional[Sequence[TagTypeDef]], optional): The tags to apply to the stack, which will get applied
        to all resources created by the stack. Defaults to None.
        EnableTerminationProtection (bool, optional): If enabled the stack is unable to be deleted until its removed.
        Defaults to False.
    """

    StackName: str
    TemplateBody: NotRequired[str]
    TemplateURL: NotRequired[str]
    Parameters: NotRequired[Sequence["ParameterTypeDef"]]
    DisableRollback: NotRequired[bool]
    RollbackConfiguration: NotRequired["RollbackConfigurationTypeDef"]
    TimeoutInMinutes: NotRequired[int]
    NotificationARNs: NotRequired[Sequence[str]]
    Capabilities: NotRequired[Sequence["CapabilityType"]]
    ResourceTypes: NotRequired[Sequence[str]]
    RoleARN: NotRequired[str]
    OnFailure: NotRequired["OnFailureType"]
    StackPolicyBody: NotRequired[str]
    StackPolicyURL: NotRequired[str]
    Tags: NotRequired[Sequence["TagTypeDef"]]
    ClientRequestToken: NotRequired[str]
    EnableTerminationProtection: NotRequired[bool]
    
class StackOperation(NamedTuple):
    name: str
    func: Callable[[Unpack[StackArgs]], "StackResource"]

I can turn this into an issue if someone wants.

@ilevkivskyi
Copy link
Member Author

I can turn this into an issue if someone wants.

Yes, ideally you can open a follow up issue. I want to handle this case since it is an obvious omission that leads to a crash.

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.

Allow using TypedDict for more precise typing of **kwds
3 participants