Skip to content
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

Minor updates to protocol semantics #3996

Merged
merged 6 commits into from
Sep 29, 2017

Conversation

ilevkivskyi
Copy link
Member

This updates protocol semantic according to the discussion in python/typing#464:

  • None should be considered a subtype of an empty protocol
  • method = None rule should apply only to callable protocol members
  • issublcass() is prohibited for protocols with non-method members.

Fixes #3906
Fixes #3938
Fixes #3939

@gvanrossum
Copy link
Member

@JukkaL Can you review this?

@ilevkivskyi What changed so that reveal_type() of builtins.list now comes out as builtins.list[Any]?

@ilevkivskyi
Copy link
Member Author

What changed so that reveal_type() of builtins.list now comes out as builtins.list[Any]?

This:

 def visit_type_type(self, t: TypeType) -> None:
-    pass
+    t.item.accept(self)

This fix will anyway happen in #3952, but without it this test (expectedly) crashed.

@JukkaL JukkaL self-assigned this Sep 27, 2017
Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Looks good, just a few minor comments.

class P(Protocol):
pass

x: P = None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also test that None is compatible with a non-empty protocol when not using strict optional checking.

@@ -273,6 +273,16 @@ def visit_call_expr(self, e: CallExpr, allow_none_return: bool = False) -> Type:
not tp.type_object().runtime_protocol):
self.chk.fail('Only @runtime protocols can be used with'
' instance and class checks', e)
if (isinstance(e.callee, RefExpr) and len(e.args) == 2 and
e.callee.fullname == 'builtins.issubclass'):
for expr in mypy.checker.flatten(e.args[1]):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the body to a separate method, as this function is already quite long. Maybe also move the body of the above if statement (from line 269) to a separate method. You could also move the if statement to be nested under the above if statement (line 267). You'd only need the check if e.callee.fullname == 'builtins.subclass' which would simplify things slightly.

@JukkaL
Copy link
Collaborator

JukkaL commented Sep 28, 2017

Can you also fix the merge conflict?

@ilevkivskyi
Copy link
Member Author

Thanks! I addressed CR and fixed the conflict.

@ilevkivskyi ilevkivskyi mentioned this pull request Sep 29, 2017
5 tasks
@gvanrossum gvanrossum merged commit b3ca169 into python:master Sep 29, 2017
gvanrossum added a commit that referenced this pull request Sep 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants