Skip to content

[mypyc] Fix regression with nested functions#16484

Merged
ilevkivskyi merged 2 commits intopython:masterfrom
ilevkivskyi:fix-mypyc-regr
Nov 16, 2023
Merged

[mypyc] Fix regression with nested functions#16484
ilevkivskyi merged 2 commits intopython:masterfrom
ilevkivskyi:fix-mypyc-regr

Conversation

@ilevkivskyi
Copy link
Copy Markdown
Member

@ilevkivskyi ilevkivskyi commented Nov 13, 2023

Fixes #16480

Fix is straightforward, but also suspicious, since I am not sure how it ever worked without this.

@mr-c
Copy link
Copy Markdown
Contributor

mr-c commented Nov 14, 2023

I can confirm that this fixes my original issue, thank you!

@mr-c mr-c mentioned this pull request Nov 14, 2023
Merged
2 tasks
@ilevkivskyi
Copy link
Copy Markdown
Member Author

ilevkivskyi commented Nov 15, 2023

Some more info for reviewers (@JukkaL @msullivan @ichard26): currently visit_symbol_node() is only called from visit_name_expr(), but the former method has two branches: for when we already seen the name, and when we see it first time. These branches are hit when a NameExpr appears as a reference, or as an assignment l.h.s. (i.e. definition), correspondingly. This is why free_variables are set correctly for variables, but for functions this was broken, because visit_symbol_node() was not called on function definition, only when it appears as reference.

ichard26
ichard26 approved these changes Nov 15, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@ichard26 ichard26 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't merge, but LGTM. I appreciate the explanation, it was helpful to understand the patch!

@ilevkivskyi ilevkivskyi merged commit b425bd6 into python:master Nov 16, 2023
@ilevkivskyi ilevkivskyi deleted the fix-mypyc-regr branch November 16, 2023 11:56
@mr-c mr-c mentioned this pull request Nov 16, 2023
Closed
2 tasks
@AlexWaygood AlexWaygood mentioned this pull request Nov 18, 2023
Merged
3 tasks
JukkaL pushed a commit that referenced this pull request Nov 22, 2023
@ilevkivskyi @JukkaL
[mypyc] Fix regression with nested functions (#16484)
c10e173 
Fixes #16480

Fix is straightforward, but also suspicious, since I am not sure how it
ever worked without this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@msullivan msullivan msullivan approved these changes

@JukkaL JukkaL Awaiting requested review from JukkaL

+1 more reviewer

@ichard26 ichard26 ichard26 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1.7.0 regression with mypyc

4 participants

@ilevkivskyi @mr-c @msullivan @ichard26