Remove/assert casts #2272
Remove/assert casts #2272
Conversation
ddfisher
left a comment
ddfisher
left a comment
There was a problem hiding this comment.
Thanks for these changes! They look largely good. There's an additional fixup to be made here, though: mypy understands assert isinstance(...), so basically all the casts made after the assert guards you added can be removed. Could you make that change?
mypy/checkmember.py
Outdated
| # A property cannot have an overloaded type => the cast | ||
| # is fine. | ||
| assert isinstance(signature, CallableType) | ||
| return cast(CallableType, signature).ret_type |
There was a problem hiding this comment.
I checked this cast -- we can remove it.
mypy/semanal.py
Outdated
| if defn.type: | ||
| functype = cast(CallableType, defn.type) | ||
| typevars = self.infer_type_variables(functype) | ||
| if isinstance(defn.type, CallableType): |
There was a problem hiding this comment.
I think this might be cleaner with an assert instead. This changes the meaning a bit.
mypy/semanal.py
Outdated
| node = self.lookup_qualified(name, type) | ||
| if node and node.kind == UNBOUND_TVAR: | ||
| assert isinstance(node.node, TypeVarExpr) | ||
| result.append((name, cast(TypeVarExpr, node.node))) |
There was a problem hiding this comment.
This cast can be removed.
mypy/semanal.py
Outdated
| b = self.globals.get('__builtins__', None) | ||
| if b: | ||
| assert isinstance(b.node, MypyFile) | ||
| table = cast(MypyFile, b.node).names |
There was a problem hiding this comment.
This cast can likely be removed.
|
Thanks for the review. I left some casts thinking they may serve as a warning sign to the reader, but of course |
| assert isinstance(node.node, TypeInfo) | ||
| return Instance(node.node, []) | ||
|
|
||
| def lookup_fully_qualified(self, name: str) -> SymbolTableNode: |
There was a problem hiding this comment.
This refactoring is not really related to the casts, but why not?
There was a problem hiding this comment.
Actually, this PR is pretty big already (and is not without risk of breakage -- changing casts to asserts potentially changes runtime behavior), so I'd prefer to keep it as focused as possible. If you'd like to make this refactor, could you submit it as a separate small PR?
|
@ddfisher I think it's done. Please notice the change in how errors are reported in |
ddfisher
left a comment
ddfisher
left a comment
There was a problem hiding this comment.
Thanks for fixing that up. The refactor of apply_generic_arguments doesn't look correct to me -- it looks like mypy actually depends on that cast for some error conditions. For that reason, the casts involving that call should not have asserts. The behavior is hacky, but is a larger project to clean up, which should not be part of this PR.
In general, it's a lot easier to review PRs if they stay focused, even though it's tempting to fix other problems discovered while writing the PR. It's better to put those in a separate follow-up PR instead. Thanks!
| assert isinstance(node.node, TypeInfo) | ||
| return Instance(node.node, []) | ||
|
|
||
| def lookup_fully_qualified(self, name: str) -> SymbolTableNode: |
There was a problem hiding this comment.
Actually, this PR is pretty big already (and is not without risk of breakage -- changing casts to asserts potentially changes runtime behavior), so I'd prefer to keep it as focused as possible. If you'd like to make this refactor, could you submit it as a separate small PR?
|
@ddfisher I don't understand your comment about |
mypy/subtypes.py
Outdated
| msg = messages.temp_message_builder() | ||
| applied = mypy.applytype.apply_generic_arguments(type, inferred_vars, msg, context=target) | ||
| if msg.is_errors() or not isinstance(applied, CallableType): | ||
| if applied is None or msg.is_errors(): |
There was a problem hiding this comment.
not isinstance(applied, CallableType) before if and only if applied is None now.
not msg.iserrors() also implies not isinstance(applied, AnyType) and therefore isinstance(applied, CallableType).
Am I wrong?
|
Could you split this up in smaller portions, e.g. one PR per file? |
…ngth is an assumption, not a runtime check
|
Working on it |
|
Changes to 9 files remain in this PR, but each file has 1-2 uncomplicated changes. |
|
Waiting for @ddfisher to finish the review. |
mypy/checker.py
Outdated
| assert isinstance(inferred, Var) | ||
| else: | ||
| m = cast(MemberExpr, lvalue) | ||
| assert isinstance(m, MemberExpr) |
There was a problem hiding this comment.
Shouldn't these lines be reversed to get rid of a cast?
There was a problem hiding this comment.
Yes. I missed these.
mypy/typeanal.py
Outdated
| def builtin_type(self, fully_qualified_name: str, args: List[Type] = None) -> Instance: | ||
| node = self.lookup_fqn_func(fully_qualified_name) | ||
| assert isinstance(node.node, TypeInfo) | ||
| info = cast(TypeInfo, node.node) |
There was a problem hiding this comment.
You should be able to get rid of this cast (and could get rid of info entirely).
Part of #2272. The only place where the number of type arguments in type application might not match is when the user does that; this is explicitly checked in visit_type_application. Other places involve type inference, which must be correct "by construction" and if it isn't, that's a bug we want to catch, instead of returning AnyType. It also allows us to remove casts.
|
This looks good now, thanks! It looks like there's a small merge conflict to fix, though. |
Conflicts: mypy/expandtype.py
|
Fixed. Thanks. |
|
Thanks! I liked all of these improvements. Death to casts! |
4 participants
Remove a cast to Any and some others, and assert-guard many casts.
The patch for each file is independent of the others. I think the only non-immediate change is in semanal.py.