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 on incompatible redefinition in named tuple #3760

Merged
merged 1 commit into from Jul 24, 2017

Conversation

Projects
None yet
3 participants
@ilevkivskyi
Collaborator

ilevkivskyi commented Jul 24, 2017

Fixes #3759

Currently we don't support generic named tuples, so using a fallback to instance is a reasonable solution.
(Later, when we will support generic named tuples, maybe we could use item types to get a more precise type).

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Jul 24, 2017

Member

This bug is a regression since 0.511, right? Is it worth cherrypicking the fix into the 0.521 release?

Member

gvanrossum commented Jul 24, 2017

This bug is a regression since 0.511, right? Is it worth cherrypicking the fix into the 0.521 release?

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Jul 24, 2017

Collaborator

This bug is a regression since 0.511, right?

I didn't do bisect, but I think this is very old, the code affected was last changed almost half year ago.

Collaborator

ilevkivskyi commented Jul 24, 2017

This bug is a regression since 0.511, right?

I didn't do bisect, but I think this is very old, the code affected was last changed almost half year ago.

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Jul 24, 2017

Member

Actually it seems to be caused by 3589c08, Add support for NamedTuple methods (#3081) by @JelleZijlstra.

Member

gvanrossum commented Jul 24, 2017

Actually it seems to be caused by 3589c08, Add support for NamedTuple methods (#3081) by @JelleZijlstra.

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Jul 24, 2017

Collaborator

So actually few days after 0.511. Then formally speaking this is a regression.

Collaborator

ilevkivskyi commented Jul 24, 2017

So actually few days after 0.511. Then formally speaking this is a regression.

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Jul 24, 2017

Member

So how confident are you? Anyway I'm testing this against the Dropbox codebase.

Member

gvanrossum commented Jul 24, 2017

So how confident are you? Anyway I'm testing this against the Dropbox codebase.

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Jul 24, 2017

Collaborator

I think this should be quite safe (at least for sure not less safe than it was). There are only three possibilities: Instance, TupleType, and None. None can't happen, since this function is only called from within a class.

Collaborator

ilevkivskyi commented Jul 24, 2017

I think this should be quite safe (at least for sure not less safe than it was). There are only three possibilities: Instance, TupleType, and None. None can't happen, since this function is only called from within a class.

@gvanrossum gvanrossum merged commit 86ef4ae into python:master Jul 24, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Jul 24, 2017

Member

Merged, as it did not introduce new errors in our codebase. I will also cp.

Member

gvanrossum commented Jul 24, 2017

Merged, as it did not introduce new errors in our codebase. I will also cp.

gvanrossum added a commit to gvanrossum/mypy that referenced this pull request Jul 24, 2017

@JelleZijlstra

This comment has been minimized.

Show comment
Hide comment
@JelleZijlstra

JelleZijlstra Jul 24, 2017

Collaborator

Thanks for fixing the crash Ivan!

Arguably we should give an error for this case, since it breaks LSP. (If you take a tuple as an argument and use .count() on it, it will break if your tuple is actually a NamedTuple with a count attribute.) But that's less urgent.

Collaborator

JelleZijlstra commented Jul 24, 2017

Thanks for fixing the crash Ivan!

Arguably we should give an error for this case, since it breaks LSP. (If you take a tuple as an argument and use .count() on it, it will break if your tuple is actually a NamedTuple with a count attribute.) But that's less urgent.

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Jul 24, 2017

Collaborator

But that's less urgent.

The error is shown (see test). I just re-use the fallback Instance to resolve the attribute type.

Collaborator

ilevkivskyi commented Jul 24, 2017

But that's less urgent.

The error is shown (see test). I just re-use the fallback Instance to resolve the attribute type.

@ilevkivskyi ilevkivskyi deleted the ilevkivskyi:crash-incompatible-namedtuple branch Jul 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment