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
| def g(self) -> None: pass | ||
| [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).
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
Nice! This simplifies things a lot and improves consistency.
| # Lambdas are allowed to have None returns. | ||
| # 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
| 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.
| 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.
| 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.
| 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.
|
|
||
| [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.
| 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.