Skip to content
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

Fix crash in daemon mode on new import cycle #14508

Merged
merged 1 commit into from
Jan 23, 2023

Conversation

ilevkivskyi
Copy link
Member

@ilevkivskyi ilevkivskyi commented Jan 23, 2023

Fixes #14329

This fixes the second crash reported in the issue (other one is already fixed). This one is tricky, it looks like it happens only when we bring in a new import cycle in an incremental update with --follow-import=normal. To explain the reason, a little reminder of how semantic analyzer passes work:

  • Originally, we recorded progress automatically when some new symbol was resolved and added to symbol tables.
  • With implementation of recursive types, this mechanism was insufficient, as recursive types require modifying some symbols in place, this is why force_progress flag was added to defer().
  • I was only careful with this flag for recursive type aliases (that were implemented first), for other things (like recursive TypedDicts, etc) I just always passed force_progress=True (without checking if we actually resolved some placeholder types).
  • The reasoning for that is if we ever add becomes_typeinfo=True, there is no way this symbol will later be unresolved (otherwise how would we know this is something that is a type).
  • It turns out this reasoning doesn't work in some edge cases in daemon mode, we do put some placeholders with becomes_typeinfo=True for symbols imported from modules that were not yet processed, thus causing a crash (see test cases).
  • There were two options to fix this: one is to stop creating placeholders with becomes_typeinfo=True for unimported symbols in daemon mode, other one is to always carefully check if in-place update of a symbol actually resulted in progress.
  • Ultimately I decided that the first way is too fragile (and I don't understand how import following works for daemon anyway), and the second way is something that is technically correct anyway, so here is this PR

I didn't add test cases for each of the crash scenarios, since they are all very similar. I only added two that I encountered "in the wild", upper bound and tuple base caused actual crash in trio stubs, plus also randomly a test for a TypedDict crash.

EDIT: and one more thing, the "cannot resolve name" error should never appear in normal mode, only in daemon update (see reasoning above), so I don't make those error messages detailed, just add some minimal info if we will need to debug them.

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Great detective work! Looks good.

@JukkaL JukkaL merged commit dcf910e into python:master Jan 23, 2023
@ilevkivskyi ilevkivskyi deleted the fix-df-crash branch January 23, 2023 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants