Polymorphic inference: basic support for variadic types #15879
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment has been minimized.
This comment has been minimized.
|
Diff from mypy_primer, showing the effect of this PR on open source code: vision (https://github.com/pytorch/vision): typechecking got 1.32x faster (49.3s -> 37.5s)
(Performance measurements are based on a single noisy sample)
|
jhance
approved these changes
Aug 16, 2023
|
OK, I am merging this now, I will open the follow-up issues I mentioned shortly. |
ilevkivskyi
added a commit
that referenced
this pull request
Aug 23, 2023
Fixes #13981 Fixes #15241 Fixes #15495 Fixes #15633 Fixes #15667 Fixes #15897 Fixes #15929 OK, I started following the plan outlined in #15879. In this PR I focused mostly on "kinematics". Here are some notes (in random order): * I decided to normalize `TupleType` and `Instance` items in `semanal_typeargs.py` (not in the type constructors, like for unions). It looks like a simpler way to normalize for now. After this, we can rely on the fact that only non-trivial (more below on what is trivial) variadic items in a type list is either `*Ts` or `*tuple[X, ...]`. A single top-level `TupleType` can appear in an unpack only as type of `*args`. * Callables turned out to be tricky. There is certain tight coupling between `FuncDef.type` and `FuncDef.arguments` that makes it hard to normalize prefix to be expressed as individual arguments _at definition_. I faced exactly the same problem when I implemented `**` unpacking for TypedDicts. So we have two choices: either handle prefixes everywhere, or use normalization helper in relevant code. I propose to go with the latter (it worked well for `**` unpacking). * I decided to switch `Unpack` to be disallowed by default in `typeanal.py`, only very specific potions are allowed now. Although this required plumbing `allow_unpack` all the way from `semanal.py`, conceptually it is simple. This is similar to how `ParamSpec` is handled. * This PR fixes all currently open crash issues (some intentionally, some accidentally) plus a bunch of TODOs I found in the tests (but not all). * I decided to simplify `TypeAliasExpr` (and made it simple reference to the `SymbolNode`, like e.g. `TypedDictExpr` and `NamedTupleExpr`). This is not strictly necessary for this PR, but it makes some parts of it a bit simpler, and I wanted to do it for long time. Here is a more detailed plan of what I am leaving for future PRs (in rough order of priority): * Close non-crash open issues (it looks like there are only three, and all seem to be straightforward) * Handle trivial items in `UnpackType` gracefully. These are `<nothing>` and `Any` (and also potentially `object`). They can appear e.g. after a user error. Currently they can cause assert crashes. (Not sure what is the best way to do this). * Go over current places where `Unpack` is handled, and verify both possible variadic items are handled. * Audit variadic `Instance` constrains and subtyping (the latter is probably OK, but the former may be broken). * Audit `Callable` and `Tuple` subtyping for variadic-related edge cases (constraints seem OK for these). * Figure out story about `map_instance_to_supertype()` (if no changes are needed, add tests for subclassing). * Clear most remaining TODOs. * Go once more over the large scale picture and check whether we have some important parts missing (or unhandled interactions between those). * Verify various "advanced" typing features work well with `TypeVarTuple`s (and add some support if missing but looks important). * Enable this feature by default. I hope to finish these in next few weeks.
davidhalter
reviewed
Feb 1, 2024
There was a problem hiding this comment.
@ilevkivskyi Thanks for this PR. Looking through some of the tests I found this small issue. I'm not sure if you still want to fix it, but I guess it tests relevant behavior. You might also just remove the line.
| g: Callable[[T], Foo[int]] | ||
| reveal_type(dec(g)) # N: Revealed type is "def (builtins.int) -> __main__.Foo[builtins.int]" | ||
| h: Callable[[Unpack[Us]], Foo[int]] | ||
| reveal_type(dec(g)) # N: Revealed type is "def (builtins.int) -> __main__.Foo[builtins.int]" |
There was a problem hiding this comment.
This line is probably wrong, because already check this two lines above, it should probably be dec(h).
There was a problem hiding this comment.
Good catch! Submitted a fix in #16853.
JelleZijlstra
added a commit
to JelleZijlstra/mypy
that referenced
this pull request
Feb 2, 2024
JelleZijlstra
added a commit
that referenced
this pull request
Feb 2, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the fifth PR in the series started by #15287, and a last one for the foreseeable future. This completes polymorphic inference sufficiently for extensive experimentation, and enabling polymorphic fallback by default.
Remaining items for which I am going to open follow-up issues:
--new-type-inferenceby default (should be done before everything else in this list).apply_poly()logic fromcheckexpr.pytoapplytype.py(this one depends on everything above).Callable(btw we already have a hacky support for capturing a generic function in an instance withParamSpec).Now some comments on the PR proper. First of all I decided to do some clean-up of
TypeVarTuplesupport, but added only strictly necessary parts of the cleanup here. Everything else will be in follow up PR(s). The polymorphic inference/solver/application is practically trivial here, so here is my view on how I see large-scale structure ofTypeVarTupleimplementation:applytype.py, so I deleted everything from there (as I did forParamSpec) and complementedvisit_callable_type()inexpandtype.py. Basically,applytype.pyshould have three simple steps: validate substitutions (upper bounds, values, argument kinds etc.); callexpand_type(); update callable type variables (currently we just reduce the number, but in future we may also add variables there, see TODO that I added).UnpackType) are insideInstances,TupleTypes, andCallableTypes. I like how there is an agreement that for callables there should never be a prefix, and instead prefix should be represented with regular positional arguments. I think that ideally we should enforce this with anassertinCallableTypeconstructor (similar to how I did this forParamSpec).expand_type()should be a priority (since it describes basic semantics ofTypeVarLikeTypes). I think I made good progress in this direction. IIUC the only valid substitution for*TsareTupleType.items,*tuple[X, ...],Any, and<nothing>, so it was not hard.TupleType(mostly forsemanal.py, see item below), plainTypeVarTupleType, and a homogeneoustupleinstances insideUnpackType. Supporting unions of those is not specified by the PEP and support will likely be quite tricky to implement. Also I propose to even eagerly expand type aliases to tuples (since there is no point in supporting recursive types likeA = Tuple[int, *A]).TupleTypes, there should be no things likeTuple[X1, *Tuple[X2, *Ts, Y2], Y1]etc after semantic analysis. (Similarly to how we always flattenParametersforParamSpec, and how we flatten nested unions inUnionTypeconstructor). Currently we do the flattening/normalization of tuples inexpand_type()etc.build_constraints_for_unpack()may be broken, at least when it was used for tuples and callables it did something wrong in few cases I tested (and there are other symptoms I mentioned in a TODO). I therefore re-implemented logic for callables/tuples using a separate dedicated helper. I will investigate more later.As I mentioned above I only implemented strictly minimal amount of the above plan to make my tests pass, but still wanted to write this out to see if there are any objections (or maybe I don't understand something). If there are no objections to this plan, I will continue it in separate PR(s). Btw, I like how with this plan we will have clear logical parallels between
TypeVarTupleimplementation and (recently updated)ParamSpecimplementation.