GeezerVison

@ilevkivskyi -- idk how much I'll be able to contribute here, but I can definitely take a look sometime during the next day or two!

@Michael0x2a

I can definitely take a look sometime during the next day or two!

Great!

Would it really be too hard to support typing.py 3.5.1?

@gvanrossum

Would it really be too hard to support typing.py 3.5.1?

Unfortunately yes, as you remember, generics were completely redesigned in #195 so that basically it would be easier to just have a completely separate implementation of _ProtocolMeta, Protocol, etc., under a huge if sys.version_info[:3] == (3, 5, 1): .... This is not impossible, but the main problem is that I already forgot how generics worked at the time of 3.5.1, so it will take time. A possible compromise would be to implement a simplified version of Protocol, so that this will be valid:

class Proto(Protocol, Generic[T]):
    ...

but this shorthand will error in 3.5.1

class Proto(Protocol[T]):
    ...

This will be very easy to implement.

As parts of Dropbox are still stuck on Python 3.5.1, I think the compromise version would be great to have.

@gvanrossum OK, I will do this (but most probably not today or tomorrow).

No hurry. When you do, will you also add a Travis-CI check to run the tests under 3.5.1?

When you do, will you also add a Travis-CI check to run the tests under 3.5.1?

The checks for typing_extensions are already run on all versions (including micro), in the current version of the PR I just skip the checks on 3.5.0. and on 3.5.1.

(Actually all tests, not only typing_extensions, generally run on all micro versions, I just checked in .travis.yml.)

@ilevkivskyi:

Note: I haven't really been paying close attention to Protocol-related discussion, so I apologize in advance if anything I'm mentioning before has already been brought up somewhere else.

In any case, I found the following bugs/inconsistencies (I'm running Python 3.6.1):

1. Attempting to define the following class results in an AttributeError: type object 'Generic' has no attribute '_gorg' error message at runtime.

   class P1(Protocol, Generic[T]):
       def bar(self, x: T) -> str: ...

   Doing class Foo(List[str], Protocol): ... also resulted in the same kind of error.

   A little more broadly, I think it would be useful to add tests that intermix Protocol, Generic, and generic types in interesting ways.

2. Doing isinstance(None, Empty) (where "Empty" is the empty protocol as it's defined in your tests) behaves inconsistently. At runtime, that expression always evaluates to true, but when running mypy, that expression evaluates to false when strict-optional mode is enabled.

   I tested this with the following program:

   @runtime
   class Empty(Protocol): pass
   
   x: str = "foo"
   if isinstance(None, Empty):
       # Raises type error only if strict-optional is disabled 
       # (binder skips this branch when strict optional is enabled
       x = 3    
   
       # Always prints at runtime
       print("bar")

   That said, @runtime isn't supposed to be fully sound anyways so maybe this is ok?

3. The isinstance checks seem a little too flexible. For example:

   @runtime
   class P2(Protocol):
       def foo(self, x: int) -> int: ...
   
   class Concrete2:
       def foo(self, x: int) -> int: ...
   
   print(issubclass(Concrete2, P2))  # prints 'True', as expected
   print(isinstance(Concrete2, P2))  # also prints 'True'??

   I found the behavior of the last line to be surprising: I would have expected that to report 'false'.

4. Using the same definitions as above -- doing issubclass(Concrete(), P2) results in the following error: AttributeError: 'Concrete' object has no attribute '__mro__'.

5. This is probably overkill, but I think it might be nice if you checked that adding @runtime to a function and to a class that inherits a protocol (but isn't a protocol itself) both fail.

6. I was able to define the following class without any runtime errors: class Bar(List[Protocol]): .... I was also able to define functions that used Protocol for the parameter or return types. Idk if we can do anything about the latter, but it might be worth adding the same checks ClassVar uses in the former case for consistency (and adding related tests).

   I guess we would need to wait until Protocol is formally added to the typing module first in order before we can fix this, though?

   (A related, but off-topic thought: the other approach could be to remove the checks for misuse of ClassVar: since static type checkers can easily catch those sorts of things, maybe it's ok to let people get away with some silliness at runtime?)

You also asked if the behavior matched my expectations, so here are some notes related to that. I don't think these are really in-scope for this pull request though, so I can open issues on either the mypy or typing issue trackers as appropriate if you want.

1. The @runtime decorator -- I think calling it @runtime is potentially too misleading. This is partly because I think it could fool people into thinking that the decorator enables the typing module to do full-fledged runtime type analysis. However, since mypy and the runtime error message they'll get makes it pretty clear this isn't the case, I guess this is only a minor UX qualm.

   The other reason is because I think it can mislead people into thinking that @runtime is more powerful then it is even when working with specifically protocols. For example, consider:

   @runtime
   class P3(Protocol):
       def foo(self, x: int) -> int: ...
   
   class Concrete3:
       def foo(self, x: str) -> str: return x
   
   print(isinstance(Concrete3(), P3))

   I know that this behavior is described in the documentation (and it made sense in retrospect), but even after reading it, I forgot and thought that the last line would print false instead of true.

   As a result, I think it might be worth renaming @runtime to a scarier, longer name to discourage these sorts of casual accidents? Maybe something like @enable_protocol_runtime_structural_checks or @enable_unsafe_protocol_runtime? Both names are probably too long, though.

   Or alternatively, it might be worth modifying the documentation to directly warn people that @runtime is unsound and that they should think carefully before using it? Basically, taking the blue box that's already there and making it scarier?

2. It wasn't fully clear to me how the NoReturn type is supposed to interact with isinstance checks against the Empty protocol in general. Currently, doing any kind of variation of issubclass(NoReturn, Empty) (e.g. swapping issubclass with isinstance, swapping the NoReturn type with a call to a function that returns NoReturn) appears to evaluate at runtime to 'true'.

3. Also maybe out-of-scope for this pull request, but it seems like how classes and protocols currently interact can accidentally encourage unsound behavior? For example, the following program type-checks with mypy, even with strict-optional enabled, but crashes at runtime:

   class P4(Protocol):
       def foo(self) -> int: ...
   
   class Bar(P4): pass
   
   b = Bar()
   x: int = b.foo()
   print(x + 3)  # Typechecks but crashes at runtime!

   The documentation says (or at least implies) that the way we created Bar is more or less another way to make an ABC. In that case, it would be nice if Bar behaved as if it were defined using the abc module (e.g. as if the methods were annotated with `abc.abstractmethod``) to prevent this sort of thing and guarantee protocols are sound by default.

4. I think it would be handy to have more documentation regarding generic protocols in general. I wasn't aware that you could do things like class Foo(Protocol[T]) until I read through your unit tests, for example.

5. Along the documentation vein: how are classvars and protocols supposed to interact? When I add a classvar to a protocol definition, is that classvar a part of the protocol? Also, I tried doing:

   class P5(Protocol):
       x: ClassVar[int] = ...
       y: int = ...

   Mypy reported an error for the definition of x ("Incompatible types in assignment (expression has type "ellipsis", variable has type "int")") but not for 'y' -- I'm not sure if that's a bug or intentional.

6. Just to confirm, the ability to actually implement functions within a protocol class is intentional, right? In case the user wants to mix classical inheritance + nominal subtyping along with structural subtyping?

@Michael0x2a Thank you very much for comments and sorry for super-long delay. Here I reply to them, a bit later a will push the commits.

1. Attempting to define the following class results in an AttributeError: type object 'Generic' has no attribute '_gorg' error message at runtime.

Thanks, good catch, will fix this and will add tests as you suggested.

2. Doing isinstance(None, Empty) (where "Empty" is the empty protocol as it's defined in your tests) behaves inconsistently.

Yes, but I think in this case this is a problem with mypy. We already have issue python/mypy#3827 to track this. In principle I think there is a good chance to make @runtime protocols almost 100% safe.

3. The isinstance checks seem a little too flexible. For example:

Although it is surprising, it is technically true from the point of view of runtime check, since the runtime check does not care about signatures of methods, only their presence. On the contrary, mypy of course distinguishes classes and instances in static structural subtype checks.

4. Using the same definitions as above -- doing issubclass(Concrete(), P2) results in the following error: AttributeError: 'Concrete' object has no attribute '__mro__'.

I think this should raise a TypeError as for example issubclass(1, int). Will fix this.

5. This is probably overkill, but I think it might be nice if you checked that adding @runtime to a function and to a class that inherits a protocol (but isn't a protocol itself) both fail.

There is already such check, I will see if there are enough tests for it:

>>> class C(Protocol):
...     x: int
>>> @runtime
... class D(C):
...     pass
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
  File "/Users/ivan/typehinting/typing_extensions/src_py3/typing_extensions.py", line 927, in runtime
    ' got %r' % cls)
TypeError: @runtime can be only applied to protocol classes, got __main__.D

6. I was able to define the following class without any runtime errors: class Bar(List[Protocol]): ... ... I guess we would need to wait until Protocol is formally added to the typing module first in order before we can fix this, though?

Yes, it will be much easier to do this in typing itself, I think I already have all the checks for wrong use of Protocol same as for Generic in the typing PR.

1. The @runtime decorator -- I think calling it @runtime is potentially too misleading.

This question already appeared, but this is probably the best name proposed so far. I think this is OK, since this mirrors how ABCs like Iterable behave, for example:

class Test:
    __iter__ = 'Surprise!'
isinstance(Test(), collections.abc.Iterable)  # True
isinstance(Test(), typing.Iterable)  # Also True, since it simply delegates to the above.

I haven't heard many complains about this.

2. It wasn't fully clear to me how the NoReturn type is supposed to interact with isinstance checks against the Empty protocol in general.

I think that it should be True, even for static structural subtype checks it is safe to say that everything is a subtype of an empty protocol. I will open an issue at mypy tracker.

3. Also maybe out-of-scope for this pull request, but it seems like how classes and protocols currently interact can accidentally encourage unsound behavior?

This is not specific to protocols. Exactly the same problem exists with normal classes since mypy does not typecheck empty function bodies. I can see that maybe it is more important for protocols. It would be easy to implement in mypy, if you think it is important, then you can open an issue at mypy tracker.

4. I think it would be handy to have more documentation regarding generic protocols in general.

I think mypy already has some decent docs, when this will be added to typing we could discuss this more.

5. Along the documentation vein: how are classvars and protocols supposed to interact?

Yes, this is not mentioned in the docs, but mypy tries to carefully verify that not only types, but also "flags" (class vs instance variable, writable vs property, static method or not, etc) for attributes are satisfied in a structural subtype. Concerning the error with ellipsis, protocol attributes without default values should be declared like this (no r.h.s.):

class P5(Protocol):
    x: ClassVar[int]
    y: int

The ellipsis is used in tub files, to indicate that there is a default value. Therefore the fact that mypy does not show an error is probably a bug, I think assigning ellipses should be allowed only in stubs.

6. Just to confirm, the ability to actually implement functions within a protocol class is intentional, right?

Yes, the user might want to provide a default implementation (just like with ABCs), for example:

class Proto(Protocol):
    @abstractmethod
    def meth(self, arg: int) -> str:
        return str(arg + 1)
class Concrete(Proto):
    def meth(arg: int) -> str:
        basic = super().meth()
        return f'Fancy {basic}'

@JukkaL Thanks for more review comments! I think they are now implemented. Summary:

• Prohibit issubclass() for protocols that declare non-method members (isinstance() is still OK).
• Apply the None rule only to callable protocol members (methods).
• Add more tests.

I also opened two issues python/mypy#3938 and python/mypy#3939 to synchronize mypy behaviour. Please check if this PR can be now merged.

Everybody, I am going to try and review this tomorrow. I just read through all the discussion (whew!) to recover as much context as I can, hopefully it's still in my head tomorrow morning.

(Whoops, hit return too soon. There's more to come.)

@gvanrossum Thanks for review, I think I have now addressed all your comments.

@gvanrossum Ah, sorry, I forgot one your comment, about the @runtime name. Your version @runtime_checkable actually looks OK. But what do you think about releasing typing_extensions with @runtime and see how it will go?

I strongly prefer the longer name.

OK, but then we will need to also update mypy and typeshed.

...and the PEP.

TBH, I am quite exhausted after last two weeks, I would prefer to postpone everything which is not urgent. I you are OK releasing typing_extensions as is, then I will be only happy.

Understood. I am okay with releasing now, without more changes. We can debate the @runtime decorator name more when we have some experience.

OK, I just uploaded typing_extensions on PyPI.

Is the rationale for disallowing issubclass still valid, 6 years later? Honestly, it would be pretty useful to have issubclass check ClassVar-attributes (and ignore non-ClassVar attributes).

Yes, still valid. See python/cpython#89138
In general, please avoid commenting on long closed pull requests.