New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Last strict optional fixes #4070

Merged
merged 22 commits into from Oct 24, 2017

Conversation

Projects
None yet
4 participants
@ilevkivskyi
Collaborator

ilevkivskyi commented Oct 8, 2017

Fixes #1955

This PR will make mypy completely --strict-optional clean. In addition to signature fixes and other minor fixes (mostly in semanal.py) I introduce a change in mypy as a typechecker, I have noticed a pattern that appears several times, but was not understood by mypy:

x: Optional[int]
if x in [1, 2, 3]:
    reveal_type(x)  # now this will be 'int'

I allow this only for built-in containers such as list, dict, set, frozenset, and tuple, since other types can change the semantics of __contains__. (Also this way it is a small change, a general treatment of Container subclasses will require code churn to get named_type).

UPDATE: The feature part now has a separate PR #4072.

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Oct 8, 2017

Member
Member

gvanrossum commented Oct 8, 2017

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Oct 8, 2017

Collaborator

@gvanrossum

How hard would it be to separate the new feature from the rest of the PR? (And describe it in an issue while you're at it?)

OK, I opened #4071 and separated the feature plus added tests in #4072. I would like to keep the code here, so that we can avoid:

if fullname in ('enum.Enum', 'enum.IntEnum'):
    assert fullname is not None

etc. The PRs can be merged in arbitrary order, however it makes sense to merge #4072 first.

Collaborator

ilevkivskyi commented Oct 8, 2017

@gvanrossum

How hard would it be to separate the new feature from the rest of the PR? (And describe it in an issue while you're at it?)

OK, I opened #4071 and separated the feature plus added tests in #4072. I would like to keep the code here, so that we can avoid:

if fullname in ('enum.Enum', 'enum.IntEnum'):
    assert fullname is not None

etc. The PRs can be merged in arbitrary order, however it makes sense to merge #4072 first.

ilevkivskyi added some commits Oct 8, 2017

ilevkivskyi added some commits Oct 13, 2017

Merge remote-tracking branch 'upstream/master' into last-strict-optio…
…nal-fixes

Conflicts:
	mypy/semanal.py
	mypy_self_check.ini
@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Oct 13, 2017

Collaborator

@JukkaL While this is fresh could you please review also this PR (to avoid more conflicts)?

Collaborator

ilevkivskyi commented Oct 13, 2017

@JukkaL While this is fresh could you please review also this PR (to avoid more conflicts)?

@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Oct 13, 2017

Collaborator

Sure, I'll give it a go.

Collaborator

JukkaL commented Oct 13, 2017

Sure, I'll give it a go.

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Oct 13, 2017

Member

Dang, there's already another conflict. :-(

Member

gvanrossum commented Oct 13, 2017

Dang, there's already another conflict. :-(

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Oct 13, 2017

Collaborator

Should be fixed now.

Collaborator

ilevkivskyi commented Oct 13, 2017

Should be fixed now.

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Oct 13, 2017

Member
Member

gvanrossum commented Oct 13, 2017

@JukkaL

Thanks, looks good! This migration has been a big undertaking and it's great to see the final strict optional exceptions removed. I left just some minor comments.

Show outdated Hide outdated mypy/semanal.py
ret_type = self.named_type_or_none('typing.Awaitable', [defn.type.ret_type])
if ret_type is None:
# We are running tests.
ret_type = self.named_type('typing_full.Awaitable', [defn.type.ret_type])

This comment has been minimized.

@JukkaL

JukkaL Oct 20, 2017

Collaborator

What is typing_full? Could we just blow up if typing.Awaitable is not available?

@JukkaL

JukkaL Oct 20, 2017

Collaborator

What is typing_full? Could we just blow up if typing.Awaitable is not available?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Oct 20, 2017

Collaborator

Without this line many tests fail (see the comment just above). I think this is because those tests use fixtures/typing-full.py. A better fix would be to always name it typing in tests, but I think this should be a separate PR.

@ilevkivskyi

ilevkivskyi Oct 20, 2017

Collaborator

Without this line many tests fail (see the comment just above). I think this is because those tests use fixtures/typing-full.py. A better fix would be to always name it typing in tests, but I think this should be a separate PR.

This comment has been minimized.

@JukkaL

JukkaL Oct 24, 2017

Collaborator

So the issue is that typing-full.py is imported as module named typing_full in tests? If this is true, this should be fixed (in another PR).

@JukkaL

JukkaL Oct 24, 2017

Collaborator

So the issue is that typing-full.py is imported as module named typing_full in tests? If this is true, this should be fixed (in another PR).

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Oct 25, 2017

Collaborator

Hm, my initial analysis is wrong, the problem is how named_type works. I will make a PR now.

@ilevkivskyi

ilevkivskyi Oct 25, 2017

Collaborator

Hm, my initial analysis is wrong, the problem is how named_type works. I will make a PR now.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Oct 25, 2017

Collaborator

OK, opened #4158

@ilevkivskyi

ilevkivskyi Oct 25, 2017

Collaborator

OK, opened #4158

Show outdated Hide outdated mypy/semanal.py
@@ -534,7 +541,7 @@ def visit_overloaded_func_def(self, defn: OverloadedFuncDef) -> None:
assert defn.impl is defn.items[-1]
defn.items = defn.items[:-1]
elif not self.is_stub_file and not non_overload_indexes:
if not (self.is_class_scope() and self.type.is_protocol):
if not (self.type and not self.is_func_scope() and self.type.is_protocol):

This comment has been minimized.

@JukkaL

JukkaL Oct 20, 2017

Collaborator

Random observation: Here it would be useful is is_class_scope() could return the binding self.type is not None when it returns true (there is a feature proposal about this, I think).

@JukkaL

JukkaL Oct 20, 2017

Collaborator

Random observation: Here it would be useful is is_class_scope() could return the binding self.type is not None when it returns true (there is a feature proposal about this, I think).

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Oct 20, 2017

Collaborator

Yes, I also noticed it would be very useful to somehow "remember" the bindings. For simple cases I was thinking about something like inlining methods (very approximately):

    @inline
    def meth(self) -> bool:
        return self.x is not None and self.y is not None
    if self.meth(): # use original expression returned by 'meth' here
        ...
@ilevkivskyi

ilevkivskyi Oct 20, 2017

Collaborator

Yes, I also noticed it would be very useful to somehow "remember" the bindings. For simple cases I was thinking about something like inlining methods (very approximately):

    @inline
    def meth(self) -> bool:
        return self.x is not None and self.y is not None
    if self.meth(): # use original expression returned by 'meth' here
        ...
@@ -690,8 +696,10 @@ def analyze_class_body(self, defn: ClassDef) -> Iterator[bool]:
if prohibited in named_tuple_info.names:
if nt_names.get(prohibited) is named_tuple_info.names[prohibited]:
continue
ctx = named_tuple_info.names[prohibited].node
assert ctx is not None

This comment has been minimized.

@JukkaL

JukkaL Oct 20, 2017

Collaborator

What if the definition is a bound type variable where node is None, I think? In a separate PR, it would be nice to make the node attribute non-optional (this is something we've discussed before).

@JukkaL

JukkaL Oct 20, 2017

Collaborator

What if the definition is a bound type variable where node is None, I think? In a separate PR, it would be nice to make the node attribute non-optional (this is something we've discussed before).

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Oct 20, 2017

Collaborator

What if the definition is a bound type variable where node is None, I think?

Currently generic named tuples are not supported, so I thought this should never happen.
I would keep this assert and then if it ever fails it will be clear what happens there.

@ilevkivskyi

ilevkivskyi Oct 20, 2017

Collaborator

What if the definition is a bound type variable where node is None, I think?

Currently generic named tuples are not supported, so I thought this should never happen.
I would keep this assert and then if it ever fails it will be clear what happens there.

if isinstance(defn.metaclass, NameExpr):
metaclass_name = defn.metaclass.name
elif isinstance(defn.metaclass, MemberExpr):
metaclass_name = get_member_expr_fullname(defn.metaclass)
else:
if metaclass_name is None:

This comment has been minimized.

@JukkaL

JukkaL Oct 20, 2017

Collaborator

Ah does this fix a bug, in case get_member_expr_fullname returns None? If yes, can you add a test case for the original issue?

@JukkaL

JukkaL Oct 20, 2017

Collaborator

Ah does this fix a bug, in case get_member_expr_fullname returns None? If yes, can you add a test case for the original issue?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Oct 20, 2017

Collaborator

Indeed, this fixes a crash (although it was not my intention, I just fixed the optional type here). I will add a test.

@ilevkivskyi

ilevkivskyi Oct 20, 2017

Collaborator

Indeed, this fixes a crash (although it was not my intention, I just fixed the optional type here). I will add a test.

@@ -2576,6 +2595,7 @@ def parse_typeddict_args(self, call: CallExpr,
for t in types:
if has_any_from_unimported_type(t):
self.msg.unimported_type_becomes_any("Type of a TypedDict key", t, dictexpr)
assert total is not None

This comment has been minimized.

@JukkaL

JukkaL Oct 20, 2017

Collaborator

This seems redundant, since we return if total is None above.

@JukkaL

JukkaL Oct 20, 2017

Collaborator

This seems redundant, since we return if total is None above.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Oct 20, 2017

Collaborator

Not from mypy's point of view. We return if total is None and len(args) == 3. I think this could be fixed by #2008

@ilevkivskyi

ilevkivskyi Oct 20, 2017

Collaborator

Not from mypy's point of view. We return if total is None and len(args) == 3. I think this could be fixed by #2008

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Oct 20, 2017

Collaborator

@JukkaL Thanks for review! I pushed fixes in a new commit.

Collaborator

ilevkivskyi commented Oct 20, 2017

@JukkaL Thanks for review! I pushed fixes in a new commit.

@JukkaL

JukkaL approved these changes Oct 24, 2017

Looks good now! Thanks for finishing the strict-optional migration!

@JukkaL JukkaL merged commit 936ceac into python:master Oct 24, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ilevkivskyi ilevkivskyi deleted the ilevkivskyi:last-strict-optional-fixes branch Oct 25, 2017

JukkaL added a commit that referenced this pull request Oct 31, 2017

Last strict optional fixes (#4070)
This makes mypy completely --strict-optional clean.

Fixes #1955.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment