New semantic analyzer: Prohibit duplicate type variable definitions and fix stale upper bounds and values#6563
Conversation
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
Thanks! I have one question about incrementally updating a type variable definition and a few nits.
| if existing and not isinstance(existing.node, (TypeVarExpr, PlaceholderNode)): | ||
| if existing and not (isinstance(existing.node, PlaceholderNode) or | ||
| # Also give error for another type variable with the same name. | ||
| isinstance(existing.node, TypeVarExpr) and |
There was a problem hiding this comment.
Style nit: Use parentheses to make precedence explicit when mixing and/or.
| # T = TypeVar('T', bound=C[Any]) | ||
| # class C(Generic[T]): | ||
| # ... | ||
| # We can use allow_placeholder=True for upper bound and values to avoid this, |
There was a problem hiding this comment.
This doesn't make it clear whether we are actually doing this. Instead, write something like "We could use ..., but we don't do this, because ...".
What happens if the type variable definition and the class definition are in different parts of an import cycle? Do we ensure that once the type variable has been completed, the new bound will be used in the class definition?
ilevkivskyi
changed the title
|
|
||
| def visit_type_var(self, t: TypeVarType) -> T: | ||
| return self.strategy([]) | ||
| return self.query_types([t.upper_bound] + t.values) |
There was a problem hiding this comment.
This is strictly speaking unrelated, but it made one new test fail (because has_placeholder() was wrong).
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
This looks like a good way to deal with the problem. I was worried that we need to do something drastic to address the issue, and I'm glad that this is a pretty straightforward change.
Left a few minor comments, otherwise looks good.
| assert isinstance(call.analyzed, TypeVarExpr) | ||
| call.analyzed.upper_bound = upper_bound | ||
| call.analyzed.values = values | ||
| self.progress = True |
There was a problem hiding this comment.
Is this always progress, or should we check for changes in the bounds?
| # self.defer(). | ||
| analyzed = AnyType(TypeOfAny.special_form) | ||
| # Type variables are special: we need to place them in the symbol table | ||
| # soon, even if upper bound is not ready yet. |
There was a problem hiding this comment.
Can you explain why they need to be added to the symbol table soon?
| allow_placeholder=True) | ||
| if analyzed is None: | ||
| # Type variables are special: we need to place them in the symbol table | ||
| # soon, even if some value is not ready yet. |
There was a problem hiding this comment.
Similar to above: can you explain why?
|
Oh and feel free to merge when you've addressed my comments. |
|
Travis hook seems to be stuck, all tests have passed, so I am merging this now. |
2 participants
This is a follow up for #6527.
Fixes #7037
I think this also essentially concludes #6300
This PR prohibits duplicate type variable definitions, previously the second one was silently winning. Also now the first one wins. Note that it didn't require additional attribute on
TypeVarExpras I initially though, it looks like we can simple uses.rvalue.analyzedfor the purpose of linking assignments to variables.This PR also naturally fixes an issue where stale upper bound or value appears in a class that uses the type variable.