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
Fixes #3279. Also fixes another related crash.
|
|
||
| [out] | ||
| == | ||
| b.py:7: note: Revealed type is 'def () -> b.C[Any]' |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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): |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
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?
|
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 |
| @@ -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): | |||
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Btw why do we need the and ... here if we anyway use zip() below?
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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?)
Fixes #3279. Also fixes another related crash.