Skip to content

Fix some crashes in dataclasses - #8271

Merged
JukkaL merged 4 commits into
masterfrom
dataclass
Jan 16, 2020
Merged

JukkaL merged 4 commits into
masterfrom
dataclass

Conversation

@msullivan

Copy link
Copy Markdown
Collaborator

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

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

@JukkaL JukkaL left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the updates! Looks good now.

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.

AssertionError: All arguments must be fully typed

2 participants