10000 Explicit types for bindables ("Tighten Types" continued) by elazarg · Pull Request #2238 · python/mypy · GitHub
[go: up one dir, main page]
Skip to content

Explicit types for bindables ("Tighten Types" continued) #2238

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.

Sign up for GitHub

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 19 commits into from
Mar 27, 2017
Merged

Explicit types for bindables ("Tighten Types" continued) #2238

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
d01d2f1
rename binder api
elazarg Oct 6, 2016
8966207
Merge upstream/master into bindables
elazarg Oct 10, 2016
a99dfcb
Merge remote-tracking branch 'upstream/master' into bindables
elazarg Oct 10, 2016
e742453
update Node->Expression
elazarg Oct 10, 2016
d634539
move literal* to Expression
elazarg Oct 10, 2016
06fe586
make literal_hash an object
elazarg Oct 10, 2016
1f8adb8
Merge remote-tracking branch 'upstream/master' into bindables
elazarg Oct 18, 2016
98167cd
update api call
elazarg Oct 18, 2016
0dab466
fix Key type
elazarg Oct 18, 2016
6f4b29c
remove type: ignore
elazarg Oct 20, 2016
8e9ea44
remove assertion
elazarg Nov 2, 2016
1989243
Merge remote-tracking branch 'upstream/master' into bindables
elazarg Nov 9, 2016
201c744
revert _if_bindable
elazarg Nov 9, 2016
208c16d
make get_declaration a function
elazarg Nov 9, 2016
5f36c72
Merge remote-tracking branch 'upstream/master' into bindables
elazarg Mar 15, 2017
5925fd0
Delete a superfluous line to trigger tests.
gvanrossum Mar 27, 2017
4dfc2c7
Merge remote-tracking branch 'upstream/master' into bindables
elazarg Mar 27, 2017
43e9f03
dodge regression - isinstance(AliasOfTuple)
elazarg Mar 27, 2017
2396458
Merge branch 'bindables' of https://github.com/elazarg/mypy into bind…
elazarg Mar 27, 2017
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
53 changes: 31 additions & 22 deletions mypy/binder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@
from mypy.join import join_simple
from mypy.sametypes import is_same_type

from mypy.nodes import IndexExpr, MemberExpr, NameExpr


BindableTypes = (IndexExpr, MemberExpr, NameExpr)
BindableExpression = Union[IndexExpr, MemberExpr, NameExpr]

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the point of introducing this BindableExpression. It seems to duplicate information with which Expressions have (non-None) literals. It should be fine to just use Expression everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lists of literals have non-None literals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's the branch with literal() and literal_hash() functions:
https://github.com/elazarg/mypy/blob/literal_hash/mypy/nodes.py#L2351-L2421


class Frame(Dict[Key, Type]):
"""A Frame represents a specific point in the execution of a program.
Expand Down Expand Up @@ -92,7 +98,7 @@ def push_frame(self) -> Frame:
self.options_on_return.append([])
return f

def _push(self, key: Key, type: Type, index: int=-1) -> None:
def _put(self, key: Key, type: Type, index: int=-1) -> None:
self.frames[index][key] = type

def _get(self, key: Key, index: int=-1) -> Type:
Expand All @@ -103,19 +109,21 @@ def _get(self, key: Key, index: int=-1) -> Type:
return self.frames[i][key]
return None

def push(self, node: Node, typ: Type) -> None:
if not node.literal:
def put_if_bindable(self, expr: Expression, typ: Type) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the change to put, but _if_bindable doesn't add any information (there's no contrasting put_even_if_not_bindable) and is pretty long.

Basically, put is just the way for the rest of the type checker to pass information into the binder, and I think of it as the binder's business whether it bothers to record a piece of information it is given or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it adds no information only if you know exactly how the binder works. When I read this code I had no idea that some expressions can be simply ignored. I do not like long names, but shorter names feels misleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course it is the binder's business, but the reader should know that it is possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

A docstring (or general documentation on the binder) seems like a better vehicle for that.

if not isinstance(expr, BindableTypes):
return
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not really sure about the need for this.

if not expr.literal:
return
key = node.literal_hash
key = expr.literal_hash
if key not in self.declarations:
self.declarations[key] = self.get_declaration(node)
self.declarations[key] = self.get_declaration(expr)
self._add_dependencies(key)
self._push(key, typ)
self._put(key, typ)

def unreachable(self) -> None:
self.frames[-1].unreachable = True

def get(self, expr: Union[Expression, Var]) -> Type:
def get(self, expr: Expression) -> Type:
return self._get(expr.literal_hash)

def is_unreachable(self) -> bool:
Expand Down Expand Up @@ -163,7 +171,7 @@ def update_from_options(self, frames: List[Frame]) -> bool:
for other in resulting_values[1:]:
type = join_simple(self.declarations[key], type, other)
if not is_same_type(type, current_value):
self._push(key, type)
self._put(key, type)
changed = True

self.frames[-1].unreachable = not frames
Expand All @@ -189,19 +197,20 @@ def pop_frame(self, can_skip: bool, fall_through: int) -> Frame:

return result

def get_declaration(self, expr: Node) -> Type:
def get_declaration(self, expr: BindableExpression) -> Type:
assert not isinstance(expr, SymbolTableNode)
Copy link
Member

Choose a reason for hiding this comment

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

This assert can't trigger given the definition of BindableExpression right? Or am I misunderstanding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a leftover from an old check,
if isinstance(node, (RefExpr, SymbolTableNode)) and isinstance(node.node, Var):
I should remove it.

if isinstance(expr, RefExpr) and isinstance(expr.node, Var):
type = expr.node.type
if isinstance(type, PartialType):
return None
return type
else:
return None
if not isinstance(type, PartialType):
return type
return None

def assign_type(self, expr: Expression,
type: Type,
declared_type: Type,
restrict_any: bool = False) -> None:
def assign_type_if_bindable(self, expr: Expression,
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, _if_bindable seems extraneous.

type: Type,
declared_type: Type,
restrict_any: bool = False) -> None:
if not isinstance(expr, BindableTypes):
return None
Copy link
Contributor

Choose a reason for hiding this comment

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

This check doesn't seem necessary.

if not expr.literal:
return
self.invalidate_dependencies(expr)
Expand All @@ -226,16 +235,16 @@ def assign_type(self, expr: Expression,
and not restrict_any):
pass
elif isinstance(type, AnyType):
self.push(expr, declared_type)
self.put_if_bindable(expr, declared_type)
else:
self.push(expr, type)
self.put_if_bindable(expr, type)

for i in self.try_frames:
# XXX This should probably not copy the entire frame, but
# just copy this variable into a single stored frame.
self.allow_jump(i)

def invalidate_dependencies(self, expr: Expression) -> None:
def invalidate_dependencies(self, expr: BindableExpression) -> None:
"""Invalidate knowledge of types that include expr, but not expr itself.

For example, when expr is foo.bar, invalidate foo.bar.baz.
Expand All @@ -246,7 +255,7 @@ def invalidate_dependencies(self, expr: Expression) -> None:
for dep in self.dependencies.get(expr.literal_hash, set()):
self._cleanse_key(dep)

def most_recent_enclosing_type(self, expr: Expression, type: Type) -> Type:
def most_recent_enclosing_type(self, expr: BindableExpression, type: Type) -> Type:
if isinstance(type, AnyType):
return self.get_declaration(expr)
key = expr.literal_hash
Expand Down
19 changes: 9 additions & 10 deletions mypy/checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1112,10 +1112,10 @@ def check_assignment(self, lvalue: Lvalue, rvalue: Expression, infer_lvalue_type
rvalue_type = self.check_simple_assignment(lvalue_type, rvalue, lvalue)

if rvalue_type and infer_lvalue_type:
self.binder.assign_type(lvalue,
rvalue_type,
lvalue_type,
False)
self.binder.assign_type_if_bindable(lvalue,
rvalue_type,
lvalue_type,
False)
elif index_lvalue:
self.check_indexed_assignment(index_lvalue, rvalue, rvalue)

Expand Down Expand Up @@ -1600,7 +1600,6 @@ def visit_assert_stmt(self, s: AssertStmt) -> Type:

# If this is asserting some isinstance check, bind that type in the following code
true_map, _ = self.find_isinstance_check(s.expr)

self.push_type_map(true_map)

def visit_raise_stmt(self, s: RaiseStmt) -> Type:
Expand Down Expand Up @@ -1830,10 +1829,10 @@ def flatten(t: Expression) -> List[Expression]:
s.expr.accept(self)
for elt in flatten(s.expr):
if isinstance(elt, NameExpr):
self.binder.assign_type(elt,
DeletedType(source=elt.name),
self.binder.get_declaration(elt),
False)
self.binder.assign_type_if_bindable(elt,
DeletedType(source=elt.name),
self.binder.get_declaration(elt),
False)
return None

def visit_decorator(self, e: Decorator) -> Type:
Expand Down Expand Up @@ -2361,7 +2360,7 @@ def push_type_map(self, type_map: Optional[Dict[Expression, Type]]) -> None:
self.binder.unreachable()
else:
for expr, type in type_map.items():
self.binder.push(expr, type)
self.binder.put_if_bindable(expr, type)


# Data structure returned by find_isinstance_check representing
Expand Down
13 changes: 4 additions & 9 deletions mypy/checkexpr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,18 +150,13 @@ def analyze_ref_expr(self, e: RefExpr, lvalue: bool = False) -> Type:
return result

def analyze_var_ref(self, var: Var, context: Context) -> Type:
if not var.type:
if var.type:
return var.type
else:
Copy link
Member

Choose a reason for hiding this comment

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

Just double-checking that what you changed here isn't an accidental merge conflict? You've done more here than just revers the sense of the check and swap the then/else clauses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part is related to #2222 - binder.get() does not accept Var; val is always None.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, I would like to extract literal-handling to a couple of functions, and avoid putting them in the Expression classes. I already have the branch ready. Will you consider it? It does not solve anything, but I think it's nice to have this information in a single place, instead of scattered around.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be worth calling out this "change" explicitly in the commit message, since it does not obviously look like a mere refactoring.

if not var.is_ready and self.chk.in_checked_function():
self.chk.handle_cannot_determine_type(var.name(), context)
# Implicit 'Any' type.
return AnyType()
else:
# Look up local type of variable with type (inferred or explicit).
val = self.chk.binder.get(var)
if val is None:
return var.type
else:
return val

def visit_call_expr(self, e: CallExpr) -> Type:
"""Type check a call expression."""
Expand Down Expand Up @@ -1696,7 +1691,7 @@ def check_for_comp(self, e: Union[GeneratorExpr, DictionaryComprehension]) -> No

if true_map:
for var, type in true_map.items():
self.chk.binder.push(var, type)
self.chk.binder.put_if_bindable(var, type)

def visit_conditional_expr(self, e: ConditionalExpr) -> Type:
cond_type = self.accept(e.cond)
Expand Down
8 changes: 3 additions & 5 deletions mypy/nodes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,11 +101,6 @@ class Node(Context):
line = -1
column = -1

# TODO: Move to Expression
# See [Note Literals and literal_hash] below
literal = LITERAL_NO
literal_hash = None # type: Key

def __str__(self) -> str:
ans = self.accept(mypy.strconv.StrConv())
if ans is None:
Expand Down Expand Up @@ -144,6 +139,9 @@ class Statement(Node):

class Expression(Node):
"""An expression node."""
literal = LITERAL_NO
literal_hash = None # type: Key


# TODO:
# Lvalue = Union['NameExpr', 'MemberExpr', 'IndexExpr', 'SuperExpr', 'StarExpr'
Expand Down
0