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.

Sign up for GitHub

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

Jump to bottom

Protocols #3132

Merged

JukkaL merged 152 commits into python:master from ilevkivskyi:protocols

Aug 16, 2017

Member

ilevkivskyi commented Apr 4, 2017 •

edited

Loading

This is an implementation of protocols proposed in PEP 544. This PR adds support for:

generic protocols (including inference for protocols)
recursive protocols
special support for Type[] and isinstance()
structural subtyping for Callable and Tuple
other things

For example, now one can do this:

class Linked(Protocol[T]):
    val: T
    def next(self) -> 'Linked[T]': ...

class L:
    val: int
    def next(self) -> 'L': ...

def last(seq: Linked[T]) -> T:
    ...

reveal_type(last(L()))  # Revealed type is 'builtins.int*'

In general, the PR is practically feature complete, the main missing thing now is structural join/meet for partially overlapping protocols. For example, sometimes an explicit annotation is needed:

class A(Protocol):
    one: int
    def __len__(self) -> int: ...

class B(Protocol):
    another: int
    def __len__(self) -> int: ...

a: A
b: B
reveal_type([a, b])  # Revealed type is 'builtins.list[builtins.object*]'

y: List[Sized] = [a, b]  # This works OK

The reason for this is that I decided to re-use Instances for protocols, and real structural join/meet would require creating a "fake" TypeInfo to describe the resulting protocol. However, I have encountered problems with serialization/de-serialization of "fake" TypeInfos and postponed this feature (as proposed by Jukka). On the other hand, reusing Instances for protocols allowed to implement this in a very simple way. The PR is very small and 2/3 of it are extensive tests.

There is something I encountered while working on this PR. It looks like structural subtyping is significantly slower than nominal. This is why I also implement "subtype cache" for Instances, so that there is a net win in speed of few percent (even when I locally updated typeshed replacing all ABC in typing with protocols).

In terms of work-flow, I would be happy if people will try this PR and give also feedback on behaviour, not just a code review. I will soon submit a PR to typeshed with updates for typing stubs, and a PR with runtime implementation. As well, a PR to PEPs repo with an updated draft that takes into account comments that appeared so far.

@JukkaL @ambv @gvanrossum

Ivan Levkivskyi and others added 30 commits

March 26, 2017 14:20


          Recognize protocols in semanal.py

884481d


          Add issubtype and basic constraints

c19b6d4


          Merge remote-tracking branch 'upstream/master' into protocols

83c1579


          Remove debugging code

b01f5b0


          Minor fixes: semanal, constrains, lib-stub; add few tests

16901fc


          Allow mypy_extensions.runtime, revert change to test name.

c8c1247


          Fix typo

f430c65


          Merge remote-tracking branch 'upstream/master' into protocols

beea7d2


          Enhance constraints, add join (meet seems to be already OK); minor fixes

260237a


          Merge remote-tracking branch 'upstream/master' into protocols

5414eaf


          Prohibit protocol instatiation except Type[]; start working on @runtime

7f8a514


          Add some tests

92b71a2


          Restore docstring formstting

83085c7


          Merge remote-tracking branch 'upstream/master' into protocols

27b8570


          Fix serialization; add few more tests

41179c2


          Add notes for missing protocol members in assignment


          Add notes for missing members also for arguments and return types

db123a3


          Prohibit definition via self.x in protocol classes

31bf16c


          Merge remote-tracking branch 'upstream/master' into protocols

03b5d72


          Make more things in fixtures structural subtypes

65a8546


          Structural support for __call__; add more tests

2768d76


          Fix lint and tests

93beb75


          Beautify error notes

5135fd9


          Bind self to potential subtype; add tests for structural self-types

fdeb89b


          One more test

1fbcb4a


          Move global data to nodes.TypeInfo

d012e38


          Add comments and docstrings; recognize and treat abstract variables

801770d


          Fix tests and lint

a625f37


          Merge remote-tracking branch 'upstream/master' into protocols

fb6b1ad


          Add two more tests for recursive

adb68eb


          Address second part of recent comments

40d635f

ilevkivskyi mentioned this pull request

Enforce compatible __init__ in structural subtypes #3823

Open

Member Author

ilevkivskyi commented Aug 12, 2017

@JukkaL Thanks! I think I have implemented all comments so far. The only question left is how precise we should be with variances in typing-full.pyi fixture? If you think it makes sense to match typeshed, then I will also update this.

JukkaL reviewed

View reviewed changes

Collaborator

JukkaL left a comment

Another batch of test case reviews.

test-data/unit/check-protocols.test

+              def last(seq: Linked[T]) -> T:
+                  pass
+              reveal_type(last(L())) # E: Revealed type is 'builtins.int*'

Collaborator

JukkaL Aug 14, 2017

Maybe add test where a class subtly does not implement a recursive protocol. For example, the return type of next() is not compatible but otherwise things are fine.

test-data/unit/check-protocols.test

+              t: P1
+              t = A() # OK
+              t = B() # E: Incompatible types in assignment (expression has type "B", variable has type "P1")

Collaborator

JukkaL Aug 14, 2017

Again, consider testing more incompatible cases, such as attr2 existing but with an incompatible list item type.

test-data/unit/check-protocols.test Outdated

+                  def f(self, x: int) -> int: pass
+                  @overload
+                  def f(self, x: str) -> str: pass
+                  def f(self, x): pass

Collaborator

JukkaL Aug 14, 2017

Overloaded method implementation should probably be optional in a protocol. If there is no implementation, the method would be treated as abstract.

test-data/unit/check-protocols.test

+                  def f(self, x: int) -> None:
+                      pass
+              x: P = C()

Collaborator

JukkaL Aug 14, 2017

Test that a class with an overloaded method with an identical signature is treated as compatible. Test that a class with an invalid overloaded method is treated as incompatible.

test-data/unit/check-protocols.test Outdated


		l = [x, y]

		reveal_type(l) # E: Revealed type is 'builtins.list[__main__.P*]'

Collaborator

JukkaL Aug 14, 2017

Also test the trivial case of [x, x] or similar.

test-data/unit/check-protocols.test

+              def bar(a: Sized) -> int:
+                  return a.__len__()
+              bar(Foo())

Collaborator

JukkaL Aug 14, 2017

Also call with a list argument?

test-data/unit/check-protocols.test Outdated

+              [builtins fixtures/isinstancelist.pyi]
+              [out]
+              main:11: error: Argument 1 to "foo" has incompatible type "str"; expected "SupportsInt"

Collaborator

JukkaL Aug 14, 2017

Use # E: ... comment?

test-data/unit/check-protocols.test

+                  @property
+                  def attr2(self) -> int: pass
+              x: P = C() # E: Incompatible types in assignment (expression has type "C", variable has type "P") \

Collaborator

JukkaL Aug 14, 2017

Do you test anywhere what happens if a class has method and the protocol has an attribute/property, and vice versa?

Member Author

ilevkivskyi Aug 14, 2017

It looks like no, I added two tests.

test-data/unit/check-protocols.test Outdated

+              fun_p(C())  # OK
+              [builtins fixtures/list.pyi]
+              [case testIpmlicitTypesInProtocols]

Collaborator

JukkaL Aug 14, 2017

Typo: Ipmlicit

test-data/unit/check-protocols.test

+              class P(Protocol[T]): # E: Invariant type variable 'T' used in protocol where covariant one is expected
+                  attr: int
+              [case testGenericProtocolsInference1]

Collaborator

JukkaL Aug 14, 2017

I don't remember if I saw a test case that would have a related case where the protocol is non-generic and a non-generic class has a generic method. Say, protocol has signature int -> int and class has signature T -> T (but T has method scope).

Member Author

ilevkivskyi Aug 14, 2017

I have one, but only with self types, I added two tests with just "usual" generic methods.

JukkaL reviewed

View reviewed changes

Collaborator

JukkaL left a comment

Another batch of comments. Hopefully the next batch will be the last one.

mypy/checker.py

+                          down_args = [UninhabitedType() if i == j else AnyType(TypeOfAny.special_form)
+                                       for j, _ in enumerate(tvars)]
+                          up, down = Instance(info, up_args), Instance(info, down_args)
+                          # TODO: add advanced variance checks for recursive protocols

Collaborator

JukkaL Aug 14, 2017

Create issue about variance checks for recursive protocols? It seems that a set of mutually recursive protocols should be checked as a unit, since the variance in one protocol can affect variance in another protocol.

mypy/checkexpr.py

                           # subtyping algorithm if type promotions are possible (e.g., int vs. float).
                           if formal.type in actual.type.mro:
                               return 2
+                          elif formal.type.is_protocol and is_subtype(actual, erasetype.erase_type(formal)):

Collaborator

JukkaL Aug 14, 2017

Should we erase the actual type as well?

Member Author

ilevkivskyi Aug 15, 2017

I am not sure, but in view of #3656 I am reluctant to increase similarity of anything to protocols.

mypy/constraints.py

                                   res.extend(infer_constraints(
                                       template.args[j], mapped.args[j], neg_op(self.direction)))
                               return res
+                          if (template.type.is_protocol and self.direction == SUPERTYPE_OF and
+                                  # We avoid infinite recursion for structural subtypes by checking
+                                  # whether this type already appeared in the inference chain.

Collaborator

JukkaL Aug 14, 2017

Add something about the correctness of this. Maybe mention that this is a standard way to perform structural subtype checks.

mypy/constraints.py Outdated

+                              for member in template.type.protocol_members:
+                                  inst = mypy.subtypes.find_member(member, instance, original_actual)
+                                  temp = mypy.subtypes.find_member(member, template, original_actual)
+                                  assert inst is not None and temp is not None

Collaborator

JukkaL Aug 14, 2017

Add comment about why this is safe.

mypy/constraints.py Outdated

+                                      res.extend(infer_constraints(temp, inst, neg_op(self.direction)))
+                              template.type.inferring.pop()
+                              return res
+                          elif (instance.type.is_protocol and self.direction == SUBTYPE_OF and

Collaborator

JukkaL Aug 14, 2017

This and the previous section of code look very similar. Can you merge them to avoid code duplication?

mypy/nodes.py Outdated

		abstract_attributes = None # type: List[str]
		protocol_members = None # type: List[str]

Collaborator

JukkaL Aug 14, 2017

Add comment. Some edge cases to discuss: What about inherited protocol members? What about members defined in object?

mypy/nodes.py Outdated

                   abstract_attributes = None  # type: List[str]
+                  protocol_members = None  # type: List[str]
+                  # These represent structural subtype matrices. Note that these are shared

Collaborator

JukkaL Aug 14, 2017

The first sentence is not descriptive enough. This is a pretty tricky concept so please explain it in more detail and maybe give an example.

mypy/nodes.py Outdated

+                  # during the type checking phase.
+                  assuming = None  # type: List[Tuple[mypy.types.Instance, mypy.types.Instance]]
+                  assuming_proper = None  # type: List[Tuple[mypy.types.Instance, mypy.types.Instance]]
+                  # Ditto for temporary stack of recursive constraint inference.

Collaborator

JukkaL Aug 14, 2017

Explain what goes into the stack and why we need it.

mypy/nodes.py Outdated

+                  inferring = None  # type: List[mypy.types.Instance]
+                  cache = None  # type: Set[Tuple[mypy.types.Type, mypy.types.Type]]
+                  cache_proper = None  # type: Set[Tuple[mypy.types.Type, mypy.types.Type]]
+                  # 'inferring' and 'assumig' can't be also made sets, since we need to use

Collaborator

JukkaL Aug 14, 2017

Typo: 'assumig'.

mypy/nodes.py Outdated

+                  # there is a dependency infer_constraint -> is_subtype -> is_callable_subtype ->
+                  # -> infer_constraints.
+                  inferring = None  # type: List[mypy.types.Instance]
+                  cache = None  # type: Set[Tuple[mypy.types.Type, mypy.types.Type]]

Collaborator

JukkaL Aug 14, 2017

Document cache and cache_proper. Explain why we have them.

JukkaL reviewed

View reviewed changes

Collaborator

JukkaL left a comment

This concludes my review. As I mentioned earlier, fixing most issues now is optional -- there wasn't anything super critical. They can be fixed after this has been merged. If you leave things out, create issues which mention comments that haven't been followed through yet to more easily track what is not done yet.

mypy/subtypes.py Outdated

-                          # NOTO: left.type.mro may be None in quick mode if there
+                          if (left, right) in right.type.cache:
+                              return True
+                          # NOTE: left.type.mro may be None in quick mode if there
                           # was an error somewhere.
                           if left.type.mro is not None:
                               for base in left.type.mro:
                                   if base._promote and is_subtype(
                                           base._promote, self.right, self.check_type_parameter,
                                           ignore_pos_arg_names=self.ignore_pos_arg_names):

Collaborator

JukkaL Aug 14, 2017

Do we need to propagate ignore_declared_variance in all recursive calls?

Member Author

ilevkivskyi Aug 15, 2017

Ideally, yes, but there are too many places, this is related to #3828, I will add a TODO item here.

mypy/subtypes.py Outdated

                           # was an error somewhere.
                           if left.type.mro is not None:
                               for base in left.type.mro:
                                   if base._promote and is_subtype(
                                           base._promote, self.right, self.check_type_parameter,
                                           ignore_pos_arg_names=self.ignore_pos_arg_names):
+                                      right.type.cache.add((left, right))

Collaborator

JukkaL Aug 14, 2017

Would be better style to use a TypeInfo method such as record_subtype_cache_entry(left, right) instead of directly accessing cache.

Collaborator

JukkaL Aug 14, 2017

Also, should we only add to cache if no subtype flags such as ignore_pos_arg_names are being used?

Member Author

ilevkivskyi Aug 15, 2017

I am not super sure, but I think it is OK to always add them, since these are only for Instances and they (at least currently) are always used with ignore_pos_arg_names.

mypy/subtypes.py Outdated

@@ @@ -130,24 +143,33 @@ def visit_instance(self, left: Instance) -> bool: @@
                       if isinstance(right, TupleType) and right.fallback.type.is_enum:
                           return is_subtype(left, right.fallback)
                       if isinstance(right, Instance):
-                          # NOTO: left.type.mro may be None in quick mode if there
+                          if (left, right) in right.type.cache:

Collaborator

JukkaL Aug 14, 2017

Would be better style to use a method such as if right.type.is_cached_subtype_check(left, right):.

mypy/subtypes.py Outdated

+                          # nominal subtyping currently ignores '__init__' and '__new__' signatures
+                          if member in ('__init__', '__new__'):
+                              continue
+                          # The third argiment below indicates to what self type is bound.

Collaborator

JukkaL Aug 14, 2017

Typo: argiment

mypy/subtypes.py Outdated

		stack.pop()


		def is_protocol_implementation(left: Instance, right: Instance, allow_any: bool = True) -> bool:

Collaborator

JukkaL Aug 14, 2017

What about changing allow_any to proper_subtype (and negate the value) or similar for consistency?

mypy/subtypes.py Outdated

		return missing


		def get_conflict_types(left: Instance, right: Instance) -> List[Tuple[str, Type, Type]]:

Collaborator

JukkaL Aug 14, 2017

Again, it looks like this should be in messages.py (can still be a top-level function) and should include protocol in the name for clarity.

mypy/subtypes.py Outdated

		return conflicts


		def get_bad_flags(left: Instance, right: Instance) -> List[Tuple[str, Set[int], Set[int]]]:

Collaborator

JukkaL Aug 14, 2017

Like above, move to messages.py and include 'protocol' in the function name.

mypy/subtypes.py Outdated

@@ @@ -549,7 +809,8 @@ def restrict_subtype_away(t: Type, s: Type) -> Type: @@
                       # Since runtime type checks will ignore type arguments, erase the types.
                       erased_s = erase_type(s)
                       new_items = [item for item in t.relevant_items()
-                                   if (not is_proper_subtype(erase_type(item), erased_s)
+                                   if (not (is_proper_subtype(erase_type(item), erased_s) or
+                                            is_proper_subtype(item, erased_s))

Collaborator

JukkaL Aug 14, 2017

What's this change?

Member Author

ilevkivskyi Aug 15, 2017

This is not only related to protocols, but it affected several tests. This is actually a step in the direction of #3827. The logic is following, if I erase both item and s, then there will be "too many" Anys there, and is_proper_subtype will return False, while at runtime isinstance() will actually return True for protocols. Maybe this is not 100% ideal, but it fixed several test failures without causing new failures in existing tests.

More generally, I think we could reconsider use of is_proper_subtype(erased_t, erased_s) for isinstance() with nominal types, maybe there could be a better concept, something like is_runtime_subtype? I don't think we need a separate issue for this, this is basically just an extended discussion for #3827

Collaborator

JukkaL Aug 15, 2017

Yeah, this seems okay. Maybe add a reference to #3827 here?

Member Author

ilevkivskyi Aug 15, 2017

OK, I will add a comment.

mypy/subtypes.py Outdated

@@ @@ -601,26 +862,38 @@ def visit_deleted_type(self, left: DeletedType) -> bool: @@
                   def visit_instance(self, left: Instance) -> bool:
                       right = self.right
                       if isinstance(right, Instance):
+                          if (left, right) in right.type.cache_proper:

Collaborator

JukkaL Aug 14, 2017

If you add helper methods for accessing the cache, use them here.

mypy/types.py

@@ @@ -188,6 +188,15 @@ def __init__(self, @@
                   def accept(self, visitor: 'TypeVisitor[T]') -> T:
                       return visitor.visit_unbound_type(self)
+                  def __hash__(self) -> int:

Collaborator

JukkaL Aug 14, 2017

I haven't reviewed this file but I can do it later (even after the PR has been landed) since this looks pretty straightforward.

ilevkivskyi mentioned this pull request

Variance of generics in typing-full fixture #3826

Open


          First part of todays comments

19073e5

ilevkivskyi mentioned this pull request

Safer treatment of @runtime protocols in isinstance() #3827

Closed

Collaborator

JukkaL commented Aug 15, 2017

Before protocols are usable, Protocol needs to be added to typing_extensions (this can happen after this PR has been merged).

Member Author

ilevkivskyi commented Aug 15, 2017

Protocol needs to be added to typing_extensions

Yes, I will do this soon.

This was referenced Aug 15, 2017

Implement variance checks for recursive protocols. #3828

Open

Improve type inference for recursive protocols #3829

Open


          Second part of yesterdays comments

50d98c0

JukkaL mentioned this pull request

Fix handling of non-method callable attribute #3227

Closed

ilevkivskyi mentioned this pull request

Implement better join and meet for protocols #3830

Closed

JukkaL mentioned this pull request

Use object as the fallback for various Callable types #3831

Closed

ilevkivskyi mentioned this pull request

Refactor common code in analyze_member_access and find_member #3832

Open

ilevkivskyi added 2 commits

August 15, 2017 20:09


          Last part of review comments


          One last missing comment

f1c915e

Member Author

ilevkivskyi commented Aug 15, 2017

@JukkaL It looks like all the comments are implemented now (or corresponding issues filed).

Thanks for review once more!

JukkaL approved these changes

View reviewed changes

Collaborator

JukkaL left a comment

This looks good now! Thanks for persevering with the very long review process!

JukkaL merged commit c55e48b into python:master

Collaborator

JukkaL commented Aug 16, 2017

Here are some important follow-up tasks:

Add implementation of Protocol to typing_extensions (or if @gvanrossum feels that it's too early, it can go to mypy_extensions) and release a version of typing_extensions with Protocol.
Add definition of Protocol to typeshed (but don't turn existing ABCs in typeshed into protocols yet).

After the above steps user-defined protocols should work.

Changing typing ABCs to be protocols can happen later -- potentially we'll have a mypy release with only support for user-defined protocols first. It's still not clear how we'll move forward with the bigger typeshed change.

JukkaL mentioned this pull request

User-defined protocols #3838

Closed

Member Author

ilevkivskyi commented Aug 16, 2017

Add implementation of Protocol to typing_extensions

I will hopefully make PRs for typing_extensions (implementation and stubs) tomorrow. I think it is OK to go straight with typing_extensions since @vlasovskikh (and maybe other) is already interested in this.

Member Author

ilevkivskyi commented Aug 16, 2017

By the way, @matthiaskramm what do you (i.e. pytype team) think about protocols and making things like Iterable etc. protocols in typeshed?

Member

gvanrossum commented Aug 16, 2017

Woot! Congrats and thanks everyone.

Member

vlasovskikh commented Aug 17, 2017

Thanks!

@ilevkivskyi +1 for putting Protocol into typing_extensions.

evhub mentioned this pull request

Mypy error evhub/coconut#317

Closed

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

Labels