Support generic partial types for attributes #8044
Conversation
There was a problem hiding this comment.
Makes sense -- this improves consistency and reduces the number of type annotations needed.
Left a few minor comments and suggested one change in semantics (I propose that the new semantics wouldn't work across method boundaries, since the None case basically works by accident).
test-data/unit/check-inference.test
Outdated
| class C: | ||
| a = {} | ||
| def __init__(self) -> None: | ||
| self.a[0] = 'yes' |
test-data/unit/check-inference.test
Outdated
| class C: | ||
| a = [] | ||
| def __init__(self) -> None: | ||
| self.a = [1] |
There was a problem hiding this comment.
I'd rather not add support for this, since I want to deprecate not using --local-partial-types in the not too distant future (to make daemon and non-daemon modes uniform). If we further increase the semantic differences of --local-partial-types, the deprecation is going to be more painful.
test-data/unit/check-inference.test
Outdated
| class C: | ||
| a = [] | ||
| def __init__(self) -> None: | ||
| self.a.append(1) |
test-data/unit/check-inference.test
Outdated
| class C: | ||
| a = None | ||
| def __init__(self) -> None: | ||
| self.a = 1 |
There was a problem hiding this comment.
This worked before this PR already, so this needs to be supported for now.
| @@ -2714,6 +2714,7 @@ class C: | |||
| class D: | |||
| def __init__(self) -> None: | |||
| self.x = {} | |||
| def meth(self) -> None: | |||
There was a problem hiding this comment.
What about also testing that the type inference works correctly in fine-grained mode (within one method)?
| def __init__(self) -> None: | ||
| self.a = [] | ||
| if bool(): | ||
| self.a = [1] |
There was a problem hiding this comment.
Test using a non-self attribute -- it shouldn't affect type inferece. For example:
...
self.a = []
if bool():
a = self
a.a = [1] # Should not infer type
a.append(1) # Should not infer type| If the expression is not a self attribute, or attribute is not variable, | ||
| or variable is not partial, return None. | ||
| """ | ||
| if not (isinstance(expr.expr, NameExpr) and |
There was a problem hiding this comment.
Does this duplicate any logic used for None partial type handling? Could they be merged?
There was a problem hiding this comment.
Does this duplicate any logic used for
Nonepartial type handling? Could they be merged?
Short answers are no and no. Some things may be however simplified after (or while) fixing #8043 (at least the first bullet).
Work towards #1055
Currently partial types are supported for local variables. However, only partial
Nonetypes are supported forselfattributes. This PR adds the same level of support to generic partial types. They follow mostly the same rules:local_partial_types = True.The logic is pretty simple: the
.nodeattribute forself.attrexpressions is set toNone, so I added a little helper to get it from the class symbol table instead.I add tests for various scenarios, including for partial
Nonetypes. It may happen they are already tested elsewhere, but this way it is easy to compare side-by-side behavior is consistent for both kinds of partial types. I can remove them if you think they are redundant.