[clean-strict-optional] Clean-up the type checking files #3957

ilevkivskyi · 2017-09-14T18:35:53Z

This makes another major piece of mypy --strict-optional clean: the type checking files.
After this PR there will be only single exception left semanal.py.

gvanrossum

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.

gvanrossum · 2017-09-14T23:56:08Z

mypy/checker.py

@@ -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):


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>

gvanrossum · 2017-09-14T23:57:15Z

mypy/checker.py

-                    self.check_getattr_method(typ, defn, name)
-                elif name == '__setattr__':
-                    self.check_setattr_method(typ, defn)
+                if name:  # Special method names


Insert a blank line before this block, and another one before the next block at the same level.

gvanrossum · 2017-09-15T00:00:18Z

mypy/checker.py

@@ -1454,7 +1456,7 @@ def check_compatibility_super(self, lvalue: NameExpr, lvalue_type: Optional[Type
        # lvalue had a type defined; this is handled by other
        # 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]


This annotation seems unnecessary (partial types do their thing here).

gvanrossum · 2017-09-15T00:11:50Z

mypy/checkexpr.py

@@ -217,6 +218,7 @@ def visit_call_expr(self, e: CallExpr, allow_none_return: bool = False) -> Type:
                    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 again...

gvanrossum · 2017-09-15T00:12:39Z

mypy/checkexpr.py

@@ -201,11 +201,12 @@ def visit_call_expr(self, e: CallExpr, allow_none_return: bool = False) -> Type:
                and len(e.args) == 2):
            for typ in mypy.checker.flatten(e.args[1]):
                if isinstance(typ, NameExpr):
+                    node = None  # type: Optional[SymbolTableNode]


Again, partial types will do their thing here.

gvanrossum · 2017-09-15T01:53:52Z

mypy/checkexpr.py

-        stride = None  # type:int
+        begin = None  # type: Optional[int]
+        end = None  # type: Optional[int]
+        stride = None  # type: Optional[int]


These three # type annotations seem unnecessary too...

gvanrossum · 2017-09-15T01:55:29Z

mypy/checkexpr.py

@@ -1943,6 +1960,7 @@ def visit_dict_expr(self, e: DictExpr) -> Type:
        vtdef = TypeVarDef('VT', -2, [], self.object_type())
        kt = TypeVarType(ktdef)
        vt = TypeVarType(vtdef)
+        rv = None  # type: Optional[Type]


gvanrossum · 2017-09-15T01:57:05Z

mypy/checkmember.py

@@ -630,7 +630,7 @@ class B(A): pass

    """
    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()]))


Wow, was that a bug?

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).

gvanrossum · 2017-09-15T02:01:24Z

mypy/myunit/__init__.py

@@ -112,7 +112,7 @@ def __init__(self, name: str, suite: 'Optional[Suite]' = None,
        self.name = name
        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]]


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...

Yes, this is a good case for PEP 563.

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?

gvanrossum · 2017-09-15T02:03:04Z

mypy_self_check.ini

-[mypy-mypy.checkexpr]
-strict_optional = False
-
+; historical exception


I prefer # as comment character in .ini files.

gvanrossum

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?

gvanrossum · 2017-09-17T17:23:11Z

(I've restarted the AppVeyor build, we'll see how it fares.)

gvanrossum · 2017-09-17T21:23:44Z

No, still the same failure. :-(

emmatyping · 2017-09-17T22:00:21Z

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.

ilevkivskyi · 2017-09-17T22:32:13Z

OK, all tests pass now.

ilevkivskyi added 3 commits September 14, 2017 13:20

Clean-up most of checker; started checkexpr

178ad41

More clean-up

9d77fbc

Finish clean-up of checker, checkexpr and myunit

1f73106

gvanrossum reviewed Sep 15, 2017

View reviewed changes

Address CR

218253a

gvanrossum approved these changes Sep 17, 2017

View reviewed changes

emmatyping mentioned this pull request Sep 17, 2017

Require LXML <4.0 #3965

Merged

Restart CI builds

d461fc1

ilevkivskyi merged commit 356454a into python:master Sep 17, 2017

ilevkivskyi deleted the clean-optional-checker branch September 17, 2017 22:33

ilevkivskyi mentioned this pull request Sep 17, 2017

Potential bug in per-file strict-optional #3966

Closed

elazarg mentioned this pull request May 20, 2018

Add self-check to pytests #5087

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clean-strict-optional] Clean-up the type checking files #3957

[clean-strict-optional] Clean-up the type checking files #3957

ilevkivskyi commented Sep 14, 2017

gvanrossum left a comment

gvanrossum Sep 14, 2017

gvanrossum Sep 14, 2017

gvanrossum Sep 15, 2017

gvanrossum Sep 15, 2017

gvanrossum Sep 15, 2017

gvanrossum Sep 15, 2017

gvanrossum Sep 15, 2017

gvanrossum Sep 15, 2017

elazarg Sep 15, 2017

gvanrossum Sep 15, 2017

ilevkivskyi Sep 17, 2017

elazarg May 20, 2018

gvanrossum Sep 15, 2017

gvanrossum left a comment

gvanrossum commented Sep 17, 2017 via email

gvanrossum commented Sep 17, 2017

emmatyping commented Sep 17, 2017

ilevkivskyi commented Sep 17, 2017

[clean-strict-optional] Clean-up the type checking files #3957

[clean-strict-optional] Clean-up the type checking files #3957

Conversation

ilevkivskyi commented Sep 14, 2017

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvanrossum left a comment

Choose a reason for hiding this comment

gvanrossum commented Sep 17, 2017 via email

gvanrossum commented Sep 17, 2017

emmatyping commented Sep 17, 2017

ilevkivskyi commented Sep 17, 2017