not sure I like this as IIUC it makes the world non-deterministic, in that how this works will depends on what else happens to have been imported. Another alternative might be a function which allows you to register the subclass by GUID, and store them in a global map of some sort? I guess you could argue that's still non-deterministic, but at least requires explicit calls rather than side-effects)

Please excuse my ignorance but I'm afraid I don't understand why this does depend on side effects.
Maybe you can point me on it with the help of the following examples.

Let's start with the plain vanilla pythoncom.com_record class:

import pythoncom

TL_CLSID = '{E6F07342-C1F7-4E4E-B021-11BBD54B9F37}'
TL_MJVER = 2
TL_MNVER = 3
TL_LCID = 0

T_ENTITY_CLSID = '{9F2C4E2E-2C5C-4F39-9FDB-840A1E08B165}'
T_LINE_CLSID = '{B1461DD4-8C86-4627-B444-3D833C980111}'

print(pythoncom.com_record.__subclasses__())

[]

and create an instance of the T_LINE COM Record:

line_1 = pythoncom.GetRecordFromGuids(TL_CLSID, TL_MJVER, TL_MNVER, TL_LCID, T_LINE_CLSID)
print(line_1)
print(type(line_1))

com_struct(startX=0.0, startY=0.0, startZ=0.0, endX=0.0, endY=0.0, endZ=0.0)
<class 'com_record'>

If we now define the subclasses of pythoncom.com_record with the appropriate GUIDs for the T_ENTITY and T_LINE COM Records from the TypeLibrary:

class T_ENTITY(pythoncom.com_record):
    __slots__ = tuple()
    TLBID = TL_CLSID
    MJVER = TL_MJVER
    MNVER = TL_MNVER
    LCID = TL_LCID
    GUID = T_ENTITY_CLSID

    
class T_LINE(pythoncom.com_record):
    __slots__ = tuple()
    TLBID = TL_CLSID
    MJVER = TL_MJVER
    MNVER = TL_MNVER
    LCID = TL_LCID
    GUID = T_LINE_CLSID

print(pythoncom.com_record.__subclasses__())

[<class 'main.T_ENTITY'>, <class 'main.T_LINE'>]

we can do:

line_2 = T_LINE()
print(line_2)
print(type(line_2))

com_struct(startX=0.0, startY=0.0, startZ=0.0, endX=0.0, endY=0.0, endZ=0.0)
<class 'main.T_LINE'>

The type of the COM Record instance line_1, which we had created before the subclasses were defined, of course didn't change:

print(type(line_1))

<class 'com_record'>

However, creating a new T_LINE COM Record instance with the factory function, does now also return an instance of the subclass.

line_3 = pythoncom.GetRecordFromGuids(TL_CLSID, TL_MJVER, TL_MNVER, TL_LCID, T_LINE_CLSID)
print(line_3)
print(type(line_3))

com_struct(startX=0.0, startY=0.0, startZ=0.0, endX=0.0, endY=0.0, endZ=0.0)
<class 'main.T_LINE'>

If we define an invalid COM Record subclass using a GUID that is not contained in the TypeLibrary:

T_BAD_CLSID = '{A50732E7-F546-498B-AF28-E58161F9CA5C}'

class T_BAD(pythoncom.com_record):
    __slots__ = tuple()
    TLBID = TL_CLSID
    MJVER = TL_MJVER
    MNVER = TL_MNVER
    LCID = TL_LCID
    GUID = T_BAD_CLSID

print(pythoncom.com_record.__subclasses__())

[<class 'main.T_ENTITY'>, <class 'main.T_LINE'>, <class 'main.T_BAD'>]

the attempt to create an instance fails with an exception:

bad = T_BAD()

---------------------------------------------------------------------------
com_error Traceback (most recent call last)
----> bad = T_BAD()
com_error: (-2147319765, 'Element nicht gefunden.', None, None)

Of course you are free to define all kinds of dysfunctional subclasses:

class T_STUPID(pythoncom.com_record):
    description = 'blabla'

print(pythoncom.com_record.__subclasses__())

[<class 'main.T_ENTITY'>, <class 'main.T_LINE'>, <class 'main.T_BAD'>, <class 'main.T_STUPID'>]

but you cannot instantiate them:

gaga = T_STUPID()

---------------------------------------------------------------------------
SystemError Traceback (most recent call last)
----> gaga = T_STUPID()
SystemError: <class 'main.T_STUPID'> returned NULL without setting an exception

So to define working subclasses of pythoncom.com_record you have to follow the recipe and provide the GUID of the TypeLibrary that containts the COM Record definition, as well as its major and minor version number, plus the LCID and last but not least the GUID of the COM Record type as class variables.

I would argue that the definition of such a subclass is a kind of global registration.

Please excuse if I completely missed the point with the above examples but hopefully they can help to identify where there are side effects or non-determinisms.

My point is that these subclasses are presumably going to be in your code rather than in pywin32. Thus:

print(pythoncom.com_record.__subclasses__())

[<class 'main.T_ENTITY'>, <class 'main.T_LINE'>]

Assuming T_ENTITY/T_LINE etc are in modules you wrote, then the print above will only generate that output if those modules you wrote were actually imported. Thus, this new capability will implicitly depend on exactly what modules were previously imported and presumably what order they are in when multiple modules try and register the same type.

I would argue that the definition of such a subclass is a kind of global registration.

That's true, but it's implicit and hard to reason about. An explicit registration makes more sense to be because (a) it's easier to locate where these calls are and (b) there's an opportunity to fail if 2 callers try and register the same type. I'm saying that without (b), which type is used is largely non-deterministic.

OK, understood.
Do you think it is necessary to implement the nuts and bolts of this registration in pythoncom or would it be sufficient to keep the implementation as is and just provide an 'official' class-factory function for pythoncom.com_record subclasses that does the registration and checks to prevent double registration?

I mean the whole class machinery of Python itself does not prevent from doing all kinds of weird things and in particular somehow related to your point (b):

class A():
    a = 1

class B(A):
    b = 2

class B(A):
    c = 3

print(A.__subclasses__())

[<class 'main.B'>, <class 'main.B'>]

instantiating B does return an instance of the class that was last defined with this name:

x = B()

print(x.b)

---------------------------------------------------------------------------
AttributeError Traceback (most recent call last)
x = B()
----> print(x.b)
AttributeError: 'B' object has no attribute 'b'

print(x.c)

3

The class registration function could easily be written in Python and use the machinery as implemented.
Of course users could then still exploit the machinery under the hood and invoke the implicit behavior.
However, if you're not using the 'official' method you should know what you do.

A Python implementation seems fine to me and I'm not bothered that people can arrange to avoid this - it's just the fact that a base class which mediates which subclass is instantiated just smells inverted to me.

it's just the fact that a base class which mediates which subclass is instantiated just smells inverted to me.

The issue for which I couldn't come up with a better solution is how to instantiate the proper pythoncom.com_record subclass for Records returned by a COM method call. Without the 'smelly' part everything was fine as long as the Records were created by client code. However, all Records returned from a COM method call were just generic base class pythoncom.com_record instances. Therefore I introduced this 'identification' logic based on the GUID of the returned Record.

I was thinking get_registered_record_class or similar would hold classes, so the logic around here would be roughly the same - you are still making a call back into Python, but instead of subclasses you would be call the new function with some args. You would then expect the result to be a single class (or None) rather than iterating here - which you'd instantiate.

I've added the dictionary 'RecordClasses' to pythoncom.

The function 'PyRecord::new_record' does now check if the COM Record GUID is contained in this dict and uses the value returned for the GUID key as the subclass to instantiate.

The new Python function 'register_record_class' in the client module can be used to populate the dictionary with 'GUID' key / 'com_record subclass' value pairs. It checks that the value to be registered is a proper, instantiable com_record subclass and that the GUID is not already contained in the dictionary thus preventing accidential, potentially conflicting double registration.

While working on the tests, I recognized that it might be confusing that you could register a subclass with a name different from the name for that GUID in the TypeLibrary. In general it shouldn't matter what name you give to a struct because it's identity is based on the GUID. However, it might be confusing if you have to use a different name in 'win32com.client.Record' to create the struct and the returned instance is then of a type with the registered name.

There are pros and cons. On the one hand it could be helpful to give COM Record types a more meaningful name with respect to your application than the one from the TypeLibrary. On the other hand it might create inconsistencies when mixing subclass instance creation with the use of 'win32com.client.Record'.
Although one could argue that a user who defines and registers a COM Record type with a different name should be aware of the consequences.

What do you think?

I've given this a second thought and would value consistency over flexibility.

Therefore I've added a check for the record type name in the 'register_record_class' Python function.
To enable this in a straight forward way I've also added another dunder attribute '__record_type_name__' to the 'PyRecord::getattro' function that retrieves the record type name via 'IRecordInfo::GetName'.

If someone would really like to give the subclass a different name from the name in the TypeLibrary there is still the possibility to bypass the registration function and enter the class directly into the 'pythoncom.RecordClasses' dictionary.
However, using the "official" way everything should be consistent now.

We should probably add a null check? It should have already been there for new, but there are many more error conditions now.

You're right. The question is where to put this null check?
There are other places where PyRecord::new_record is used without a null check.

Should we instead raise a Python exception in PyRecord::new_record itself in every place where it currently returns a null without raising an exception?

IIUC, it will only currently return null when OOM. In this patch we have a more complicated situation - sometimes when it returns null there will be an exception set, whereas OOM will not. Having those OOM cases call PyErr_NoMem() seems easy, and it looks to me like all of the callers of PyRecord::new_record will do the right thing if null/false is returned - but what the docs say will not be Ok is PyTuple_SET_ITEM.

I've added commit c9aeec2 and commit 61662ab to resolve this.