fix(11064): fix crash due to undefined cyclic import *

[hi-ogawa]

(Sorry for another draft PR. I wanted to see CI's results. I'll soon write a detail below)
Sorry for stalling this PR so long. I updated the description below.

Description

Fixes #11064

It seems that the original issue has been fixed already (which corresponds to the test case testCyclicUndefinedImportWithStar3). However, the other two cases (testCyclicUndefinedImportWithStar1 and testCyclicUndefinedImportWithStar2) still cause crash, so this PR is indended to fix them.
While working on the fix, first I tried to detect cyclic import during semanal (e.g. from add_imported_symbol), but in the end, it turns out simply setting can_defer produces sensible error message, so that's what this PR does.

Test Plan

I added four tests related to cyclic import in check-modules.test:

• testCyclicUndefinedImportWithName
• testCyclicUndefinedImportWithStar1
• testCyclicUndefinedImportWithStar2
• testCyclicUndefinedImportWithStar3 (replication of the linked issue)

where testCyclicUndefinedImportWithName and testCyclicUndefinedImportWithStar3 are already working in current master, and the other two are addressed in this PR.

[hauntsaninja]

Thanks for the PR and the additional test cases!

First off, note that I don't understand the deferral parts of the semantic analyser very well :-) Because of my lack of familiarity, while it's clear to me how this avoids the assertion, it's hard for me to know whether this change is actually correct. For example, should we be changing this in add_symbol_table_node itself, rather than just this one place?

Reading the code with that line of thinking got me this alternative patch:

diff --git a/mypy/semanal.py b/mypy/semanal.py
index 3c0d548b2..dc70d0918 100644
--- a/mypy/semanal.py
+++ b/mypy/semanal.py
@@ -4567,7 +4567,12 @@ class SemanticAnalyzer(NodeVisitor[None],
         names = self.current_symbol_table(escape_comprehensions=escape_comprehensions)
         existing = names.get(name)
         if isinstance(symbol.node, PlaceholderNode) and can_defer:
-            self.defer(context)
+            if context is not None:
+                self.process_placeholder(name, 'name', context)
+            else:
+                # see note in docstring about None contexts
+                self.defer(context)
+
         if (existing is not None
                 and context is not None
                 and not is_valid_replacement(existing, symbol)):

Reading the docstrings of defer and process_placeholder, I feel pretty good about that. The net effect should be pretty similar to your PR, but perhaps less specific to the exact issue in #11064 and ensures we'll get some kind of error message if we do hit that during the final iteration. It also avoids crashes in the test cases you added.

What do you think? :-)

[hi-ogawa]

Thanks for the suggestion!
To tell you the truth, I really don't have a clue about defer/placeholder mechanism.
My strategy for investigating this issue was to first try to understand why the case testCyclicUndefinedImportWithName is handled properly (i.e. visit_import_from) and to apply the same solution to the case of "star" import (i.e. visit_import_all), but from what I tried, the situation doesn't seem to be so simple and I ended up just adding can_defer.
My instinct was that a fix should be possible before going to add_simbol_table_node and so I was a bit hesitant to change things around the code you mention (in fact, I haven't even read those parts yet).
I really appreciate the discussion with you, but I have to admit that I might not be up to this task yet.

[jrabbit]

What's the changed behavior supposed to be for large codebases w/ some circular imports? Because I still get a deferral trace and the same message about checking being halted for errors. (when running mypy w/ this PR)

[hauntsaninja]

@jrabbit it sounds like you have a different issue than the one in #11064. Do you have a stack trace, and ideally a repro you can share?

[jrabbit]

Ah, Yeah I can collect logs, I don't know specifically what's causing it, it's a massive somewhat old (and unfortunately, closed source) codebase. I can look into it some more.

[hauntsaninja]

I really appreciate the discussion with you, but I have to admit that I might not be up to this task yet.

I opened #11681 that builds on your work but uses that^ suggested diff. Thank you for making mypy better!!

[hi-ogawa]

@hauntsaninja Thanks for picking up my commits. I hope your PR will get merged soon!