New semantic analyzer: Support multiple passes over functions. #6280
Conversation
Previously we would process top levels in alternating orders if they are deferred (a->b, b->a). This would result in the same module (b) processed twice in a row, which may not be efficient if the deferral is due to another module in the cycle, as the second step may then make no progress.
Also remove fake definitions of these aliases from stubs, as they are not necessary.
There was a problem hiding this comment.
This is a very clean way to deal with forward references! Left a few comments. Also, it's not clear if this will we able to catch references to variables before their definition, similar to the old semantic analyzer. I mean things like x; x = 1 currently generate an error. If this is a regression that is non-trivial to fix, it's okay to fix it later.
| @@ -150,6 +150,8 @@ def run_case_once(self, testcase: DataDrivenTestCase, | |||
| options.show_traceback = True | |||
| if 'optional' in testcase.file: | |||
| options.strict_optional = True | |||
| if 'newsemanal' in testcase.file: | |||
| options.new_semantic_analyzer = True | |||
mypy/newsemanal/semanal_main.py
Outdated
| active_type) | ||
| assert not deferred # There can't be cross-function forward refs | ||
| assert not incomplete # Ditto | ||
| iteration = 0 |
There was a problem hiding this comment.
It would be better to move the new logic to a separate function. I think that process_functions is now too big.
| @@ -630,6 +602,9 @@ def analyze_overload_sigs_and_impl( | |||
| types = [] | |||
| non_overload_indexes = [] | |||
| impl = None # type: Optional[OverloadPart] | |||
| if defn.impl: | |||
There was a problem hiding this comment.
Ew the way this works is ugly (the existing logic, not this change in particular). Add TODO item about not modifying the items list.
| return int() | ||
| def other() -> str: | ||
| reveal_type(one()) # E: Revealed type is 'builtins.int' | ||
| return str() |
There was a problem hiding this comment.
It would be good to have a test case where we hit the maximum iteration count (cyclic definition).
mypy/newsemanal/semanal.py
Outdated
| @@ -228,6 +211,8 @@ def __init__(self, | |||
| errors: Report analysis errors using this instance | |||
| """ | |||
| self.locals = [None] | |||
| # Saved namespaces from previous passes. | |||
There was a problem hiding this comment.
A more detailed explanation would be nice here. Also, avoid the use of the term 'pass' since this means a different thing in the old semantic analyzer (maybe 'iteration'?).
| def enter(self) -> None: | ||
| self.locals.append(SymbolTable()) | ||
| def enter(self, function: Optional[FuncItem] = None) -> None: | ||
| if function: |
There was a problem hiding this comment.
Add docstring and describe the argument. Also discuss when it's okay to leave it out.
mypy/newsemanal/semanal_main.py
Outdated
| assert not deferred # There can't be cross-function forward refs | ||
| assert not incomplete # Ditto | ||
| iteration = 0 | ||
| # We need one more pass after incomplete is False. |
There was a problem hiding this comment.
Similar to above, it's probably better to avoid the term 'pass' here. Also consider renaming the more_passes variable.
Yes, it looks like non-trivial to fix in general, I will open an issue. |
This allows supporting forward references within function namespaces. The logic is very simple, go over and over each top-level function/method until everything inside it is resolved, plus one more pass to report errors.
The big diff in
_visit_func_def()is because of whitespace, I didn't change any function body analysis logic apart from removing previous two-pass special-casing for functions (GitHub has an option to ignore whitespace in the diff).