Pr/strict optional #1562

ddfisher · 2016-05-20T22:23:11Z

Implementation of #1450. Largely complete -- just missing the ability for non-Optional class vars to be initialized to None.

Just ignore all the crazy commits -- they're still useful history for me, so I'd prefer not to squash yet.

gvanrossum · 2016-05-20T22:26:29Z

Any chance you can resolve the conflicts?

gvanrossum · 2016-05-20T22:42:13Z

mypy/experimental.py

@@ -0,0 +1 @@
+STRICT_OPTIONAL = False


I don't think an adjective make a good module name. Can you make it a noun, e.g. 'experiments'?

ddfisher · 2016-05-20T23:36:04Z

Now allows non-Optional class variables to be initialized to None, though it doesn't check that they are set to something else in init (or a method init calls).

ddfisher · 2016-05-21T01:21:52Z

I've figured out one of the mypy-on-mypy errors this introduces, though it's not entirely clear to me what the best solution is. Basically, the problem boils down to mypy inferring member variable types from isinstance calls but not from assignments. To be concrete:

This results in an error:

from typing import Union
class C:
    x = ""  # type: Union[int, str]
    def f(self) -> None:
        self.x = 1
        self.x + 1  # E: Unsupported operand types for + ("Union[int, str]" and "int")

But this works:

def f(x: Union[int, str]) -> None:
    x = 1
    x + 1

And so does this:

from typing import Union
class C:
    x = ""  # type: Union[int, str]
    def f(self) -> None:
        if isinstance(self.x, int):
            self.x + 1

This problem was exposed because we have code that looks like this:

class C:
    x = ""
    def f(self) -> None:
        if self.x is not None:
            return
        self.x = "foo"
        self.x.capitalize()  # E: None has no attribute "capitalize"

With this PR, mypy now understands that returning when self.x is not None implies that self.x = None for the rest of the function (because it understands foo is not None as an isinstance-style check). Mypy continues to not understand that self.x = "foo" means x becomes a str.

It's not generally sound to assume that instance variables don't change their type within a function after assignment (within the constraints of their declared type), because method calls might mutate them. So it's not obvious what the best fix here is.

really not convinced that the line length is set right

ddfisher · 2016-05-21T20:38:32Z

On further investigation, not inferring local types from member assignment appears to not be intended behavior, but rather the result of a bug. Here's what I've traced so far:

In check_assignment, the binder is correctly called with the locally inferred type on this line: https://github.com/python/mypy/blob/master/mypy/checker.py#L1202
The binder attempts to get the declared type here: https://github.com/python/mypy/blob/master/mypy/checker.py#L232
The MemberExpr has no associated node property, so the check here fails: https://github.com/python/mypy/blob/master/mypy/checker.py#L218
MemberExprs have no associated node property because none is set in the semantic analysis phase here: https://github.com/python/mypy/blob/master/mypy/semanal.py#L1877

rwbarton · 2016-05-22T12:46:15Z

mypy/parse.py

@@ -482,6 +484,9 @@ def parse_function(self, no_type_checks: bool=False) -> FuncDef:
            if is_error:
                return None

+            if typ and isinstance(typ.ret_type, UnboundType):
+                typ.ret_type.ret_type = True


This is confusing; maybe name the new field of UnboundType is_ret_type?

Yeah, that's much better.

rwbarton · 2016-05-22T13:28:44Z

I assume we intend to support non-strict optional checking for the foreseeable future; if not then perhaps the following comments aren't relevant.

Seeing many occurrences of

        if experiments.STRICT_OPTIONAL:
            return UninhabitedType()
        else:
            return NoneTyp()

made me think about the following reorganization. Instead of NoneTyp and UninhabitedType, have BottomType and NoneTyp, where

BottomType is always (regardless of whether strict optional checking is enabled) the bottom of the type hierarchy, so it is a subtype of any other type and taking a Union with BottomType does nothing. It behaves the same as the old NoneTyp.
NoneTyp is the type of None when strict optional checking is enabled. When it's disabled, None has type BottomType (since then None belongs to every type) and NoneTyp is not used for anything.

Ideally the only difference between the two modes would be whether None is given type NoneTyp or type BottomType. This way we'll have a single type hierarchy to think about, and my hope is to remove the dependencies of modules join, meet, solve, and subtypes on the setting of strict optional checking.

I haven't yet read the whole diff carefully, so I'm not sure how this will work out in practice.

rwbarton · 2016-05-22T13:31:21Z

Something we might want to think about before too long is whether it makes sense to check some parts of a program (e.g., some of its dependencies) with strict optional checking and others without, and how we might plan to support that.

gvanrossum · 2016-05-22T21:46:34Z

Honestly I would be happier if this was for a brief period only. But not sure how realistic that I.

ddfisher · 2016-05-23T18:42:53Z

I agree with Guido: I think it would be best to target having this only for a brief period.

@rwbarton: I've thought about your suggestion, and I think it can work, but I actually find the current way of thinking about things to be easier. Making sure BottomType behaves appropriately in both interpretations could get a bit complicated, I think, especially as relates to random weird edge cases in the type system (where things don't completely behave as expected).

ddfisher · 2016-05-24T20:28:09Z

I've updated the new isinstance checking to only be active with --strict-optional enabled so this PR doesn't have to block on #1568, which is a bit complicated.

JukkaL · 2016-05-24T22:56:26Z

mypy/test/data/check-optional.test

+if not x:
+  g(x)  # E: Argument 1 to "g" has incompatible type "Optional[int]"; expected None
+else:
+  f(x)


Reveal the type of x here to make sure it's not Any or similar.

JukkaL · 2016-05-30T21:43:40Z

It would be nice to have test cases for various things that have been fixed.

JukkaL · 2016-05-30T21:45:31Z

mypy/checker.py

-    def assign_type(self, expr: Node, type: Type,
+    def assign_type(self, expr: Node,
+                    type: Type,
+                    declared_type: Type,
                    restrict_any: bool = False) -> None:


Add docstring (at least explain declared_type and what it's used for).

JukkaL · 2016-05-30T22:16:06Z

Docstring of NoneTyp doesn't seem to be up to date:

    """The type of 'None'.

    This is only used internally during type inference.  Programs
    cannot declare a variable of this type, and the type checker
    refuses to infer this type for a variable. However, subexpressions
    often have this type. Note that this is not used as the result
    type when calling a function with a void type, even though
    semantically such a function returns a None value; the void type
    is used instead so that we can report an error if the caller tries
    to do anything with the return value.
    """

JukkaL · 2016-05-30T22:24:51Z

I'd like a more detailed docstring for UninhabitedType. For example, it could answer questions like these:

Why do we have this type? What is is used for?
What is it's relationship with NoneTyp?
Why isn't this called the bottom type? Is it somehow different from a bottom type? If not, mention that this is the same as bottom type.
How do various type operations work with NoneTyp? For example, how do meet, join and subtyping work? I think it's worth describing them here (but not in too much detail) as the type is a little unusual.
Can a variable have this type as its inferred type?

Also, add unit tests to mypy/test/testtypes.py that test various type operations with UninhabitedType.

JukkaL · 2016-05-30T23:44:31Z

One additional thing to test: see if overloaded functions work with None. If this doesn't work, it's okay to create a separate issue for it.

JukkaL · 2016-06-10T14:37:49Z

mypy/test/data/check-inference.test

@@ -1415,13 +1415,16 @@ main: note: In member "f" of class "A":
 [case testAttributePartiallyInitializedToNoneWithMissingAnnotation]
 class A:
    def f(self) -> None:
-        self.x = None # E: Need type annotation for variable
+        self.x = None


This should still produce Need type annotation for variable. Reasoning:

Strict optional checking really shouldn't have an effect on this. This change seems unrelated to the rest of the PR.

There's not enough information in the method scope to infer a non-None type for the attribute. I wouldn't like to infer the type of an attribute from two method bodies (type inference should be local for predictability).

Also, it would be useful to have a similar test case with strict optional enabled.

The Need type annotation for variable error didn't go away, it just needed to be moved to the [out] section. The only thing that changed is that we can now infer that x is int for the body of g(), so there's an additional int not callable error.

With strict optional checking enabled, None will be inferred as a type if no other information is available other than the None assignment (and this is already tested).

JukkaL · 2016-06-10T14:42:02Z

This is good to rebase and land after resolving the 'Need type annotation for variable' issue.

First pass strict Optional checking. Adds the experimental `--strict-optional` flag. Fixes #1450. With --strict-optional: - "None" in type annotations refers to NoneTyp, except as a return value, where it still refers to Void - None and List[None] will now be inferred as a type if no other information is available. - Class variables may be initialized to None without having an Optional type. Mypy does not currently check that they're assigned to in __init__ or elsewhere before use. See #1450 for more details. This also fixes the bug where mypy didn't understand that x.y = "foo" implied that x.y would be a str for the remaineder of that block. This isn't entirely sound, but is in line with the way we do isinstance checks.

ddfisher added 8 commits May 6, 2016 16:12

Basic strict Optional checking

3dcb752

WIP

30f3a0f

cleanup

b5e8433

the return of uninhabitedtype

8f8fb49

fix tests WIP

8c0577a

tests, bug fixes, partial types

b136ef5

isinstance checks

e274c13

fixup todos

6047860

gvanrossum reviewed May 20, 2016
View reviewed changes

allow None class variable declarations

d60c93f

ddfisher added 3 commits May 20, 2016 16:37

Merge branch 'master' into PR/strict-optional

48f9311

Rename experimental -> experiments

07cce60

add missing type annotations

15cad44

fix lint errors

53ebea0

really not convinced that the line length is set right

rwbarton reviewed May 22, 2016
View reviewed changes

ddfisher added 2 commits May 23, 2016 11:46

rename UnboundType.ret_type -> is_ret_type

5625c75

fix lack of type narrowing for member variables

157f57d

ddfisher mentioned this pull request May 24, 2016

isinstance checks cause type declarations to leak from the binder #1568

Closed

ddfisher added 2 commits May 24, 2016 13:18

gate new isinstance checks behind experiments flag

0a2c4e1

fix tests broken by member type variable narrowing

b6f6032

JukkaL reviewed May 24, 2016
View reviewed changes

JukkaL reviewed May 30, 2016
View reviewed changes

gnprice mentioned this pull request Jun 2, 2016

Add support for tuple values on except clauses #1610

Merged

ddfisher added 6 commits June 6, 2016 15:46

Merge branch 'master' into PR/strict-optional

ffd4301

unnest functions

a40636e

Fixup tests; add new test

6661f93

update docstrings

fbbab64

fix lint errors

b9e309f

add test for overloads and fix overloading

87f46a0

JukkaL reviewed Jun 10, 2016
View reviewed changes

ddfisher added 2 commits June 10, 2016 11:07

minor test fixup

7247430

Merge branch 'master' into PR/strict-optional

ba743ec

ddfisher merged commit 1ee2b12 into python:master Jun 10, 2016

ddfisher deleted the PR/strict-optional branch June 10, 2016 18:19

gnprice mentioned this pull request Jul 29, 2016

Error in subclass of generic base at None #1956

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pr/strict optional #1562

Pr/strict optional #1562

ddfisher commented May 20, 2016

gvanrossum commented May 20, 2016

gvanrossum May 20, 2016

ddfisher commented May 20, 2016

ddfisher commented May 21, 2016

ddfisher commented May 21, 2016

rwbarton May 22, 2016

ddfisher May 23, 2016

rwbarton commented May 22, 2016

rwbarton commented May 22, 2016

gvanrossum commented May 22, 2016

ddfisher commented May 23, 2016

ddfisher commented May 24, 2016

JukkaL May 24, 2016

JukkaL commented May 30, 2016

JukkaL May 30, 2016

JukkaL commented May 30, 2016

JukkaL commented May 30, 2016

JukkaL commented May 30, 2016

JukkaL Jun 10, 2016

JukkaL Jun 10, 2016

ddfisher Jun 10, 2016

ddfisher Jun 10, 2016

JukkaL commented Jun 10, 2016

Pr/strict optional #1562

Pr/strict optional #1562

Conversation

ddfisher commented May 20, 2016

gvanrossum commented May 20, 2016

Choose a reason for hiding this comment

ddfisher commented May 20, 2016

ddfisher commented May 21, 2016

ddfisher commented May 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwbarton commented May 22, 2016

rwbarton commented May 22, 2016

gvanrossum commented May 22, 2016

ddfisher commented May 23, 2016

ddfisher commented May 24, 2016

Choose a reason for hiding this comment

JukkaL commented May 30, 2016

Choose a reason for hiding this comment

JukkaL commented May 30, 2016

JukkaL commented May 30, 2016

JukkaL commented May 30, 2016

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

JukkaL commented Jun 10, 2016