New semantic analyzer: support named tuples#6330
Conversation
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
Looks good! Just one significant request (don't reanalyze if it was previously successful) and some minor things. The prior can be implemented in a separate PR.
|
@JukkaL Thanks for review! Because you merged your PR first, it was necessary to update my implementation logic for assignment based named tuples (basically it is much closer to class based one now). I however discovered couple small bugs:
I fixed them and added more tests. |
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
Thanks for the updates! Left some more comments.
mypy/newsemanal/semanal.py
Outdated
| return | ||
| # Yes, it's a valid namedtuple, but defer if it is not ready. | ||
| if not info: | ||
| self.add_symbol(name, PlaceholderNode(self.qualified_name(name), lvalue, True), |
There was a problem hiding this comment.
mark_incomplete adds a PlaceholderNode. The logic is a bit delicate so it's important that all additions go through the same implementation.
| while deferred and more_iterations: | ||
| iteration += 1 | ||
| if not incomplete or iteration == MAX_ITERATIONS: | ||
| if not (deferred or incomplete) or iteration == MAX_ITERATIONS: |
There was a problem hiding this comment.
A namespace can be deferred while it's incomplete. I think that we should also remove the namespace from incomplete namespaces if it's complete. Now it looks like that we sometimes don't remove the namespace from incomplete_namespaces even though we probably should.
test-data/unit/check-newsemanal.test
Outdated
| In = NamedTuple('In', [('s', str), ('t', Other)]) | ||
| class Other: pass | ||
|
|
||
| [case testNewAnalyzerNamedTupleMixed1] |
There was a problem hiding this comment.
Is there a specific reason for adding these tests? It will be impractical to test all combinations of features anyway, as the number of test cases will grow too quickly. For example, is this more useful than a mixed named tuple vs. typed dict test case? It may be better to only test combinations that are known to be risky or have been broken previously.
There was a problem hiding this comment.
Yes, these two tests cases were broken, but then the breakage had the same cause as for assignment based vs assignment based, so it is probably OK to remove them.
| defn.info._fullname = self.cur_mod_id + '.' + local_name | ||
| if defn.info.is_named_tuple: | ||
| # Module already correctly set for named tuples. | ||
| defn.info._fullname += '@' + str(defn.line) |
There was a problem hiding this comment.
It's unclear why we need this special case. Even if the module was already correctly set, if it's always self.cur_mod_id, I think that it's better to avoid this if statement if it doesn't change behavior.
There was a problem hiding this comment.
As discussed elsewhere this is (unfortunately) actually needed.
JukkaL
changed the title
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
Thanks for the updates! Looks good now, just one remaining nit.
mypy/newsemanal/semanal.py
Outdated
| name: The name that we weren't able to define (or '*' if the name is unknown) | ||
| node: The node that refers to the name (definition or lvalue) | ||
| becomes_typeinfo: Pass this to Placeholder (used by special forms like named tuples | ||
| that will create TypeInfos). |
There was a problem hiding this comment.
Style nit: we generally further indent the second line of argument decsriptions.
2 participants
Fixes #6284
Fixes #6285
This doesn't add support for recursive definitions, but all the forward reference patterns that are supported in old semantic analyzer, should be also supported.
Some notes:
semanal_namedtuple.pyTypeInfos in multi-iteration scenario_NTto the bodies (otherwise type variable scope breaks), but it is probably fine because names starting with underscores are anyway prohibited in named tuples.