Skip to content

Basic support for ParamSpec type checking #11594

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

Merged
merged 30 commits into from
Nov 23, 2021
Merged

Basic support for ParamSpec type checking #11594

merged 30 commits into from
Nov 23, 2021

Conversation

JukkaL
Copy link
Collaborator

@JukkaL JukkaL commented Nov 22, 2021

Add support for type checking several ParamSpec use cases (PEP 612).

@hauntsaninja previously added support for semantic analysis of ParamSpec
definitions, and this builds on top that foundation.

The implementation has these main things going on:

  • ParamSpecType that is similar to TypeVarType but has three "flavors" that
    correspond to P, P.args and P.kwargs
  • CallableType represents Callable[P, T] if the arguments are
    (*args: P.args, **kwargs: P.kwargs) -- and more generally, there can also
    be arbitrary additional prefix arguments
  • Type variables of functions and classes can now be represented using
    ParamSpecType in addition to TypeVarType

There are still a bunch of TODOs. Some of these are important to address before the
release that includes this. I believe that this is good enough to merge and remaining
issues can be fixed in follow-up PRs.

Notable missing features include these:

@JukkaL JukkaL requested a review from hauntsaninja November 22, 2021 15:15
@JukkaL JukkaL requested a review from ilevkivskyi November 22, 2021 15:15
@github-actions

This comment has been minimized.

arg_type = self.named_generic_type('builtins.tuple',
[arg_type])
arg_type = get_proper_type(arg_type)
if not isinstance(arg_type, ParamSpecType):
Copy link
Member

Choose a reason for hiding this comment

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

I think you should update the proper_plugin to not require this. ParamSpecType can never by a target of a type alias. (Note TypeVarType is already excluded).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A good idea! Done. Also removed various now-redundant get_proper_type calls.

param_spec = t.param_spec()
if param_spec is not None:
repl = get_proper_type(self.variables.get(param_spec.id))
if isinstance(repl, CallableType):
Copy link
Member

Choose a reason for hiding this comment

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

TBH it is hard to follow the logic here: could you please add a comment on why do we need to have some non-trivial logic for callable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a comment.

mypy/typeanal.py Outdated
finally:
if nested:
self.nesting_level -= 1
# Use type(...) to ignore proper/non-proper type distinction.
if (not allow_param_spec
and type(analyzed) is ParamSpecType
Copy link
Member

Choose a reason for hiding this comment

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

Is this really needed? You can update the plugin instead as I suggested above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Replaced with regular isinstance.

def __eq__(self, other: object) -> bool:
if not isinstance(other, ParamSpecType):
return NotImplemented
return self.id == other.id and self.flavor == other.flavor
Copy link
Member

Choose a reason for hiding this comment

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

Why upper_bound is missing here and in various other places like is_same_type()? Add a comment about this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not user-configurable and always depends on flavor (actually currently it's always object). Added some comments.

mypy/fixup.py Outdated
@@ -247,6 +248,9 @@ def visit_type_var(self, tvt: TypeVarType) -> None:
if tvt.upper_bound is not None:
tvt.upper_bound.accept(self)

def visit_param_spec(self, p: ParamSpecType) -> None:
pass # Nothing to descend into.
Copy link
Member

Choose a reason for hiding this comment

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

What about .upper_bound? Also why it is ignored in similar fine-grained related logic?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I hadn't updated some places after I added upper_bound. It's now processed here and fine-grained logic.

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.

ParamSpec! 🎉 🎏

@@ -63,6 +63,11 @@ def visit_deleted_type(self, t: DeletedType) -> T:
def visit_type_var(self, t: TypeVarType) -> T:
pass

@abstractmethod
def visit_param_spec(self, t: ParamSpecType) -> T:
assert False
Copy link
Member

Choose a reason for hiding this comment

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

Looks like an implementation artifact. Isn't it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I used this for testing. Removed the assert.

@@ -446,13 +447,63 @@ def deserialize(cls, data: JsonDict) -> 'TypeVarType':
)


class ParamSpecFlavor:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe Enum? Like ArgKind: https://github.com/python/mypy/blob/master/mypy/nodes.py#L1636

Sorry, Enums are really my thing 🙂

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tend to avoid enums since mypyc only partially supports them, so it's possible to accidentally write code that it's pretty slow.

def m(self, *args: P.args, **kwargs: P.kwargs) -> int:
return 1

c: C[Any]
Copy link
Member

Choose a reason for hiding this comment

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

Would any other non-Any type arguments work? I haven't see any examples of it in tests 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is not implemented. I've mentioned it in the commit summary as follow-up work.

@JukkaL
Copy link
Collaborator Author

JukkaL commented Nov 23, 2021

Thanks for the feedback! I think that I've addressed all the comments (either by updating the PR or by leaving a reply).

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

spark (https://github.com/apache/spark.git)
+ python/pyspark/shell.py:49: error: Cannot infer type of lambda  [misc]

tornado (https://github.com/tornadoweb/tornado.git)
+ tornado/util.py:55: error: Cannot infer type of lambda

@JukkaL
Copy link
Collaborator Author

JukkaL commented Nov 23, 2021

This is still broken:

import atexit

def f() -> None: pass
atexit.register(lambda: f())  # Cannot infer type of lambda

I'll fix this in a follow-up PR.

@JukkaL JukkaL merged commit ce3975d into master Nov 23, 2021
@JukkaL JukkaL deleted the paramspec branch November 23, 2021 13:14
JukkaL added a commit that referenced this pull request Nov 23, 2021
If ParamSpec is in the context of a lambda, treat it similar
to `Callable[..., Any]`. This allows us to infer at least argument
counts and kinds. Types can't be inferred since that would require
"backwards" type inference, which we don't support.

Follow-up to #11594.
JukkaL added a commit that referenced this pull request Nov 23, 2021
If ParamSpec is in the context of a lambda, treat it similar
to `Callable[..., Any]`. This allows us to infer at least argument
counts and kinds. Types can't be inferred since that would require
"backwards" type inference, which we don't support.

Follow-up to #11594.
JukkaL added a commit that referenced this pull request Nov 23, 2021
Check that the correct ParamSpec and flavor are used in
`*args` and `**kwargs`.

Follow-up to #11594.
JukkaL added a commit that referenced this pull request Nov 24, 2021
Check that the correct ParamSpec and flavor are used in
`*args` and `**kwargs`.

Follow-up to #11594.
tushar-deepsource pushed a commit to DeepSourceCorp/mypy that referenced this pull request Jan 20, 2022
Add support for type checking several ParamSpec use cases (PEP 612).

@hauntsaninja previously added support for semantic analysis of ParamSpec
definitions, and this builds on top that foundation.

The implementation has these main things going on:
 * `ParamSpecType` that is similar to `TypeVarType` but has three "flavors" that
   correspond to `P`, `P.args` and `P.kwargs`
 * `CallableType` represents `Callable[P, T]` if the arguments are 
   (`*args: P.args`, `**kwargs: P.kwargs`) -- and more generally, there can also
   be arbitrary additional prefix arguments
 * Type variables of functions and classes can now be represented using
   `ParamSpecType` in addition to `TypeVarType`

There are still a bunch of TODOs. Some of these are important to address before the
release that includes this. I believe that this is good enough to merge and remaining 
issues can be fixed in follow-up PRs.

Notable missing features include these:
 * `Concatenate`
 * Specifying the value of ParamSpec explicitly (e.g. `Z[[int, str, bool]]`)
 * Various validity checks -- currently only some errors are caught
 * Special case of decorating a method (python/typeshed#6347)
 * `atexit.register(lambda: ...)` generates an error
tushar-deepsource pushed a commit to DeepSourceCorp/mypy that referenced this pull request Jan 20, 2022
If ParamSpec is in the context of a lambda, treat it similar
to `Callable[..., Any]`. This allows us to infer at least argument
counts and kinds. Types can't be inferred since that would require
"backwards" type inference, which we don't support.

Follow-up to python#11594.
tushar-deepsource pushed a commit to DeepSourceCorp/mypy that referenced this pull request Jan 20, 2022
Check that the correct ParamSpec and flavor are used in
`*args` and `**kwargs`.

Follow-up to python#11594.
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