KEMBAR78
Fix unresolved reference + tuples crashing by A5rocks · Pull Request #10401 · python/mypy · GitHub
Skip to content

Fix unresolved reference + tuples crashing #10401

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
May 11, 2021
Merged

Fix unresolved reference + tuples crashing #10401

merged 5 commits into from
May 11, 2021

Conversation

A5rocks
Copy link
Collaborator

@A5rocks A5rocks commented May 2, 2021 •
edited
Loading

Description

Fixes #10400

This PR prevents passing in a TypeJoinVisitor to PlaceholderType#accept (which only expects SyntheticTypeVisitors).

There's probably a better way, as PlaceholderType claims "After semantic analysis, no placeholder types must exist," yet this happens after semantic analysis (see line 1552 in the crash traceback)... So, the better way would involve not getting to this join_types function in the first place, but I'm not sure where to put a check.

Test Plan

I'm not quite sure how to add tests for this (especially since there's no testjoin.py in mypy/test)... (I think I figured this out, I'll add them eventually) Added a regression test. Additionally, I verified this works locally with the reproduction code from the issue linked:

$ mypy repro.py
repro.py:7: error: Name "aaaaaaaaaa" is not defined
repro.py:7: error: Class cannot subclass 'aaaaaaaaaa' (has type 'Any')
Found 2 errors in 1 file (checked 1 source file)

@A5rocks
Copy link
Collaborator Author

A5rocks commented May 3, 2021

It would seem this PR needs some sort of button to be pressed to have mypy primer run against it?

image

@emmatyping
Copy link
Member

It would seem this PR needs some sort of button to be pressed to have mypy primer run against it?

Yeah github now requires maintainers to approve PRs to run in CI for security reasons.

@TH3CHARLie
Copy link
Collaborator

There's probably a better way, as PlaceholderType claims "After semantic analysis, no placeholder types must exist," yet this happens after semantic analysis (see line 1552 in the crash traceback)... So, the better way would involve not getting to this join_types function in the first place, but I'm not sure where to put a check.

Personally, I think this is a good fix and since it fixes a crash I tend to approve and merge it. But that's very valid concern and I am not very familiar with this part of the checker. So cc @JukkaL

@TH3CHARLie TH3CHARLie requested review from JukkaL and hauntsaninja May 8, 2021 06:15
hauntsaninja
hauntsaninja approved these changes May 11, 2021
View reviewed changes
Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thank you for this fix, and the original clear bug report! :-) Like th3charlie I'm not comfortable enough with PlaceholderTypes to know how to get rid of them before we start doing type ops, but this quite effective as a way to avoid a crash. I took the liberty of adding a comment and changing the TypeOfAny.

@hauntsaninja hauntsaninja merged commit 466017d into python:master May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hauntsaninja hauntsaninja hauntsaninja approved these changes

@JukkaL JukkaL Awaiting requested review from JukkaL

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Unresolved references + tuples crashes mypy

4 participants

@A5rocks @emmatyping @TH3CHARLie @hauntsaninja