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

Merged
merged 3 commits into from
Jun 21, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions mypy/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
from os.path import dirname, basename

from typing import (AbstractSet, Dict, Iterable, Iterator, List,
NamedTuple, Optional, Set, Tuple, Union)
NamedTuple, Optional, Set, Tuple, Union, Callable)
# Can't use TYPE_CHECKING because it's not in the Python 3.5.1 stdlib
MYPY = False
if MYPY:
Expand Down Expand Up @@ -1601,15 +1601,21 @@ def parse_file(self) -> None:
self.check_blockers()

def semantic_analysis(self) -> None:
fixups = [] # type: List[Callable[[], None]]
with self.wrap_context():
self.manager.semantic_analyzer.visit_file(self.tree, self.xpath, self.options)
self.manager.semantic_analyzer.visit_file(self.tree, self.xpath, self.options, fixups)
self.fixups = fixups

def semantic_analysis_pass_three(self) -> None:
with self.wrap_context():
self.manager.semantic_analyzer_pass3.visit_file(self.tree, self.xpath, self.options)
if self.options.dump_type_stats:
dump_type_stats(self.tree, self.xpath)

def semantic_analysis_fixups(self) -> None:
for fixup in self.fixups:
fixup()

def type_check_first_pass(self) -> None:
manager = self.manager
if self.options.semantic_analysis_only:
Expand Down Expand Up @@ -2043,6 +2049,8 @@ def process_stale_scc(graph: Graph, scc: List[str], manager: BuildManager) -> No
graph[id].semantic_analysis_pass_three()
for id in fresh:
graph[id].calculate_mros()
for id in stale:
graph[id].semantic_analysis_fixups()
for id in stale:
graph[id].type_check_first_pass()
more = True
Expand Down
22 changes: 19 additions & 3 deletions mypy/semanal.py
Original file line number Diff line number Diff line change
Expand Up @@ -257,13 +257,20 @@ def __init__(self,
self.postponed_functions_stack = []
self.all_exports = set() # type: Set[str]

def visit_file(self, file_node: MypyFile, fnam: str, options: Options) -> None:
def visit_file(self, file_node: MypyFile, fnam: str, options: Options,
fixups: List[Callable[[], None]]) -> None:
"""Run semantic analysis phase 2 over a file.

Add callbacks by mutating the fixups list argument. They will be called
after all semantic analysis phases but before type checking.
"""
self.options = options
self.errors.set_file(fnam, file_node.fullname())
self.cur_mod_node = file_node
self.cur_mod_id = file_node.fullname()
self.is_stub_file = fnam.lower().endswith('.pyi')
self.globals = file_node.names
self.fixups = fixups

if 'builtins' in self.modules:
self.globals['__builtins__'] = SymbolTableNode(
Expand All @@ -290,6 +297,7 @@ def visit_file(self, file_node: MypyFile, fnam: str, options: Options) -> None:
g.module_public = False

del self.options
del self.fixups

def refresh_partial(self, node: Union[MypyFile, FuncItem]) -> None:
"""Refresh a stale target in fine-grained incremental mode."""
Expand Down Expand Up @@ -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',
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

[self.str_type(), mapping_value_type])
[self.str_type(), self.object_type()])
or self.object_type())

def fixup() -> None:
mapping_value_type = join.join_type_list(types)
fallback.args[1] = mapping_value_type
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can replace it with a comment.


# We can't calculate the complete fallback type until after semantic
# analysis, since otherwise MROs might be incomplete. Postpone a fixup
# function that patches the fallback.
self.fixups.append(fixup)

info = self.basic_new_typeinfo(name, fallback)
info.typeddict_type = TypedDictType(OrderedDict(zip(items, types)), fallback)

Expand Down
1 change: 1 addition & 0 deletions mypy/server/update.py
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ def build_incremental_step(manager: BuildManager,
# TODO: state.fix_suppressed_dependencies()?
state.semantic_analysis()
state.semantic_analysis_pass_three()
# TODO: state.semantic_analysis_fixups()
state.type_check_first_pass()
# TODO: state.type_check_second_pass()?
state.finish_passes()
Expand Down
40 changes: 40 additions & 0 deletions test-data/unit/check-typeddict.test
Original file line number Diff line number Diff line change
Expand Up @@ -813,3 +813,43 @@ 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
Copy link
Member

Choose a reason for hiding this comment

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

Just have few comments:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good comments! My responses:

  • This seems to remove the only remaining join during semantic analysis. Updated fixed issues in PR description.
  • Added Use join to calculate better fallback for tuples #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.


[case testForwardReferenceInTypedDict]
from typing import Mapping
from mypy_extensions import TypedDict
X = TypedDict('X', {'b': 'B', 'c': 'C'})
class B: pass
class C(B): pass
x: X
reveal_type(x) # E: Revealed type is 'TypedDict(b=__main__.B, c=__main__.C, _fallback=__main__.X)'
m1: Mapping[str, B] = x
m2: Mapping[str, C] = x # E: Incompatible types in assignment (expression has type "X", variable has type Mapping[str, C])
[builtins fixtures/dict.pyi]

[case testForwardReferenceInClassTypedDict]
from typing import Mapping
from mypy_extensions import TypedDict
class X(TypedDict):
b: 'B'
c: 'C'
class B: pass
class C(B): pass
x: X
reveal_type(x) # E: Revealed type is 'TypedDict(b=__main__.B, c=__main__.C, _fallback=__main__.X)'
m1: Mapping[str, B] = x
m2: Mapping[str, C] = x # E: Incompatible types in assignment (expression has type "X", variable has type Mapping[str, C])
[builtins fixtures/dict.pyi]

[case testForwardReferenceToTypedDictInTypedDict]
from typing import Mapping
from mypy_extensions import TypedDict
# Forward references don't quite work yet
X = TypedDict('X', {'a': 'A'}) # E: Invalid type "__main__.A"
A = TypedDict('A', {'b': int})
x: X
reveal_type(x) # E: Revealed type is 'TypedDict(a=TypedDict(b=builtins.int, _fallback=__main__.A), _fallback=__main__.X)'
reveal_type(x['a']['b']) # E: Revealed type is 'builtins.int'
[builtins fixtures/dict.pyi]