Skip to content

Make FakeInfo a more general-purpose TypeInfo placeholder #5469

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 5 commits into from
Aug 14, 2018
Merged

Conversation

msullivan
Copy link
Collaborator

There are a lot of places in mypy currently that shove None into
fields expecting TypeInfo. Since mypyc does not appreciate this kind
of behavior, use FakeInfo instances instead of None. FakeInfo is
generalized to allow different assertion messages and to override
__bool__ so that it can be conveniently checked against.

There are a lot of places in mypy currently that shove None into
fields expecting TypeInfo. Since mypyc does not appreciate this kind
of behavior, use FakeInfo instances instead of None. FakeInfo is
generalized to allow different assertion messages and to override
`__bool__` so that it can be conveniently checked against.
Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

Thanks, looks good! I have only minor comments.

mypy/nodes.py Outdated
self.info = FUNC_NO_INFO # type: TypeInfo
self.is_property = False # type: bool
self.is_class = False # type: bool
self.is_static = False # type: bool
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need these three bools 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.

Because referring to FUNC_NO_INFO, which is defined later in the file, causes __init__ to get second-pass typechecked, which causes these to not get inferred right or something.
Annotating the FakeInfos fixes this and the weird subtype.py assert also so I'm doing that instead.

mypy/nodes.py Outdated
@@ -2184,6 +2185,7 @@ def __init__(self, names: 'SymbolTable', defn: ClassDef, module_name: str) -> No
self.assuming_proper = []
self.inferring = []
self.add_type_vars()
self.replaced = TYPEINFO_NO_REPLACED
Copy link
Member

Choose a reason for hiding this comment

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

I would actually make this Optional, it is used in few places, so should be easy to fix. Also I think we have an issue about removing replaced altogether.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, that was easy.

mypy/nodes.py Outdated
if (isinstance(node, Var) and node.type is not None):
return node.type
# mypy thinks this branch is unreachable but it is wrong
elif (isinstance(node, FuncBase) and node.type is not None):
Copy link
Member

Choose a reason for hiding this comment

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

Hm, is this a bug then? Do we have an issue for this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's the "intersection types bug". #3603. I'll point to it in the comments

mypy/subtypes.py Outdated
@@ -560,6 +560,7 @@ def find_node_type(node: Union[Var, FuncBase], itype: Instance, subtype: Type) -
else:
typ = signature
itype = map_instance_to_supertype(itype, node.info)
assert typ is not None
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be redundant, could you please add a comment why this is needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a knock-on effect of Var's __init__ getting typechecked in the second pass I think. But it is pretty bogus and probably a mypy bug, but I don't think I have the cycles right now to track it down. Annotating the FakeInfos fixes the problem, though. SIgh.

mypy/typeanal.py Outdated
@@ -413,7 +413,8 @@ def visit_callable_type(self, t: CallableType, nested: bool = True) -> Type:
variables = self.bind_function_type_variables(t, t)
ret = t.copy_modified(arg_types=self.anal_array(t.arg_types, nested=nested),
ret_type=self.anal_type(t.ret_type, nested=nested),
fallback=t.fallback or self.named_type('builtins.function'),
fallback=(t.fallback if t.fallback.type
Copy link
Member

Choose a reason for hiding this comment

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

This pattern is a bit confusing, I would add a little remainder here that FakeInfo is Falsey. (maybe also below?)

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes!

@msullivan msullivan merged commit e43eb89 into master Aug 14, 2018
@msullivan msullivan deleted the fakeinfo branch August 14, 2018 19:18
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.

2 participants