Skip to content

Always create correct instances #3305

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

Merged

Conversation

ilevkivskyi
Copy link
Member

As discussed with @JukkaL in #3279 (comment) we should always create Instances with correct number of type arguments.

This PR makes named_type and other similar functions return correct instances. At the moment this discovered only two actual bugs (with namedtuple and Awaitable). Unfortunately, I could not turn on all the asserts and check for other possible problems, since most test fixtures do:

class tuple: pass
class dict: pass

@ilevkivskyi
Copy link
Member Author

This PR revealed another bigger problem, most "synthetic" types are never visited in third pass, so that the type arguments stay incorrect. For example with NamedTuple:

class N(NamedTuple):
    x: int
    y: List

reveal_type(N(1, []).y)  # Revealed type is 'builtins.list'

The revealed type should be builtins.list[Any], the same applies to TypedDict and NewType.
Even worse, sometimes this can lead to crashes:

class N(NamedTuple):
    x: int
    y: List[int, str, Any]

reveal_type(N(1, []).y) # mypy crashes here with "IndexError: list index out of range"

I will play with this a bit, and if I will find a simple solution, then I will add it here.

@gvanrossum
Copy link
Member

I will play with this a bit, and if I will find a simple solution, then I will add it here.

Thank you! If you don't, we should have an issue to track it.

@gvanrossum gvanrossum changed the title Allways create correct instances Always create correct instances May 3, 2017
@ilevkivskyi
Copy link
Member Author

@gvanrossum It looks like the issue with synthetic types I mentioned above is a bit bigger than I thought, so that I would prefer to make a separate additional PR. Do you have comments on this PR?

@JukkaL
Copy link
Collaborator

JukkaL commented May 3, 2017

The fixtures using class tuple: ... etc. should be fixed to use generic classes (no need to do it here though). It shouldn't be hard, at least unless some tests expect them to not be generic.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

You can add the comment and merge!

mypy/semanal.py Outdated
@@ -2040,7 +2048,7 @@ def build_namedtuple_typeinfo(self, name: str, items: List[str], types: List[Typ
# Actual signature should return OrderedDict[str, Union[types]]
ordereddictype = (self.named_type_or_none('builtins.dict', [strtype, AnyType()])
or self.object_type())
fallback = self.named_type('__builtins__.tuple', types)
fallback = self.named_type('__builtins__.tuple', [join.join_type_list(types)])
Copy link
Member

Choose a reason for hiding this comment

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

This is an Aha! that deserves a comment!

@ilevkivskyi ilevkivskyi merged commit d423061 into python:master May 3, 2017
@ilevkivskyi ilevkivskyi deleted the allways-create-correct-instances branch May 3, 2017 17:23
fallback = self.named_type('__builtins__.tuple', types)
# 'builtins.tuple' has only one type parameter, the corresponding type argument
# in the fallback instance is a join of all item types.
fallback = self.named_type('__builtins__.tuple', [join.join_type_list(types)])
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can't use type joins safely during semantic analysis, since MROs may be incomplete :-(

One potential way to fix this would be to postpone all joins to a new, fourth semantic analysis pass. That sounds risky though, so for the 0.510 release we could just use Any here as the fallback as a temporary workaround.

This has happened so many times that I've been considering asserting during joins and other type operations that need MROs that they aren't called during semantic analysis, at least when running tests.

@ilevkivskyi
Copy link
Member Author

One potential way to fix this would be to postpone all joins to a new, fourth semantic analysis pass.

I just did this in third pass. It seems to work OK. This also solves other crashes, see my comment
#3315 (comment) (I will add tests and make a PR this evening).

But I think you should do whatever you think is safest for the release. So maybe Any is a good workaround.

@JukkaL
Copy link
Collaborator

JukkaL commented May 4, 2017

I just did this in third pass. It seems to work OK. This also solves other crashes, see my comment
#3315 (comment) (I will add tests and make a PR this evening).

We recompute some MROs in the third pass so they could still be incomplete in the middle of the third pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants