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 with forward reference in TypedDict #3560

Merged
merged 3 commits into from Jun 21, 2017

Conversation

Projects
None yet
3 participants
@JukkaL
Collaborator

JukkaL commented Jun 16, 2017

Move join to happen after semantic analysis to avoid crash due to
incomplete MRO in TypeInfo. The implementation uses a new semantic
analysis 'fixup' phase.

Fixes #3319. Fixes #2489. Fixes #3316.

Fix crash with forward reference in TypedDict
Move join to happen after semantic analysis to avoid crash due to
incomplete MRO in TypeInfo. The implementation uses a new semantic
analysis 'fixup' phase.

Fixes #3319.
@@ -813,3 +813,29 @@ p = TaggedPoint(type='2d', x=42, y=1337)
p.get('x', 1 + 'y') # E: Unsupported operand types for + ("int" and "str")
[builtins fixtures/dict.pyi]
[typing fixtures/typing-full.pyi]
-- Special cases

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 19, 2017

Collaborator

Just have few comments:

  • Will this PR also fix #3316 and #2489?
  • There is a similar situation with NamedTuple (it just uses fallback = self.named_type('__builtins__.tuple', [AnyType()]) now).
  • Maybe it makes sense to add class syntax to the tests just in case?
@ilevkivskyi

ilevkivskyi Jun 19, 2017

Collaborator

Just have few comments:

  • Will this PR also fix #3316 and #2489?
  • There is a similar situation with NamedTuple (it just uses fallback = self.named_type('__builtins__.tuple', [AnyType()]) now).
  • Maybe it makes sense to add class syntax to the tests just in case?

This comment has been minimized.

@JukkaL

JukkaL Jun 20, 2017

Collaborator

Good comments! My responses:

  • This seems to remove the only remaining join during semantic analysis. Updated fixed issues in PR description.
  • Added #3575 about using join with tuples. This seems to affect ordinary tuples as well, not just named tuples.
  • I'll add a test case using the class syntax.
@JukkaL

JukkaL Jun 20, 2017

Collaborator

Good comments! My responses:

  • This seems to remove the only remaining join during semantic analysis. Updated fixed issues in PR description.
  • Added #3575 about using join with tuples. This seems to affect ordinary tuples as well, not just named tuples.
  • I'll add a test case using the class syntax.
@gvanrossum

The two comments are optional, but I wonder if you could use a different term than "fixup" since that's also used for post-deserialization fixup (e.g. fixup.py, used by build.py).

@@ -2366,11 +2374,19 @@ def fail_typeddict_arg(self, message: str,
def build_typeddict_typeinfo(self, name: str, items: List[str],
types: List[Type]) -> TypeInfo:
mapping_value_type = join.join_type_list(types)
fallback = (self.named_type_or_none('typing.Mapping',

This comment has been minimized.

@gvanrossum

gvanrossum Jun 20, 2017

Member

Just curious whether it would ever make a difference whether this was MutableMapping?

@gvanrossum

gvanrossum Jun 20, 2017

Member

Just curious whether it would ever make a difference whether this was MutableMapping?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 20, 2017

Collaborator

Probably the most precise fallback would be just Dict, but probably we want to have some flexibility because Mapping is covariant in value type (unlike MutableMapping and Dict that are invariant).

@ilevkivskyi

ilevkivskyi Jun 20, 2017

Collaborator

Probably the most precise fallback would be just Dict, but probably we want to have some flexibility because Mapping is covariant in value type (unlike MutableMapping and Dict that are invariant).

This comment has been minimized.

@gvanrossum

gvanrossum Jun 21, 2017

Member

Hm, now you mention variance, why shouldn't this be invariant?

@gvanrossum

gvanrossum Jun 21, 2017

Member

Hm, now you mention variance, why shouldn't this be invariant?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 21, 2017

Collaborator

My previous comment was just my interpretation of the status quo. As far as I am concerned, I would prefer fallback to be just Dict (this would be most precise and most similar to how tuples and named tuples work).

@ilevkivskyi

ilevkivskyi Jun 21, 2017

Collaborator

My previous comment was just my interpretation of the status quo. As far as I am concerned, I would prefer fallback to be just Dict (this would be most precise and most similar to how tuples and named tuples work).

This comment has been minimized.

@JukkaL

JukkaL Jun 21, 2017

Collaborator

The reason why we don't use a mutable type as a fallback is that it would compromise type safety. For example, if the fallback would be Dict[str, (join of value types)], then for {'a': 1, 'b': 'x'} the fallback type would be Dict[str, object]. Through the fallback type we could do things like del d['a'] or d['a'] = [1] which would break safety. Mapping only supports getting values, so it's safe. Also, since Mapping only provides read operations, it can be covariant in the value type.

Making a typed dict compatible with Dict[str, Any] without making this the fallback might be a reasonable thing to do, but it's unclear if that would result in ambiguity or other problems.

@JukkaL

JukkaL Jun 21, 2017

Collaborator

The reason why we don't use a mutable type as a fallback is that it would compromise type safety. For example, if the fallback would be Dict[str, (join of value types)], then for {'a': 1, 'b': 'x'} the fallback type would be Dict[str, object]. Through the fallback type we could do things like del d['a'] or d['a'] = [1] which would break safety. Mapping only supports getting values, so it's safe. Also, since Mapping only provides read operations, it can be covariant in the value type.

Making a typed dict compatible with Dict[str, Any] without making this the fallback might be a reasonable thing to do, but it's unclear if that would result in ambiguity or other problems.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 21, 2017

Collaborator

Maybe I am missing something, but TypedDict is special-cased in cheker.check_indexed_assignment, however del d['a'] indeed goes through just checking for __delitem__ and the latter is looked up on the fallback. Maybe we can just special-case this too? TypedDicts at runtime are just dicts and have methods like update that can't be accessed on Mapping. Also TypedDict with total=False can support item deletion.

Anyway, it looks like both Mapping and Dict fallbacks require some special-casing, and it is not something where I have a strong opinion.

@ilevkivskyi

ilevkivskyi Jun 21, 2017

Collaborator

Maybe I am missing something, but TypedDict is special-cased in cheker.check_indexed_assignment, however del d['a'] indeed goes through just checking for __delitem__ and the latter is looked up on the fallback. Maybe we can just special-case this too? TypedDicts at runtime are just dicts and have methods like update that can't be accessed on Mapping. Also TypedDict with total=False can support item deletion.

Anyway, it looks like both Mapping and Dict fallbacks require some special-casing, and it is not something where I have a strong opinion.

This comment has been minimized.

@JukkaL

JukkaL Jun 21, 2017

Collaborator

Yeah, we could special case additional methods, but it wouldn't still be safe: a TypedDict is a subtype of the fallback type, and if the fallback type is Dict, we could upcast a TypedDict to the Dicttype and do things that the TypedDict wouldn't support, such as setting items with incompatible value types, or deleting required keys.

@JukkaL

JukkaL Jun 21, 2017

Collaborator

Yeah, we could special case additional methods, but it wouldn't still be safe: a TypedDict is a subtype of the fallback type, and if the fallback type is Dict, we could upcast a TypedDict to the Dicttype and do things that the TypedDict wouldn't support, such as setting items with incompatible value types, or deleting required keys.

Show outdated Hide outdated mypy/semanal.py
or self.object_type())
def fixup() -> None:
mapping_value_type = join.join_type_list(types)
fallback.args[1] = mapping_value_type

This comment has been minimized.

@gvanrossum

gvanrossum Jun 20, 2017

Member

I don't see much value in introducing an extra local variable here.

@gvanrossum

gvanrossum Jun 20, 2017

Member

I don't see much value in introducing an extra local variable here.

This comment has been minimized.

@JukkaL

JukkaL Jun 21, 2017

Collaborator

I can replace it with a comment.

@JukkaL

JukkaL Jun 21, 2017

Collaborator

I can replace it with a comment.

@gvanrossum gvanrossum merged commit 94b4cfd into master Jun 21, 2017

3 checks passed

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

@gvanrossum gvanrossum deleted the typeddict-crash branch Jun 21, 2017

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