bpo-46244: Remove __slots__ from typing.TypeVar, .ParamSpec

[ariebovenberg]

The mixin class "typing._TypeVarLike" has no __slots__. Its subclasses do define __slots__, so it looks like a mistake.

This was confirmed in the bug report

https://bugs.python.org/issue46244

[AlexWaygood]

LGTM

(We could maybe think about whether it might be good to add __slots__ to some other typing classes, but that can be left for another PR -- _TypeVarLike is definitely the most clear-cut)

[gvanrossum]

Whoa. Looking at the diff I realized for the first time that the two subclasses, TypeVar and ParamSpec, have a __dict__ slot, so adding slots to the base class does not actually make a difference.

I'm sure that the __dict__ slots exist for a reason -- if we removed those, some user code would break that assigns additional attributes.

Now I wonder if the problem isn't the inverse -- why do the two subclasses have slots? What does that add?

[ariebovenberg]

@gvanrossum I can imagine the __slots__ still serve their purpose of improved attribute lookup speed 🤔 , although I of course don't know the reasoning when the code was written.

I can imagine __dict__ was added in the end to enable class-like attributes (e.g. __module__, __doc__, etc.) to be set in user code. (Having __dict__ in __slots__ is explicitly allowed for this possibility.)

Possible solutions:

1. Keep as many slots as can be justified

   • keep the slots in the base class (for improved attribute lookup speed)
   • remove __slots__ = ('__dict__', ) from ParamSpec. It serves no use.
   • keep __slots__ in TypeVar (for __name__ lookup speed I suppose)

2. Remove slots from all three classes. Benefits are marginal and some other typing classes (like _GenericAlias) don't seem to declare them either.

[Fidget-Spinner]

Interesting. I admit that when I implemented ParamSpec, it was meant to mirror any existing behavior in TypeVar, and I didn't give much thought into whether it required a __dict__.

Digging through commit history tells me that we only added a __dict__ to TypeVar recently (3.9-ish) c1c7d8e. Prior to this, TypeVar had __slots__ but no __dict__, all the way back to when typing was still in a separate package from Python.

[sobolevn]

Removing '__dict__' slot and running tests does not affect their result. All tests pass without it. It is also not documented.

I also cannot recall any real-life use-case where T.some_attr is used 🤔

I think that removing __dict__ from __slots__ might be a good idea in the long run.

[ariebovenberg]

@sobolevn I'm inclined to agree, although adding __doc__ to the allowed slots may be a good idea.

[gvanrossum]

I'd be very reluctant to remove the __dict__ slot -- even though it's undocumented and we don't test for it, someone is likely using it (since having a __dict__ is the default for most classes). I don't believe that attribute lookup speed is ever a factor for these.

I suppose we could ask @serhiy-storchaka if he recalls why he added the __dict__ slot in #19719.

It's possible that there's a space-saving reason for having __slots__ that include a __dict__ -- I'm guessing that as long as you don't use the __dict__ it's NULL, so for the common case (where all attributes fit in slots) the __slots__ version is more space-efficient.

I used to just know this kind of stuff, but the code is a lot more convoluted now due to key-sharing (the dict keys are stored, once, on the class object, and the instances only have a values array), and is about to become more so -- for 3.11 Mark Shannon has improvements that store the values array in a more compact form (IIRC without holes).

Anyway, I think the sensible solution is to remove the __slots__ from these classes.

[serhiy-storchaka]

@ariebovenberg is right, __dict__ is needed for supporting the __module__ attribute in instances. You cannot add '__module__' in __slots__, because descriptor __module__ conflicts with the same-named class attribute.

'__dict__' was explicitly added in __slots__ in c1c7d8e. Previously there was an implicit __dict__ inherited from _Immortal which lacked __slots__. After adding an empty __slots__ in _Immortal I was needed to add '__dict__' in __slots__ for TypeVar. Alternate solution would be to remove __slots__ in TypeVar, but I hoped to find other solution of this conflict sometime.

There was no conflict for __name__, because __name__ of class is accessed via a metaclass descriptor. But __module__ of class is saved in class' dict. Adding a dedicated field in PyTypeObject and descriptor in class type would solve this conflict. It is a relatively large change which breaks binary compatibility of PyTypeObject (which is not in the stable API anyway) and may simplify and speed up access to __module__ attribute of classes. But there may be side effects.

[gvanrossum]

@serhiy-storchaka What do you think of just getting rid of the slots for TypeVar and ParamSpec?

[serhiy-storchaka]

What do you think of just getting rid of the slots for TypeVar and ParamSpec?

I am fine with this.

[ariebovenberg]

@serhiy-storchaka @gvanrossum about the slots on _TypeVarLike: I assume we don't keep them there either, since we can assume it'll only be subclassed by ParamSpec and TypeVar?

If its purpose would be a general mixin, __slots__ would make more sense.

[gvanrossum]

@serhiy-storchaka @gvanrossum about the slots on _TypeVarLike: I assume we don't keep them there either, since we can assume it'll only be subclassed by ParamSpec and TypeVar?

Indeed.

If its purpose would be a general mixin, __slots__ would make more sense.

It is not.

[gvanrossum]

LG, but please add a brief blurb.

[kumaraditya303]

Suggested change

	Removed ``__slots__`` from :class:``typing.ParamSpec`` and :class:``typing.TypeVar``.
	Removed ``__slots__`` from :class:`typing.ParamSpec` and :class:`typing.TypeVar`.

rst uses single backticks.

[ariebovenberg]

whoops! Committed the fix below ✅

[gvanrossum]

Thanks all! Landing now.

Should we bother backporting? It doesn't make a difference, so probably not.

[gvanrossum]

@Fidget-Spinner Do you want to land this? Make sure to edit the commit title and message so they match what's ultimately done.

[bedevere-bot]

@Fidget-Spinner: Please replace # with GH- in the commit message next time. Thanks!

[Fidget-Spinner]

@gvanrossum Sorry, I didn't realize that GH mobile app's squash and merge doesn't allow me to edit the message. After pressing the button it just merged it without prompting me for anything 😞.

Lesson learnt: always do the major things on the computer. I'll revert and redo the message later today.

Anyways, @ariebovenberg thanks for the patch, and congrats on your first contribution to CPython!

[gvanrossum]

@gvanrossum Sorry, I didn't realize that GH mobile app's squash and merge doesn't allow me to edit the message. After pressing the button it just merged it without prompting me for anything 😞.

Their mobile website allows it though. :-)

Lesson learnt: always do the major things on the computer. I'll revert and redo the message later today.

I wouldn't bother. The summary line is correct: "Remove slots from typing.TypeVar, .ParamSpec ". The commit body is spammy, but that doesn't matter enough to revert and fix the commit. We can live with this!

Anyways, @ariebovenberg thanks for the patch, and congrats on your first contribution to CPython!

Indeed.