Make strict Optional type system changes standard #3024
Conversation
ddfisher
force-pushed
the
per-file-strict-optional
branch
from
06ae294
to
c0350ca
Compare
ddfisher
force-pushed
the
per-file-strict-optional
branch
from
c0350ca
to
9f73487
Compare
ddfisher
force-pushed
the
per-file-strict-optional
branch
from
9f73487
to
5c44790
Compare
| [out] | ||
| main:6: error: Return type of "f" incompatible with supertype "A" | ||
| main:7: error: Return type of "g" incompatible with supertype "A" |
There was a problem hiding this comment.
Why is this no longer an error?
There was a problem hiding this comment.
None is a subtype of A, so this is valid.
There was a problem hiding this comment.
Well, only without --strict-optional, or if the base class has -> Optional[A].
There was a problem hiding this comment.
Sure, but --strict-optional is still not enabled by default.
There was a problem hiding this comment.
Ah, I was confused by the PR title (and didn't read the whole thing).
mypy/checker.py
Outdated
| # Functions returning a value of type None are allowed to have a None return. | ||
| if isinstance(self.scope.top_function(), FuncExpr) or isinstance(typ, NoneTyp): | ||
| if is_lambda or isinstance(typ, NoneTyp): |
There was a problem hiding this comment.
Added some comments
test-data/unit/check-inference.test
Outdated
| @@ -420,7 +420,8 @@ class A: pass | |||
| a = None # type: A | |||
|
|
|||
| def ff() -> None: | |||
| x = f() # E: Need type annotation for variable | |||
| x = f() | |||
| reveal_type(x) | |||
There was a problem hiding this comment.
There's no output from reveal_type. The desired behavior would probably be to require an annotation for x.
There was a problem hiding this comment.
Filed issue for this instead: #3032.
| @@ -430,19 +431,6 @@ def f() -> T: pass | |||
| def g(a: T) -> None: pass | |||
| [out] | |||
|
|
|||
| [case testUnsolvableInferenceResult] | |||
There was a problem hiding this comment.
Consider enabling this test case and renaming it if the output doesn't correspond to the original intent of the test case.
There was a problem hiding this comment.
This is a test involving void and is no longer relevant. It no longer tests anything interesting that is untested elsewhere.
| @@ -840,7 +828,7 @@ for x in [A()]: | |||
| b = x # E: Incompatible types in assignment (expression has type "A", variable has type "B") | |||
| a = x | |||
|
|
|||
| for y in []: # E: Need type annotation for variable | |||
| for y in []: | |||
There was a problem hiding this comment.
What's the type of y here? I think that the original message was fine. At least add a reveal_type.
There was a problem hiding this comment.
y is inferred as None. Added a reveal_type.
test-data/unit/check-inference.test
Outdated
| @@ -886,7 +873,7 @@ for x, y in [[A()]]: | |||
| a = x | |||
| a = y | |||
|
|
|||
| for e, f in [[]]: # E: Need type annotation for variable | |||
| for e, f in [[]]: | |||
There was a problem hiding this comment.
Similar to above; at least add reveal_types.
| @@ -1419,7 +1406,8 @@ def f() -> None: | |||
| [builtins fixtures/list.pyi] | |||
| [out] | |||
|
|
|||
| [case testPartiallyInitializedToNoneAndThenToIncompleteType] | |||
| [case testPartiallyInitializedToNoneAndThenToIncompleteType-skip] | |||
| # TODO(ddfisher): fix partial type bug and re-enable | |||
There was a problem hiding this comment.
Add an issue for this so that we won't lose track of it.
| @@ -269,15 +269,16 @@ def greet() -> 'Generator[Any, None, None]': | |||
| @asyncio.coroutine | |||
| def test() -> 'Generator[Any, None, None]': | |||
| yield from greet() | |||
| x = yield from greet() # Error | |||
| x = yield from greet() | |||
There was a problem hiding this comment.
For consistency, we should retain the original error message (... does not return a value).
After this lands, per-file strict Optional should be straightforward. This should fix a lot of bugs around None. Introduces a small bug with partial types (where mypy is overly permissive) which will be fixed in a follow up PR.