Repository navigation
[clean-strict-optional] Clean-up the type checking files - #3957
Conversation
gvanrossum
left a comment
gvanrossum
left a comment
There was a problem hiding this comment.
This actually looks great, but I have a bit of a preference for avoiding variable annotations if mypy can figure them out, and I think I found a whole bunch of them. There's one that looks suspicious though, I fear there's some kind of bug in mypy.
| @@ -610,7 +610,7 @@ def check_func_def(self, defn: FuncItem, typ: CallableType, name: str) -> None: | |||
| # function. In the first case, set up another reference with the | |||
| # precise type. | |||
| if isinstance(item, FuncDef): | |||
There was a problem hiding this comment.
This is just begging to be combined with the following if -- I think the whole thing can be collapsed into
if isinstance(item, FuncDef):
fdef = item
# Check if __init__ has an invalid, non-None return type.
<and the rest of that block>| self.check_getattr_method(typ, defn, name) | ||
| elif name == '__setattr__': | ||
| self.check_setattr_method(typ, defn) | ||
| if name: # Special method names |
There was a problem hiding this comment.
Insert a blank line before this block, and another one before the next block at the same level.
| # parts, and all we have to worry about in that case is | ||
| # that lvalue is compatible with the base class. | ||
| compare_node = None # type: Node | ||
| compare_node = None # type: Optional[Node] |
There was a problem hiding this comment.
This annotation seems unnecessary (partial types do their thing here).
| elif typ.node.is_newtype: | ||
| self.msg.fail(messages.CANNOT_ISINSTANCE_NEWTYPE, e) | ||
| self.try_infer_partial_type(e) | ||
| type_context = None # type: Optional[CallableType] |
| and len(e.args) == 2): | ||
| for typ in mypy.checker.flatten(e.args[1]): | ||
| if isinstance(typ, NameExpr): | ||
| node = None # type: Optional[SymbolTableNode] |
There was a problem hiding this comment.
Again, partial types will do their thing here.
| stride = None # type:int | ||
| begin = None # type: Optional[int] | ||
| end = None # type: Optional[int] | ||
| stride = None # type: Optional[int] |
There was a problem hiding this comment.
These three # type annotations seem unnecessary too...
| vtdef = TypeVarDef('VT', -2, [], self.object_type()) | ||
| kt = TypeVarType(ktdef) | ||
| vt = TypeVarType(vtdef) | ||
| rv = None # type: Optional[Type] |
| """ | ||
| if isinstance(method, Overloaded): | ||
| return cast(F, Overloaded([bind_self(c, method) for c in method.items()])) | ||
| return cast(F, Overloaded([bind_self(c, original_type) for c in method.items()])) |
There was a problem hiding this comment.
Indeed. I recall fixing it in at some point, but probably only on a local branch.
The reason it hadn't surface (AFAIK) is that it requires an overloaded method with generic self that's also has as bound a supertype of the class, without overlapping the other overload. This ish is a rare combination which is hard to get right anyway, and the checking for the "being a supertype" part is too strict in the case of overload, so I actually struggle to make an example where this is the only error.
from typing import TypeVar, overload
T = TypeVar('T')
class A:
@overload
def f(self: T) -> T: return self
def f(self): return self
reveal_type(A().f())result:
../tmp.py:6: error: Single overload definition, multiple required
../tmp.py:10: error: Revealed type is 'Overload(def [T] (self: T`9) -> T`9)'
Adding a valid overload is not obvious (to me).
| self.suite = suite | ||
| self.old_cwd = None # type: Optional[str] | ||
| self.tmpdir = None # type: Optional[tempfile.TemporaryDirectory] | ||
| self.tmpdir = None # type: Optional[tempfile.TemporaryDirectory[str]] |
There was a problem hiding this comment.
Totally off-topic: this would be a case where if you converted it to PEP 526 style (tmpdir: Optional[tempfile.TemporaryDirectory[str]] at the class level) it would fail at runtime, because the real tempfile module doesn't declare TemporaryDirectory as generic -- but Łukasz's PEP 563 would save the day...
There was a problem hiding this comment.
Yes, this is a good case for PEP 563.
There was a problem hiding this comment.
This has surfaced in #5087 during self-check:
mypy/test/data.py:232: error: "TemporaryDirectory" expects no type arguments, but 1 given
How did that pass before?
| [mypy-mypy.checkexpr] | ||
| strict_optional = False | ||
|
|
||
| ; historical exception |
There was a problem hiding this comment.
I prefer # as comment character in .ini files.
gvanrossum
left a comment
gvanrossum
left a comment
There was a problem hiding this comment.
LG now. We need to file and investigate (in that order!) the alleged per-file strict-optional bug.
Also, the AppVeyor build failed building some xml thing -- maybe it's a flake and a retry will pass?
|
(I've restarted the AppVeyor build, we'll see how it fares.)
|
|
No, still the same failure. :-( |
|
The issue is there is a new lxml release and there are not Windows wheels out yet, as IIRC they are provided by a third party. For now we should pin to lxml 3.8. I'll make a PR. |
|
OK, all tests pass now. |
This makes another major piece of mypy
--strict-optionalclean: the type checking files.After this PR there will be only single exception left
semanal.py.