[ty] Support recursive and stringified annotations in functional typing.NamedTuples#22718
[ty] Support recursive and stringified annotations in functional typing.NamedTuples#22718AlexWaygood merged 9 commits intomainfrom
typing.NamedTuples#22718Conversation
Typing conformance resultsNo changes detected ✅ |
|
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-return-type |
4 | 1 | 2 |
invalid-argument-type |
0 | 1 | 1 |
invalid-named-tuple |
1 | 0 | 0 |
| Total | 5 | 2 | 3 |
AlexWaygood
force-pushed
the
alex/defer-namedtuple
branch
from
c329358 to
7614073
Compare
This comment was marked as resolved.
This comment was marked as resolved.
AlexWaygood
requested review from
carljm,
dcreager and
sharkdp
as code owners
This comment was marked as resolved.
This comment was marked as resolved.
AlexWaygood
force-pushed
the
alex/defer-namedtuple
branch
2 times, most recently
from
75fb4d6 to
d46b150
Compare
AlexWaygood
force-pushed
the
alex/defer-namedtuple
branch
from
d46b150 to
b25dcc8
Compare
There was a problem hiding this comment.
Unfortunately a lot of the diff on this file is still just code moving around. I tried to keep a fairly atomic commit history on this PR, though; it may be easiest to review commit-by-commit.
|
Looks great. Are we certain there are no other cases in which a dangling call could make a recursive reference? What about, e.g.: from typing import NamedTuple
X = type("X", (NamedTuple("NT", [("field", "X | int")]),), {}) |
Nice. That causes a panic on this branch 🙃 I think what that means is that we may have to defer inference of the bases tuple passed to |
I'm sort-of inclined to do that as a followup? It's a real bug that we should fix, but it also feels very unlikely to come up in real code, and it'll make the diff on this PR much bigger. I would add an x-failing mdtest with the |
|
And the panic isn't new -- it's a pre-existing issue. I can trigger it on X = type("X", (tuple["X | None"],), {})So we do just need to defer inference of the bases tuple passed to |
|
Sounds good 👍 |
|
I suspect I can fix that panic once this merges given the patterns established here (unless you're on it). |
yeah, that would be great! |
dcreager
left a comment
dcreager
left a comment
There was a problem hiding this comment.
No concerns with the implementation, just some clarifying questions for my own curiosity
| fn deferred_spec_initial<'db>( | ||
| db: &'db dyn Db, | ||
| _id: salsa::Id, | ||
| _definition: Definition<'db>, | ||
| ) -> NamedTupleSpec<'db> { | ||
| NamedTupleSpec::unknown(db) | ||
| } |
There was a problem hiding this comment.
I like how the cycle function is right next to the tracked function itself.
| /// Dangling calls can always store the spec. They *can* contain | ||
| /// forward references if they appear in class bases: |
There was a problem hiding this comment.
Does this suggest that it's okay to have a dangling call outside of a base class list, but then the call cannot have forward/string references?
Something like
def foo(base):
class Point(base): ...
return Point
foo(NamedTuple("Good", (("x", int), ("y", int)))) # okay
foo(NamedTuple("Bad", (("x", "Forward"), ("y", "Forward")))) # bad
class Forward: ...(Or are dangling calls only allowed in base class lists?)
There was a problem hiding this comment.
Dangling calls are allowed in arbitrary locations, but I think they can only cause problematic cycles if they appear in class bases or other locations where we know we will need to defer inference (such as the bases list passed to type() calls -- see astral-sh/ty#2564).
I would not describe myself as 100% confident that this is correct, so if you can think of any other examples where this PR branch stack-overflows or panics, I'd love to know about them 😆
There was a problem hiding this comment.
(We don't emit any errors on that snippet with this branch!)
| actual: Type<'db>, | ||
| polarity: TypeVarVariance, | ||
| mut f: &mut dyn FnMut(TypeVarAssignment<'db>) -> Option<Type<'db>>, | ||
| seen: &mut FxHashSet<(Type<'db>, Type<'db>)>, |
There was a problem hiding this comment.
I was going to suggest making seen a field of the SpecBuilder, to reduce the churn on these method signatures. But on second thought I'm not sure it would be worth it — the naive approach would mean that if we call infer_map more than once with the same formal/actual type, we would skip all inference work for the second and later calls. But the result is important, since we use it to produce "invalid argument" diagnostics. So you'd have to make seen a map, and store the method result, so that we still return the same result for later (cached) calls. And at that point I'm not convinced it's worth the effort just to avoid a new method argument.
Co-authored-by: Douglas Creager <dcreager@dcreager.net>
AlexWaygood
force-pushed
the
alex/defer-namedtuple
branch
from
4ab0615 to
8c0a88a
Compare
3 participants
Fixes astral-sh/ty#2528, fixes astral-sh/ty#2529.
In order to support
typing.NamedTuples with recursive or stringified types in their field specs, we must defer inference of the second argument totyping.NamedTuple()calls. However, we don't need to defer inference ofcollections.namedtuple()calls (those calls don't expect type expressions -- they just expect a string literal or a sequence of string literals), and nor do we need to defer inference of "dangling"NamedTuplecalls. DanglingNamedTupleclasses can be recursive, but only in the context of when they appear inside a class's bases list, and we already defer inference of all expressions inside a class's bases list.Test plan
Mdtests updated and extended
Co-authored-by @charliermarsh (picking up from #22627)