Repository navigation
New semantic analyzer: conditional function definitions - #6429
Conversation
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
Thanks for fixing this tricky part! I have few comments.
| # Add module-level function to symbol table on first iteration | ||
| # (recurse_into_functions == False). Also add nested functions | ||
| # always, since they won't be processed as separate targets. | ||
| if not self.recurse_into_functions or len(self.function_stack) > 0: |
There was a problem hiding this comment.
This explanation is not very clear. I would rather say that we don't add function top-level functions when we just enter each of it during second phase of analysis -- processing of functions.
Also I would rather explain why this is needed (I agree that this looks reasonable).
| from m import f | ||
| else: | ||
| @dec | ||
| def f(x: int) -> None: |
There was a problem hiding this comment.
Do we have a test case for when this causes a type error? (Decorated function signature differs from original one.)
There was a problem hiding this comment.
This doesn't work correctly with the old semantic analyzer either. I'll create an issue about this.
| 1() # E: "int" not callable | ||
| reveal_type(f) # E: Revealed type is 'def (x: builtins.str)' | ||
| [file m.py] | ||
| def f(x: str) -> None: pass |
There was a problem hiding this comment.
I would add a test that a function with same name as a class is still semantically analyzed. In general, do we have enough tests for something like:
x = 1
class C:
pass
class C:
x + 'no way'etc?
| else: | ||
| def f(x: str) -> None: | ||
| ... | ||
| reveal_type(f) # E: Revealed type is 'def (builtins.str)' |
There was a problem hiding this comment.
Does old semantic analyzer support the opposite order? I mean:
if int():
def f(x: str) -> None:
...
else:
f: Callable[[str], None]If yes, then the new one should probably support this too. Otherwise, this is probably not important.
There was a problem hiding this comment.
This generates an error with the old semantic analyzer as well.
… or classes (#6681) Fixes #6432 This is a hot fix that simply extends the solution in PR #6429 to more situations. We agreed with @JukkaL that he will clean this up when he is back from PTO. Note that we still give the errors for redefinitions that we consider invalid (see tests), but we don't crash now.
2 participants
To semantically analyze conditional function definitions (and function
redefinitions) we add redefinitions to a symbol table with a mangled
name of form
name-redefinition[N]. This is necessary sincefunctions are only analyzed if they can be accessed through a symbol
table.
I checked that this fixes most relevant test cases in
check-functions.test. Using a decorated function in a redefinitionis still not working perfectly. That is also partially implemented in
the old semantic analyzer. Conditionally defining an overloaded
function is also not supported. I'll create follow-up issues for these
remaining known limitations.
Fixes #6318.