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

Don't serialize redefined symbol nodes #7499

Merged
merged 1 commit into from Sep 12, 2019

Conversation

@msullivan
Copy link
Collaborator

msullivan commented Sep 11, 2019

Redefined nodes sometimes have busted internal references (like a self
argument that has the class as its type even though it won't be able
to look it up) which can cause crashes. Since they can't have
references from outside themselves, we don't lose anything by not
serializing them.

Fixes a crash I observed at Dropbox.

This partially reverts #7413, which introduced this crash (while
fixing another), but leaves the test case it added.

This (or another fix for this crash) needs to go into #7461 since it is a kind of bad regression.

Redefined nodes sometimes have busted internal references (like a self
argument that has the class as its type even though it won't be able
to look it up) which can cause crashes. Since they can't have
references from outside themselves, we don't lose anything by not
serializing them.

Fixes a crash I observed at Dropbox.

This partially reverts #7413, which introduced this crash (while
fixing another), but leaves the test case it added.
@msullivan msullivan requested a review from JukkaL Sep 11, 2019
@JukkaL
JukkaL approved these changes Sep 12, 2019
Copy link
Collaborator

JukkaL left a comment

Thanks for the quick fix! I agree that this looks important.

@JukkaL JukkaL merged commit 28423cd into master Sep 12, 2019
3 checks passed
3 checks passed
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
JukkaL added a commit that referenced this pull request Sep 12, 2019
Redefined nodes sometimes have busted internal references (like a self
argument that has the class as its type even though it won't be able
to look it up) which can cause crashes. Since they can't have
references from outside themselves, we don't lose anything by not
serializing them.

Fixes a crash I observed at Dropbox.

This partially reverts #7413, which introduced this crash (while
fixing another), but leaves the test case it added.
@JukkaL JukkaL mentioned this pull request Sep 12, 2019
@ilevkivskyi ilevkivskyi deleted the no_serialize_redefs branch Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.