Skip to content

[clean strict optional] Use dummy TypeInfo instead of None in Instance.type (and few more fixes) #3285

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

Merged
merged 7 commits into from
Apr 30, 2017
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 1 addition & 1 deletion mypy/checkmember.py
Original file line number Diff line number Diff line change
Expand Up @@ -451,7 +451,7 @@ class B(A): pass
info = itype.type # type: TypeInfo
if isinstance(t, CallableType):
# TODO: Should we propagate type variable values?
tvars = [TypeVarDef(n, i + 1, None, builtin_type('builtins.object'), tv.variance)
tvars = [TypeVarDef(n, i + 1, [], builtin_type('builtins.object'), tv.variance)
for (i, n), tv in zip(enumerate(info.type_vars), info.defn.type_vars)]
if is_classmethod:
t = bind_self(t, original_type)
Expand Down
6 changes: 3 additions & 3 deletions mypy/fixup.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
from mypy.types import (
CallableType, EllipsisType, Instance, Overloaded, TupleType, TypedDictType,
TypeList, TypeVarType, UnboundType, UnionType, TypeVisitor,
TypeType
TypeType, NOT_READY
)
from mypy.visitor import NodeVisitor

Expand Down Expand Up @@ -156,7 +156,7 @@ def visit_instance(self, inst: Instance) -> None:
# TODO: Is this needed or redundant?
# Also fix up the bases, just in case.
for base in inst.type.bases:
if base.type is None:
if base.type is NOT_READY:
base.accept(self)
for a in inst.args:
a.accept(self)
Expand Down Expand Up @@ -233,7 +233,7 @@ def visit_type_type(self, t: TypeType) -> None:


def lookup_qualified(modules: Dict[str, MypyFile], name: str,
quick_and_dirty: bool) -> SymbolNode:
quick_and_dirty: bool) -> Optional[SymbolNode]:
stnode = lookup_qualified_stnode(modules, name, quick_and_dirty)
if stnode is None:
return None
Expand Down
10 changes: 2 additions & 8 deletions mypy/maptype.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,7 @@ def map_instance_to_supertypes(instance: Instance,
# FIX: Currently we should only have one supertype per interface, so no
# need to return an array
result = [] # type: List[Instance]
typ = instance.type
assert typ is not None, 'Instance.type is not fixed after deserialization'
for path in class_derivation_paths(typ, supertype):
for path in class_derivation_paths(instance.type, supertype):
types = [instance]
for sup in path:
a = [] # type: List[Instance]
Expand Down Expand Up @@ -60,7 +58,6 @@ def class_derivation_paths(typ: TypeInfo,

for base in typ.bases:
btype = base.type
assert btype is not None, 'Instance.type is not fixed after deserialization'
if btype == supertype:
result.append([btype])
else:
Expand All @@ -75,7 +72,6 @@ def map_instance_to_direct_supertypes(instance: Instance,
supertype: TypeInfo) -> List[Instance]:
# FIX: There should only be one supertypes, always.
typ = instance.type
assert typ is not None, 'Instance.type is not fixed after deserialization'
result = [] # type: List[Instance]

for b in typ.bases:
Expand Down Expand Up @@ -103,6 +99,4 @@ def instance_to_type_environment(instance: Instance) -> Dict[TypeVarId, Type]:
arguments. The type variables are mapped by their `id`.

"""
typ = instance.type
assert typ is not None, 'Instance.type is not fixed after deserialization'
return {binder.id: arg for binder, arg in zip(typ.defn.type_vars, instance.args)}
return {binder.id: arg for binder, arg in zip(instance.type.defn.type_vars, instance.args)}
17 changes: 17 additions & 0 deletions mypy/nodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -2208,6 +2208,23 @@ def deserialize(cls, data: JsonDict) -> 'TypeInfo':
return ti


class FakeInfo(TypeInfo):
# types.py defines a single instance of this class, called types.NOT_READY.
# This instance is used as a temporary placeholder in the process of de-serialization
# of 'Instance' types. The de-serialization happens in two steps: In the first step,
# Instance.type is set to NOT_READY. In the second step (in fixup.py) it is replaced by
# an actual TypeInfo. If you see the assertion error below, then most probably something
# went wrong during the second step and an 'Instance' that raised this error was not fixed.
# Note:
# 'None' is not used as a dummy value for two reasons:
# 1. This will require around 80-100 asserts to make 'mypy --strict-optional mypy'
# pass cleanly.
# 2. If NOT_READY value is accidentally used somewhere, it will be obvious where the value
# is from, whereas a 'None' value could come from anywhere.
Copy link
Member

Choose a reason for hiding this comment

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

Excellent reasons!

def __getattr__(self, attr: str) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ilevkivskyi @gvanrossum This should probably be __getattribute__ instead. Now attributes defined in the body of TypeInfo will not trigger an AssertionError, as far as I know, and thus FakeInfo can mask some actual errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

In any case __getattribute__ looks more robust. Will make a PR soon.

Copy link
Member

Choose a reason for hiding this comment

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

OK, but then I suspect that #3281 will come back. So we should at least have a test in place to verify it doesn't. (Because that's the kind of error that's being masked here -- and since in that particular case it was just getting the wrong full name of some type for some error message I'd rather have a poor error than a crash -- our users really don't like crashes.

Copy link
Member Author

Choose a reason for hiding this comment

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

@gvanrossum
Yes, #3281 comes back, I tried your crash scenario and it indeed crashes (although in a different place). But it looks like Jukka's idea works! At least I have found two actual bugs in fixup.py, both related to metaclass de-serialization. After fixing those, your scenario from #3281 doe snot crash anymore. I will make a PR now so that you can try it.

raise AssertionError('De-serialization failure: TypeInfo not fixed')


class SymbolTableNode:
# Kind of node. Possible values:
# - LDEF: local definition (of any kind)
Expand Down
6 changes: 3 additions & 3 deletions mypy/semanal.py
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ class SemanticAnalyzer(NodeVisitor):
# Nested block depths of scopes
block_depth = None # type: List[int]
# TypeInfo of directly enclosing class (or None)
type = None # type: TypeInfo
type = None # type: Optional[TypeInfo]
# Stack of outer classes (the second tuple item contains tvars).
type_stack = None # type: List[TypeInfo]
# Type variables that are bound by the directly enclosing class
Expand Down Expand Up @@ -1745,10 +1745,10 @@ def build_newtype_typeinfo(self, name: str, old_type: Type, base_type: Instance)
info.is_newtype = True

# Add __init__ method
args = [Argument(Var('cls'), NoneTyp(), None, ARG_POS),
args = [Argument(Var('self'), NoneTyp(), None, ARG_POS),
self.make_argument('item', old_type)]
signature = CallableType(
arg_types=[cast(Type, None), old_type],
arg_types=[Instance(info, []), old_type],
arg_kinds=[arg.kind for arg in args],
arg_names=['self', 'item'],
ret_type=old_type,
Expand Down
8 changes: 5 additions & 3 deletions mypy/subtypes.py
Original file line number Diff line number Diff line change
Expand Up @@ -323,9 +323,11 @@ def is_callable_subtype(left: CallableType, right: CallableType,

if left.variables:
# Apply generic type variables away in left via type inference.
left = unify_generic_callable(left, right, ignore_return=ignore_return)
if left is None:
unified = unify_generic_callable(left, right, ignore_return=ignore_return)
if unified is None:
return False
else:
left = unified

# Check return types.
if not ignore_return and not is_compat(left.ret_type, right.ret_type):
Expand Down Expand Up @@ -493,7 +495,7 @@ def are_args_compatible(


def unify_generic_callable(type: CallableType, target: CallableType,
ignore_return: bool) -> CallableType:
ignore_return: bool) -> Optional[CallableType]:
"""Try to unify a generic callable type with another callable type.

Return unified CallableType if successful; otherwise, return None.
Expand Down
8 changes: 4 additions & 4 deletions mypy/test/testtypes.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,18 +75,18 @@ def test_tuple_type(self) -> None:
assert_equal(str(TupleType([self.x, AnyType()], None)), 'Tuple[X?, Any]')

def test_type_variable_binding(self) -> None:
assert_equal(str(TypeVarDef('X', 1, None, self.fx.o)), 'X')
assert_equal(str(TypeVarDef('X', 1, [], self.fx.o)), 'X')
assert_equal(str(TypeVarDef('X', 1, [self.x, self.y], self.fx.o)),
'X in (X?, Y?)')

def test_generic_function_type(self) -> None:
c = CallableType([self.x, self.y], [ARG_POS, ARG_POS], [None, None],
self.y, self.function, name=None,
variables=[TypeVarDef('X', -1, None, self.fx.o)])
variables=[TypeVarDef('X', -1, [], self.fx.o)])
assert_equal(str(c), 'def [X] (X?, Y?) -> Y?')

v = [TypeVarDef('Y', -1, None, self.fx.o),
TypeVarDef('X', -2, None, self.fx.o)]
v = [TypeVarDef('Y', -1, [], self.fx.o),
TypeVarDef('X', -2, [], self.fx.o)]
c2 = CallableType([], [], [], NoneTyp(), self.function, name=None, variables=v)
assert_equal(str(c2), 'def [Y, X] ()')

Expand Down
2 changes: 1 addition & 1 deletion mypy/typefixture.py
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ def make_type_info(self, name: str,
variance = variances[id - 1]
else:
variance = COVARIANT
v.append(TypeVarDef(n, id, None, self.o, variance=variance))
v.append(TypeVarDef(n, id, [], self.o, variance=variance))
class_def.type_vars = v

info = TypeInfo(SymbolTable(), class_def, module_name)
Expand Down
Loading