-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New semantic analyzer: fix corner cases in aliases to not ready classes (and other aliases) #6390
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There is actually bug in the logic, so marking as WIP. |
OK, this is now ready for review. I will create couple follow-up issues soon (for tricky corner cases, and possibly re-thinking the logic to avoiding intermediate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's great that we can now handle various tricky special cases involving type aliases. Some things were still a bit unclear to me, as the logic is fairly delicate. Probably worth making some clarifications.
@@ -407,7 +409,10 @@ def analyze_unbound_type_without_type_info(self, t: UnboundType, sym: SymbolTabl | |||
if self.allow_unbound_tvars and unbound_tvar and not self.third_pass: | |||
return t | |||
# None of the above options worked, we give up. | |||
self.fail('Invalid type "{}"'.format(name), t) | |||
if self.api.final_iteration: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit problematic, since we'll have to the maximum number of iterations to report an invalid type. At least add a note about this, since it may not be obvious that the final iteration means "the iteration when we hit the maximum number of iterations limit".
mypy/newsemanal/semanal.py
Outdated
if isinstance(s.rvalue, IndexExpr) and isinstance(s.rvalue.base, RefExpr): | ||
# Special case: analyze index expression _as a type_ to trigger | ||
# incomplete refs for string forward references, for example | ||
# Union['ClassA', 'ClassB'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update comment to mention that we throw away the results of the analysis and we only care about the detection of incomplete references (this doesn't change the expression in place?).
mypy/newsemanal/semanal.py
Outdated
# incomplete refs for string forward references, for example | ||
# Union['ClassA', 'ClassB'] | ||
self.analyze_alias(s.rvalue, allow_placeholder=True) | ||
is_alias = self.is_not_ready_alias(s.rvalue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name is_alias
is a bit confusing -- this is a very specific kind of alias.
mypy/newsemanal/semanal.py
Outdated
# Union['ClassA', 'ClassB'] | ||
self.analyze_alias(s.rvalue, allow_placeholder=True) | ||
is_alias = self.is_not_ready_alias(s.rvalue) | ||
if self.found_incomplete_ref(tag) or is_alias: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need is_alias
here? Wouldn't the first check be sufficient? If not, please explain why.
mypy/newsemanal/semanal.py
Outdated
@@ -1856,16 +1859,42 @@ def add_type_alias_deps(self, aliases_used: Iterable[str], | |||
target = self.scope.current_target() | |||
self.cur_mod_node.alias_deps[target].update(aliases_used) | |||
|
|||
def is_not_ready_alias(self, rv: Expression) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name and the first line of docstring are a bit confusing. This does only seem to look for specific kinds of not ready aliases, and not things like X = List[Y]
if Y
is not ready.
mypy/newsemanal/semanal.py
Outdated
# Initializer couldn't be fully analyzed. Defer the current node and give up. | ||
# Make sure that if we skip the definition of some local names, they can't be | ||
# added later in this scope, since an earlier definition should take precedence. | ||
for expr in names_modified_by_assignment(s): | ||
self.mark_incomplete(expr.name, expr) | ||
self.mark_incomplete(expr.name, expr, becomes_typeinfo=is_alias) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the alias is like X = List[Y]
or X = C[Y]
where only Y
is not ready? It's not clear to me how we'd set becomes_typeinfo
correctly in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case we can turn them into Var
s, this is what I mentioned in private chat. In general, I don't like the current situation, I will try a bigger change, maybe there is a way to make it more straightforward.
@JukkaL As discussed, I added the clarifying comments. I will merge this now and open a follow-up issue about refactoring the analysis of assignment statements. |
Fixes #6355
This does three non-trivial things:
becomes_typeinfo=True
if r.h.s. is such a placeholder (i.e. the definition is a potential type alias).Invalid type
and use of corresponding unbound types until final iteration.I added a bunch of new tests for things that was broken, plus some variations (although some of them are trivial, replacing
NameExpr
withMemberExpr
orIndexExpr
).I also verified that this PR fixes the crash in mypy self-check.