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

bpo-43977: Use tp_flags for collection matching #25723

Merged

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Apr 29, 2021

  • Adds the two new flags: Py_TPFLAGS_SEQUENCE and Py_TPFLAGS_MAPPING
  • Uses them in MATCH_SEQUENCE and MATCH_MAPPING to avoid importing collections.abc when matching.

https://bugs.python.org/issue43977

@markshannon markshannon force-pushed the use-tp-flag-for-collection-matching branch from d265dcc to ddef5fb Compare April 29, 2021 17:17
Copy link
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

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

Thanks! I've left a few comments.

We should also remove the map_abc and seq_abc members from the PyInterpreterState struct in Include/internal/pycore_interp.h and Python/pystate.c.

@@ -499,6 +523,11 @@ _abc__abc_register_impl(PyObject *module, PyObject *self, PyObject *subclass)
/* Invalidate negative cache */
get_abc_state(module)->abc_invalidation_counter++;

if (PyType_Check(subclass) && PyType_Check(self) &&
!PyType_HasFeature((PyTypeObject *)subclass, Py_TPFLAGS_IMMUTABLETYPE))
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

Modules/_abc.c Outdated
Comment on lines 468 to 470
if (_PyDict_DelItemId(cls->tp_dict, &PyId___flags__) < 0) {
return NULL;
}
Copy link
Member

Choose a reason for hiding this comment

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

Why are we deleting __flags__ here? It looks like it will get overwritten anyways:

>>> class C:
...     __flags__ = "Spam"
... 
>>> C.__flags__
284160

Copy link
Member Author

Choose a reason for hiding this comment

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

The __flags__ set in Sequence is in Sequence.__dict__, but normally __flags__ is a descriptor.
So, it is probably safest to delete it. I suspect it doesn't matter much in practice.

Modules/_abc.c Outdated
@@ -446,6 +449,27 @@ _abc__abc_init(PyObject *module, PyObject *self)
return NULL;
}
Py_DECREF(data);
if (PyType_Check(self)) {
PyTypeObject *cls = (PyTypeObject *)self;
PyObject *flags = _PyDict_GetItemIdWithError(cls->tp_dict, &PyId___flags__);
Copy link
Member

Choose a reason for hiding this comment

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

Hm, this whole mechanism makes me a bit uncomfortable. I'm not aware of any case where writing to __flags__ in Pythonland actually works like this.

But I can't really think of anything better, so I guess it's fine. Maybe we could just use a name like _abc_tpflags or something instead of __flags__?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, it is just a name. I like using a dunder name, as it is a special thing. __abc_tpflags__ perhaps?

Comment on lines 1 to 3
Use tp_flags on the class object to determine if the subject is a sequence
or mapping when pattern matching. Avoids the need to import collections.abc
when pattern matching.
Copy link
Member

Choose a reason for hiding this comment

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

Just some markup to make this a bit richer:

Suggested change
Use tp_flags on the class object to determine if the subject is a sequence
or mapping when pattern matching. Avoids the need to import collections.abc
when pattern matching.
Use :c:member:`~PyTypeObject.tp_flags` on the class object to determine if the subject is a sequence
or mapping when pattern matching. Avoids the need to import :mod:`collections.abc`
when pattern matching.

@@ -793,6 +793,7 @@ def __isub__(self, it):

### MAPPINGS ###

TPFLAGS_MAPPING = 1 << 6
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is public and can be relied upon, it needs to be added to the docs for collections.abc

Copy link
Member Author

Choose a reason for hiding this comment

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

This should be internal. I'll hide it.

@markshannon markshannon merged commit 069e81a into python:master Apr 30, 2021
@markshannon markshannon deleted the use-tp-flag-for-collection-matching branch May 4, 2021 09:00
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.

5 participants