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 some daemon crashes involving classes becoming generic #8157

Merged
merged 6 commits into from Dec 18, 2019

Conversation

@msullivan
Copy link
Collaborator

msullivan commented Dec 17, 2019

Fixes #3279. Also fixes another related crash.

Fixes #3279. Also fixes another related crash.
@msullivan msullivan requested review from ilevkivskyi and JukkaL Dec 17, 2019
@JukkaL
JukkaL approved these changes Dec 17, 2019
Copy link
Collaborator

JukkaL left a comment

Great, fewer crashes!

An idea about testing: have you considered also testing this by turning some classes in the mypy implementation to generic (and back)?


[out]
==
b.py:7: note: Revealed type is 'def () -> b.C[Any]'

This comment has been minimized.

Copy link
@JukkaL

JukkaL Dec 17, 2019

Collaborator

What about a similar test case where a class goes from generic to non-generic?

for i in range(len(instance.args)):
# N.B: We use zip instead of indexing because the lengths might have
# mismatches during daemon reprocessing.
for tvar, mapped_arg, instance_arg in zip(tvars, mapped.args, instance.args):

This comment has been minimized.

Copy link
@JukkaL

JukkaL Dec 17, 2019

Collaborator

It's kind of unfortunate that we have to do this. Another option might be to make args a property and have it adjust the number of args dynamically if the TypeInfo has changed. This could have performance impliciations, however.

Copy link
Collaborator

ilevkivskyi left a comment

This is unfortunate. What about returning something obviously wrong (like we do in fixup.py) for the temporary results? Otherwise this may mask some other bugs.

Also why not fix a similar issue in mypy.join.join_instances()? What about a test where number or type variables changes from 2 to 1 and/or vice versa?

@msullivan

This comment has been minimized.

Copy link
Collaborator Author

msullivan commented Dec 17, 2019

I didn't do join_instances because I couldn't manage to make it crash and because I had convinced myself from a glance that it couldn't break because of some other side condition, but I don't think that is actually true.

I don't love the idea of adjusting things as an args property, which feels too magic and has performance implications.

@@ -486,13 +486,15 @@ def visit_type_var(self, t: TypeVarType) -> ProperType:
def visit_instance(self, t: Instance) -> ProperType:
if isinstance(self.s, Instance):
si = self.s
if t.type == si.type:
if t.type == si.type and len(t.args) == len(si.args):

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Dec 17, 2019

Collaborator

Btw why do we need the and ... here if we anyway use zip() below?

This comment has been minimized.

Copy link
@msullivan

msullivan Dec 17, 2019

Author Collaborator

Oh, no, we definitely do not. The and was my first approach for fixing this, before I decided I liked the zip version more. (This check does work, though, so that could be an approach?)

Copy link
Collaborator

ilevkivskyi left a comment

Just to clarify, this is good to go, but couple more tests that Jukka and me proposed would be better.

@msullivan msullivan merged commit 331329c into master Dec 18, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@msullivan msullivan deleted the daemon-generic-crashes branch Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.