Apart from that – what is this decorator needed for? Isn't setting self.is_classmethod enough?

I borrowed this logic from another place in the same file where we add a staticmethod decorator. Should that spot likely change as well?

As for why we need the decorator at all - in my testing it seems like setting is_classmethod alone was not enough. I believe the issue is that we need the logic generated from https://github.com/cython/cython/blob/master/Cython/Compiler/Symtab.py#L2006. It probably makes sense for this logic to be generated based off of the is_classmethod setting and for the decorator to be ignored but that refactor is at least currently beyond my familiarity with this codebase.

Open to suggestions on a better place for these definitions to exist and/or better way to express this.

This is definitely the wrong way to check the version of Python - the version used to run Cython isn't necessarily the same as the version of Python the code will run in.

I still don't think this needs version checks - there's no harm in supporting this even on Python 2.7. Nobody should be using these attributes for anything else since I'm fairly sure Python reserves all unused special method names for its own use.

I just noticed that this may raise AttributeError, which the _PyObject_LookupAttr() in the original code does not. There's a __Pyx_PyObject_GetAttrStrNoError() for that purpose. And we should make sure this is covered by tests.

True, I'll add a test to test behavior when __class_getitem__ does not exist however even with the current implementation it still later raises a TypeError which I guess overwrites the AttributeError. I'll update to use the suggested method, though.

Looks like I'm failing CI from this test on python versions <3.7. Any idea what might be wrong here @da-woods?

In Py2, the classes are old-style classes, not types. Let them inherit from (object).

And the _patched test depends on the __init_subclass__ protocol, which is Py3.6+, it seems. You can disable it in Py<3.5 with a @skipIf() decorator.

I was actually under the impression that the "tag" comment would instruct the test runner to ignore this test for python < 3.7 which is what I really wanted to do.

"pure3.7" runs the (Cython compiled) tests in all versions of Python, and also runs the tests in uncompiled-Python for Python>=3.7. I think this is correct because it should be possible for Cython to support __class_getitem__ all the time. Essentially it's just providing confirmation that Cython matches Python behaviour (for the versions of Python where this is supported)

The __init_subclass__ is probably something that has to be in the Python interpreter and Cython can't do on its own, so it makes sense to disable conditionally for some versions of Python.

The old style classes are actually a bug in current master. With language_level=3, there shouldn't be any old style classes in Py2 any more. I pushed a fix as #3530, let's see if that breaks anything in the test suite.

The __init_subclass__ is probably something that has to be in the Python interpreter and Cython can't do on its own

There's actually #2781 about this. PEP-487 Just needs to get implemented.