Tighter types for strconv, report and check* #2223
Conversation
|
@gvanrossum this is ready. |
|
OK, I'd like to leave this open until after 0.4.5 is released (early next
week we hope).
|
| @@ -64,7 +64,7 @@ | |||
| DeferredNode = NamedTuple( | |||
| 'DeferredNode', | |||
| [ | |||
| ('node', Node), | |||
| ('node', Union[Expression, Statement, FuncItem]), | |||
There was a problem hiding this comment.
Actually I think it can only be FuncItem.
| @@ -209,15 +209,18 @@ def handle_cannot_determine_type(self, name: str, context: Context) -> None: | |||
| else: | |||
| 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. :-(
| """Type check a node in the given type context.""" | ||
| self.type_context.append(type_context) | ||
| try: | ||
| typ = node.accept(self) | ||
| except Exception as err: | ||
| 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 :)
| @@ -1812,7 +1816,7 @@ def visit_decorator(self, e: Decorator) -> Type: | |||
| if d.fullname == 'typing.no_type_check': | |||
| 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.
| 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.
| Callable[[Type], None]]]: | ||
| """Returns a list of tuples of two functions that check whether a replacement is | ||
| of the right type for the specifier. The first functions take a node and checks | ||
| its type in the right type context. The second function just checks a type. | ||
| """ | ||
| 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?
| @@ -101,7 +101,7 @@ def visit_assignment_stmt(self, o: AssignmentStmt) -> None: | |||
| else: | |||
| 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: | |||
|
|
||
| from mypy.util import dump_tagged, short_type | ||
| import mypy.nodes | ||
| from mypy.visitor import NodeVisitor | ||
|
|
||
|
|
||
| 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. |
Branched out #2221: converting "Node" into "Expression" wherever possible.
This change is pretty minimal, since the types are somewhat entangled across the files.