Skip to content

Conversation

@jorenham
Copy link

@jorenham jorenham commented Aug 2, 2021

See python/typing#813 for the discussion

@JelleZijlstra
Copy link
Member

Excited to see this! Per https://www.python.org/dev/peps/pep-0001/#submitting-a-pep, you'll need to find a core developer to sponsor this PEP. Core devs who are active in the typing area include @gvanrossum, @ilevkivskyi, and @ambv, but I don't know if any of them are interested in serving as a sponsor.

property of the type variables, the must be used within variant
generic classes.

- Creating separate type variables for different variances often
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this is really common. It would be useful to have examples from open source projects where you see this duplication.

pep-9999.rst Outdated

class Spam(typing.Generic[+KT]): ...
class Eggs(typing.Protocol[KT, +VT]): ...
class OrderedSet(typing.AbstractSet[+T]): ...
Copy link
Member

Choose a reason for hiding this comment

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

This is invalid according to the specification, which says you can only put it in Generic or Protocol. I was actually going to ask whether this should be allowed.

Copy link
Author

Choose a reason for hiding this comment

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

Ah that's good to know. I can see how relaxing that restriction can be desirable, but I guess that that's out of scope here.

Copy link
Member

Choose a reason for hiding this comment

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

Is it a relaxing? This syntax is definitely legal right now without the +. If you allow it with your new syntax, you'll have to decide what it means. For example, maybe it should be possible to have an invariant subclass of a covariant class.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, thinking about it a little more I believe you should allow this, because invariant subclasses of covariant classes are commonplace. My suggestion would be that you have to specify the + again for every subclass, and if there's no + the subclass is invariant.

For example, we have this in the standard library:

class AbstractSet(Generic[+T]): ...  # covariant

class set(Sequence[T]): ...  # invariant

class frozenset(Sequence[+T]): ... # covariant

To express that, we need what I suggested above.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also agree with Jelle here, I think it should be allowed anywhere the the old forms are allowed.
On another note typing.AbstractSet is deprecated in favour of collections.abc.Set, so it might be a good idea to change that.

Copy link
Member

Choose a reason for hiding this comment

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

Another edge case is this:

class WhatIsMyVariance(Generic[+T], Sequence[-T]): ...

That should probably be disallowed, but this should be spelled out.

Copy link
Author

Choose a reason for hiding this comment

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

I agree with your suggestion @JelleZijlstra . But I think that In your example set and frozenset also need to subclass Generic, i.e.

class set(Sequence[T], Generic[T]): ...  # invariant

class frozenset(Sequence[T], Generic[+T]): ... # covariant

Then the restriction that +T and -T can only be used within the type parameter list of Generic and Protocol still holds. If you were to instead write class frozenset(Sequence[+T], Generic[+T]): ..., to me it looks as if Sequence itself is marked as covariant under T, whereas the covariance only applies to the specific definition of frozenset; the variance information does not need to be passed to the Sequence superclass.
This also avoids the possibility for edge cases, like in WhatIsMyVariance.

Copy link
Member

Choose a reason for hiding this comment

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

That's not how things currently work, though: writing class set(Sequence[T]): is legal and accepted by typecheckers.

Copy link
Author

Choose a reason for hiding this comment

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

Ah I see, somehow I thought that in that case you couldn't do e.g. spam: set[str], and that it required an explicit Generic as additional superclass.

Anyway, I now indeed see how this behavior has to be explicitly defined. Requiring the same variance for the same type variable in the same generic class should do the trick. So no mixing; only allow e.g.

  • class LinkedList(Iterable[T], Container[T]): ...
  • class LinkedList(Iterable[+T], Container[+T]): ...
  • class LinkedList(Iterable[-T], Container[-T]): ...

This could be checked within typing.Generic.__init_subclass__.

Syntax fix in the `Rejected Ideas` subheading
@Rosuav
Copy link
Contributor

Rosuav commented Aug 2, 2021

Can you sort out matters of process (like getting a sponsor, a PEP number, and a build that passes), and then discuss details of the content of the PEP afterwards?

@gvanrossum
Copy link
Member

Do we have a sponsor yet?

@jorenham
Copy link
Author

jorenham commented Aug 3, 2021

@gvanrossum No not yet. Would you like to be the sponsor?

@gvanrossum
Copy link
Member

@gvanrossum No not yet. Would you like to be the sponsor?

Hm, I am supporting this proposal but I don't really have the time to act as a good sponsor. According to this section in PEP 1 a sponsor should also act as a mentor, and while I would like to, realistically I just don't have the time. If @JelleZijlstra wants to be a sponsor, we can ask the SC to approve this (and I think they would).

@JelleZijlstra
Copy link
Member

I'd be happy to do that!

If so my first recommendation would be to go through another round of review before formally submitting the PEP, since a few of the issues I brought up above are fairly large.

@gvanrossum
Copy link
Member

Great, let me know when you need me to do anything.

@brettcannon
Copy link
Member

Since it sounds like this PEP isn't ready to be merged I'm going to close it for now. Feel free to either re-open it or open a new one when the PEP is ready.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants