Streamline some elements of variadic types support #15924
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.
|
This PR also fixes the just-opened #15929 |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
jhance
approved these changes
Aug 23, 2023
ilevkivskyi
added a commit
that referenced
this pull request
Sep 7, 2023
This PR closes the first part of support for `TypeVarTuple`: the "static" analysis of types (of course everything is static in mypy, but some parts are more static): `semanal`/`typeanal`, `expand_type()`, `map_instance_to_supertype()`, `erase_type()` (things that precede and/or form foundation for type inference and subtyping). This one was quite tricky, supporting unpacks of forward references required some thinking. What is included in this PR: * Moving argument count validation from `semanal_typeargs` to `typeanal`. In one of previous PRs I mentioned that `get_proper_type()` may be called during semantic analysis causing troubles if we have invalid aliases. So we need to move validation to early stage. For instances, this is not required, but I strongly prefer keeping instances and aliases similar. And ideally at some point we can combine the logic, since it gets more and more similar. At some point we may want to prohibit using `get_proper_type()` during semantic analysis, but I don't want to block `TypeVarTuple` support on this, since this may be a significant refactoring. * Fixing `map_instance_to_supertype()` and `erase_type()`. These two are straightforward, we either use `expand_type()` logic directly (by calling it), or following the same logic. * Few simplifications in `expandtype` and `typeops` following previous normalizations of representation, unless there is a flaw in my logic, removed branches should be all dead code. * Allow (only fixed) unpacks in argument lists for non-variadic types. They were prohibited for no good reason. * (Somewhat limited) support for forward references in unpacks. As I mentioned this one is tricky because of how forward references are represented. Usually they follow either a life cycle like: `Any` -> `<known type>`, or `<Any>` -> `<placeholder>` -> `<known type>` (second one is relatively rare and usually only appears for potentially recursive things like base classes or type alias targets). It looks like `<placeholder>` can never appear as a _valid_ unpack target, I don't have a proof for this, but I was not able to trigger this, so I am not handling it (possible downside is that there may be extra errors about invalid argument count for invalid unpack targets). If I am wrong and this can happen in some valid cases, we can add handling for unpacks of placeholders later. Currently, the handling for `Any` stage of forward references is following: if we detect it, we simply create a dummy valid alias or instance. This logic should work for the same reason having plain `Any` worked in the first place (and why all tests pass if we delete `visit_placeholder_type()`): because (almost) each time we analyze a type, it is either already complete, or we analyze it _from scratch_, i.e. we call `expr_to_unanalyzed_type()`, then `visit_unbound_type()` etc. We almost never store "partially analyzed" types (there are guards against incomplete references and placeholders in annotations), and when we do, it is done in a controlled way that guarantees a type will be re-analyzed again. Since this is such a tricky subject, I didn't add any complex logic to support more tricky use cases (like multiple forward references to fixed unpacks in single list). I propose that we release this, and then see what kind of bug reports we will get. * Additional validation for type arguments position to ensure that `TypeVarTuple`s are never split. Total count is not enough to ban case where we have type variables `[T, *Ts, S, U]` and arguments `[int, int, *Us, int]`. We need to explicitly ensure that actual suffix and prefix are longer or equal to formal ones. Such splitting would be very hard to support, and is explicitly banned by the PEP. * Few minor cleanups. Some random comments: * It is tricky to preserve valid parts of type arguments, if there is an argument count error involving an unpack. So after such error I simply set all arguments to `Any` (or `*tuple[Any, ...]` when needed). * I know there is some code duplication. I tried to factor it away, but it turned out non-trivial. I may do some de-duplication pass after everything is done, and it is easier to see the big picture. * Type applications (i.e. when we have `A[int, int]` in runtime context) are wild west currently. I decided to postpone variadic support for them to a separate PR, because there is already some support (we will just need to handle edge cases and more error conditions) and I wanted minimize size of this PR. * Something I wanted to mention in one of previous PRs but forgot: Long time ago I proposed to normalize away type aliases inside `Unpack`, but I abandoned this idea, it doesn't really give us any benefits. As I said, this is the last PR for the "static part", in the next PR I will work on fixing subtyping and inference for variadic instances. And then will continue with remaining items I mentioned in my master plan in #15924 Fixes #15978 --------- Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
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.
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):
TupleTypeandInstanceitems insemanal_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*Tsor*tuple[X, ...]. A single top-levelTupleTypecan appear in an unpack only as type of*args.FuncDef.typeandFuncDef.argumentsthat 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).Unpackto be disallowed by default intypeanal.py, only very specific potions are allowed now. Although this required plumbingallow_unpackall the way fromsemanal.py, conceptually it is simple. This is similar to howParamSpecis handled.TypeAliasExpr(and made it simple reference to theSymbolNode, like e.g.TypedDictExprandNamedTupleExpr). 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):
UnpackTypegracefully. These are<nothing>andAny(and also potentiallyobject). 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).Unpackis handled, and verify both possible variadic items are handled.Instanceconstrains and subtyping (the latter is probably OK, but the former may be broken).CallableandTuplesubtyping for variadic-related edge cases (constraints seem OK for these).map_instance_to_supertype()(if no changes are needed, add tests for subclassing).TypeVarTuples (and add some support if missing but looks important).I hope to finish these in next few weeks.