Tighter types for strconv, report and check*#2223
Conversation
elazarg
changed the title
|
@gvanrossum this is ready. |
|
OK, I'd like to leave this open until after 0.4.5 is released (early next
week we hope).
|
gvanrossum
left a comment
gvanrossum
left a comment
There was a problem hiding this comment.
Only a few nits. Overall great improvement!
| 'DeferredNode', | ||
| [ | ||
| ('node', Node), | ||
| ('node', Union[Expression, Statement, FuncItem]), |
There was a problem hiding this comment.
Actually I think it can only be FuncItem.
| self.msg.cannot_determine_type(name, context) | ||
|
|
||
| def accept(self, node: Node, type_context: Type = None) -> Type: | ||
| def accept(self, node: Union[Expression, Statement, FuncItem], |
There was a problem hiding this comment.
Not your fault (except for pointing it out :-), but it is really terrible that this takes either a syntax node or a SymbolNode. Those really aren't like each other. :-(
| report_internal_error(err, self.errors.file, node.line, self.errors, self.options) | ||
| self.type_context.pop() | ||
| self.store_type(node, typ) | ||
| if typ is not None: |
There was a problem hiding this comment.
Explain why this check is needed? store_type() doesn't mind if typ is None, and the assert appears to be unrelated.
There was a problem hiding this comment.
store_type() expects an expression, and node might be a statement, in which case node.accept() returns None (answering your question below regarding del_stmt). So the assertion holds only if typ is None. And I wanted the assertion to actually catch mistakes if there are any, so if isinstance is not good enough.
This (and other issues) will be better handled by a separate ExpressionVisitor and a StatementVisitor, but I did try to keep the change small :)
| e.var.type = AnyType() | ||
| e.var.is_ready = True | ||
| return NoneTyp() | ||
| return None |
There was a problem hiding this comment.
Explain? This changes the return type to Optional[Type], and that will become a problem once we turn in --strict-optional. I sort of see that this function doesn't return a useful type in the other branch either (it just falls off the end) and it seems the return value isn't used, but if this really is just being called for its side effect maybe we should just admit that in its signature? (Or if that's beyond hope let's at least add a clarifying comment.)
There was a problem hiding this comment.
See my comment above. The solution has to be a separate visitor, or at least it makes sense to me.
mypy/checker.py
Outdated
| visitor = TypeTransformVisitor(map) | ||
| ret = defn.accept(visitor) | ||
| assert isinstance(ret, FuncItem) | ||
| return cast(FuncItem, ret) |
There was a problem hiding this comment.
You shouldn't need the cast now that you have the assert. Or does that not work somehow here?
There was a problem hiding this comment.
You are right. I think it's a leftover of some experiment. I'll fix it.
mypy/checkstrformat.py
Outdated
| """ | ||
| checkers = [] # type: List[ Tuple[ Callable[[Node], None], Callable[[Type], None] ] ] | ||
| checkers = [ | ||
| ] # type: List[ Tuple[ Callable[[Expression], None], Callable[[Type], None] ] ] |
There was a problem hiding this comment.
While you're at it can you remove the non-PEP-8 spaces from the type expression?
| items = [lvalue] | ||
| for item in items: | ||
| if hasattr(item, 'is_def') and cast(Any, item).is_def: | ||
| if isinstance(item, RefExpr) and item.is_def: |
mypy/strconv.py
Outdated
|
|
||
| class StrConv(NodeVisitor[str]): | ||
| """Visitor for converting a Node to a human-readable string. | ||
| """Visitor for converting nodes to a human-readable string. |
| @@ -1 +1 @@ | |||
| Subproject commit 0e5003b61e7c0aba8f158f72fe1827fd6dcd4673 | |||
| Subproject commit f90a6d1c0eeda7b2b267d9bbfcc6681f4d9f8168 | |||
There was a problem hiding this comment.
It always happens. I don't really know why, whether it's OK, and how to avoid it (I often use commit -a -m'...').
There was a problem hiding this comment.
Read the man page for git submodule...
It looks like this is just due to to you rebasing to a revision that has an updated typeshed. So you don't have anything to do, this will disappear when I merge.
|
Whoops, I merged before you fixed DeferredNode. I'll fix it myself. |
2 participants
Branched out #2221: converting "Node" into "Expression" wherever possible.
This change is pretty minimal, since the types are somewhat entangled across the files.