Support recursive NamedTuple classes #13029
Closed
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.
Description
Recursive named tuple classes like this now pass:
(Instead of causing a
Cannot resolve name "MyNamedTuple" (possible cyclic definition)
error.)This is done by letting
mypy.semanal_namedtuple.analyze_namedtuple_classdef
complete recursive type references in a subsequent pass.Fixes #9397.
Test Plan
Tests are updated to reflect this new support. (I did not add my own use case above since the existing tests already cover this feature which previously caused the error (along with many more special cases).)
Questions
I believe this change is good, but it raises some questions.
The now conditional call to
add_symbol
inmypy.semanal.prepare_class_def
does add to the somewhat complex situation already in there, regarding func scope nesting. I tried to make the logic clear, but would you prefer me to attempt to improve it further?(I did note the already present
TODO
in there. It seems hard to get at the clarity without untangling that, which I think is a separate task. I initially added a flag to avoid the double check, which I can't say made it more understandable, but rather indicated the already noted need for a refactoring.)Related but distinct, an odd error cropped up in
check-incremental.test::testIncrementalWithDifferentKindsOfNestedTypesWithinMethod
which madelookup_fully_qualified
throw an error:Somehow this is prevented by a call to
reveal_type
(regardless of value). Did I stumble upon a separate issue, or is there an obvious mistake in this change?In
check-namedtuple.test::testSelfRefNT4
it is shown thatreveal_type
for an item-accessed named tuple value now returnsbuiltins.object
instead ofAny
. While it is beyond the scope of this PR to fix type-inference for named tuple item access in general, would you consider the resulting semantics a regression or an improvement?I used a
for ... else
inanalyze_namedtuple_classdef
since (IMHO) it's a good fit and made things clearer. But if you're adverse to that language feature I can change it (and if so reasonably also advice against it in the coding conventions).