- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 3k
          Support expand_type with ParamSpec.{args,kwargs}
          #20119
        
          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
base: master
Are you sure you want to change the base?
  
    Support expand_type with ParamSpec.{args,kwargs}
  
  #20119
              Conversation
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
| According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ | 
| reveal_type(SneakyPrefix(f9, 1, '', 0).args) # N: Revealed type is "tuple[builtins.str, builtins.int]" | ||
|  | ||
| reveal_type(Sneaky(f10, 1, '', '').args) # N: Revealed type is "Union[tuple[()], tuple[builtins.int], tuple[builtins.int, Unpack[builtins.tuple[builtins.str, ...]]]]" | ||
| reveal_type(SneakyPrefix(f10, 1, '', '').args) # N: Revealed type is "Union[tuple[()], tuple[Unpack[builtins.tuple[builtins.str, ...]]]]" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is correct, but ugly. Would making it "nice" justify complexity increase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
| # TODO: assert this is a trivial type, like Any, Never, or object. | ||
| return repl | ||
|  | ||
| @classmethod | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you make these classmethods? They don't use cls. Staticmethod or plain method seem less surprising.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nowhere near performance-critical code (we got a crash report on failing assert only a few years later after it was introduced), so it doesn't matter how optimal mypyc-compiled code is, so it's mostly a matter of preference. I prefer using classmethod because it makes later refactoring easier: it allows accessing other methods without extra diff lines and hardcoding the class name (and not regular methods to keep this code externally usable).
Fixes #19839.
Looks like it was relatively easy to do the right way, let me try! When splitting a callable/parameters into args and kwargs, we have the following options:
*args, required unless has a default. If we encounter such required arg, all previously collected optional args become required (this only happens due to faulty TVT expansion somewhere; probably I should look into that too)**kwargs, required unless has a default*argsas anUnpack(possibly normalized by tuple constructor)**kwargsand is only used if there are no kwargs with known names, because PEP 728 is not yet implemented, so we have to choose betweendictandTypedDict. (thoughts? Maybe it is better to preferdictwithunion(kwarg, *kwargs.values())as value type? Either way I do not consider this question important as PEP728 will be eventually implemented, and we'll haveextra_itemsfor ourTypedDicts)Applying these steps to every argument in order, we collect required and optional args and kwargs candidates. Now, the type of
**kwargsis aTypedDictif we know any keys,dict[str, KwargType]if we only have something like**kw: str, anddict[str, Never]if no kwargs were found.The type of
*argsis union of all prefixes ofoptional_argsconcatenated withrequired_args: all required args must be there, and optional args can only be passed in order. Since it is uncommon to have a function with more than 10-20 args, I think this union is a reasonable solution.