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 crashes in dataclasses #8271

Merged
merged 4 commits into from Jan 16, 2020
Merged

Fix some crashes in dataclasses #8271

merged 4 commits into from Jan 16, 2020

Conversation

@msullivan
Copy link
Collaborator

msullivan commented Jan 10, 2020

Store the type of an attribute in DataclassAttribute so we don't
need to pull it out of TypeInfo in a fragile way (since the child
might try to override it in a way that breaks things). This also
allows us to get rid of some pretty dodgy code having to do with
InitVars (that could cause an incremental mode crash.)

Fixes #6809 and an unreported incremental bug

@msullivan msullivan requested review from ilevkivskyi and JukkaL Jan 10, 2020
@msullivan msullivan force-pushed the dataclass branch from fcb012b to 6ebed7b Jan 10, 2020
Store the type of an attribute in `DataclassAttribute` so we don't
need to pull it out of `TypeInfo` in a fragile way (since the child
might try to override it in a way that breaks things). This also
allows us to get rid of some pretty dodgy code having to do with
InitVars (that could cause an incremental mode crash.)

Fixes #6809 and an unreported incremental bug
@msullivan msullivan force-pushed the dataclass branch from 6ebed7b to dde1280 Jan 10, 2020
Copy link
Collaborator

JukkaL left a comment

Looks good, just a few questions. I like how the the new code looks much cleaner.

mypy/plugins/dataclasses.py Outdated Show resolved Hide resolved
assert attr.is_init_var and not attr.is_in_init
continue
if node.type is None:
if attr.type is None:

This comment has been minimized.

Copy link
@JukkaL

JukkaL Jan 13, 2020

Collaborator

What if the some attribute has no type? We seem to handle this case during deserialization, so maybe it can happen here as well. Could this result in always deferring for some attribute? If not, maybe we should require that the serialized type is not None?

This comment has been minimized.

Copy link
@msullivan

msullivan Jan 15, 2020

Author Collaborator

We only consider something as being an attribute if it has a type annotation, so the type missing is (should be) always due to resolution issues that can be fixed by deferring.

I think we can only serialize things with non-None types, yeah.

@JukkaL
JukkaL approved these changes Jan 16, 2020
Copy link
Collaborator

JukkaL left a comment

Thanks for the updates! Looks good now.

@JukkaL JukkaL merged commit 07a20a6 into master Jan 16, 2020
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
@JelleZijlstra JelleZijlstra deleted the dataclass branch Jan 18, 2020
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.

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