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

Support recursive named tuples #13371

Merged
merged 12 commits into from
Aug 10, 2022

Conversation

ilevkivskyi
Copy link
Member

This is a continuation of #13297

The main change here is that although named tuples are still stored in symbol tables as TypeInfos, when type analyzer sees them, it creates a TypeAliasType targeting what it would return before (a TupleType with a fallback to an instance of that TypeInfo). Although it is a significant change, IMO this is the simplest but still clean way to support recursive named tuples.

Also it is very simple to extend to TypedDicts, but I wanted to make the latter in a separate PR, to minimize the scope of changes.
It would be great if someone can take a look at this PR soon.

The most code changes are to make named tuples semantic analysis idempotent, previously they were analyzed "for real" only once, when all types were ready. It is not possible anymore if we want them to be recursive. So I pass in existing_info everywhere, and update it instead of creating a new one every time.

cc @JukkaL

@ilevkivskyi
Copy link
Member Author

Note that the new attribute tuple_alias for TypeInfo that I added, can be reused for TypedDicts. I am going to rename it to special_alias (or similar), and can be used by both these special forms. Btw, it seems to me after this PR we don't really need tuple_type itself anymore, but it is handy to simplify (de-)serialization, so I decided to keep it.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@ilevkivskyi
Copy link
Member Author

Hm, there is a chance rebasing this on top of #13352 will make the jax errors go away.

@sobolevn
Copy link
Member

sobolevn commented Aug 9, 2022

Related PR: #13029

@ilevkivskyi
Copy link
Member Author

@sobolevn As I mentioned elsewhere that PR will just not work. You can just close it.

@github-actions

This comment has been minimized.

@ilevkivskyi
Copy link
Member Author

Hm, there is a chance rebasing this on top of #13352 will make the jax errors go away.

Yep, it looks like it helped even more than I predicted!

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Looks good. Could be worth an incremental test where the tuple goes from generic to non-generic

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Excellent progress! I think that this is a reasonable approach. Left a few comments, but they don't need to be addressed before merging.

def f(a: T, b: T) -> T: ...
tnt: Tuple[NT]

# Should these be tuple[object] instead?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I think that it would be more correct for these to be tuple[object, ...].

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 updated the comment to be a TODO, but didn't fix it yet. I may take a closer look at meets/joins in general later.

@@ -3448,6 +3448,30 @@ f(a.x)
[out]
==

[case testNamedTupleUpdate5]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you also a coarse-grained incremental test? In particular, test that we can serialize recursive named tuples (both type infos and types in annotations).

@ilevkivskyi
Copy link
Member Author

Thanks for reviews! I added a bunch of both coarse and fine grained tests, and they found couple bugs in the PR (fixing the one in astmerge.py was quite a ride).

I also found an existing bug in aststrip.py (we didn't strip analyzed attribute for classes).

@github-actions

This comment has been minimized.

1 similar comment
@github-actions
Copy link
Contributor

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

jax (https://github.com/google/jax)
- jax/interpreters/mlir.py:96: error: Name "Traceback" is not defined  [name-defined]
- jax/core.py:96: error: Name "Traceback" is not defined  [name-defined]
- jax/interpreters/xla.py:96: error: Name "Traceback" is not defined  [name-defined]
- jax/interpreters/partial_eval.py:96: error: Name "Traceback" is not defined  [name-defined]
- jax/interpreters/batching.py:96: error: Name "Traceback" is not defined  [name-defined]
- jax/experimental/maps.py:96: error: Name "Traceback" is not defined  [name-defined]

pyinstrument (https://github.com/joerick/pyinstrument)
- pyinstrument/profiler.py:201: error: Variable "pyinstrument.typing.LiteralStr" is not valid as a type
- pyinstrument/profiler.py:201: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases

@ilevkivskyi ilevkivskyi merged commit 03638dd into python:master Aug 10, 2022
@ilevkivskyi ilevkivskyi deleted the recursive-named-tuples-2 branch August 10, 2022 15:46
ilevkivskyi added a commit that referenced this pull request Aug 11, 2022
This is a continuation of #13297
Depends on #13371 

It was actually quite easy, essentially just a 1-to-1 mapping from the other PR.
ilevkivskyi added a commit that referenced this pull request Jun 19, 2023
The tests added with this piece of code now pass even without it. It
looks like the crash was properly fixed by
#13371
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.

4 participants