COM Record field assignment using type information and multidim SAFEARRAYs as field values

[geppi]

This PR implements:

• Respecting type information when setting COM Record field values which enables SAFEARRAY fields of various element types, not just SAFEARRY(VARIANT).
• Creating a multidimensional SAFEARRAY(COM Record) from a nested sequence of COM Records.
• Retrieving a multidimensional SAFEARRAY(COM Record) as a nested sequence of COM Records.
• Using multidimensional SAFEARRAYs of various element types as Record fields.
• Tests for the implementation.
  These do also cover a test case as requested in Bugfix for COM Record instance creation. #2641 (review).

[geppi]

@Avasam I had to retarget the Visual Studio project of PyCOMTest for the build tools v143 with a wild card for "the latest" installed Win SDK version because the build did fail, complaining that:

The Windows SDK version 10.0.20348.0 was not found.

[Avasam]

@Avasam I had to retarget the Visual Studio project of PyCOMTest for the build tools v143 with a wild card for "the latest" installed Win SDK version because the build did fail, complaining that:

I don't mind the retargeting and bumping the platform toolset to v143 (Visual Studio 2022) makes sense. I probably missed that somewhere in the recent CI migration to windows-2022 (and/or something else changed after the initial migration).

---

As for the rest of the PR, I am not qualified to review that / it is past my areas of expertise, so 🤷 Leaving to @mhammond

[mhammond]

Thanks - I'm going to need more time to look at this thoroughly, but some initial thoughts...

[geppi]

After reviewing the code path for the assignment of values to Record fields, I think the problem is that it was wrong from the beginning.

When I implemented the creation of SAFEARRAY(VT_RECORD) from a sequence of COM Records #2317, I followed the existing code path into PyCom_VariantFromPyObject and changed it to fit my actual use case. Later I encountered another COM Record type which has a field defined as SAFEARRAY(double) in the IDL file and tried to apply a similar change in the current PR, to also fit this use case.

Obviously IDL files in the field could contain struct definitions for SAFEARRAYs with other contained types, so we would need to change PyCom_VariantFromPyObject for the SAFEARRAYs of other types as well.
However, what would really be a problem is that an IDL file could also have a field defined as SAFEARRAY(VARIANT) which would then create an unresolvable conflict for this implementation.

The culprit is the setattro method of PyRecord that ignores the type information for Record fields which would well be available.

I think setattro should have never taken the code path via PyCom_VariantFromPyObject.
Instead it should retrieve the type information for the Record field via ITypeInfo and call PyCom_SAFEARRAYFromPyObject directly using this information in the vt parameter. For scalar types it should call the MakeObjToVariant method of PythonOleArgHelper, again using the retrieved type information in the m_reqdType attribute.

I have now reverted my changes in PyCom_VariantFromPyObject to the state before PR #2317, so that this function keeps doing what its name says, i.e. it always creates a VARIANT from PyObjects. Therefore the SAFEARRAYs it creates will contain VARIANT elements as was before PR #2317.

To fulfill the type requirements defined in an IDL file for the struct fields, the setattro method of PyRecord is changed in the way described above.

@mhammond Please let me know if you agree with this in general.

[geppi]

@mhammond Apart from the details, are you in general OK with the revised approach for setting COM Record field values?

[geppi]

@mhammond I'm feeling a little bit ignored. Although I don't want to push this too much, I would really like to know if you agree with the approach to retrieve the ITypeInfo of the Record field in setattro and if this properly addresses the concern you raised in this comment, please?

[geppi]

Is there anything I can do to move this PR forward?

While I understand that reviewing this PR is not a 5 minute task, not getting a single comment over the past 4 months is a little bit disappointing.

If some of the changes need more explanation or if you disagree with the general approach please let me know.

[mhammond]

I'm very sorry about the delay here, but this seems good to me, and the tests are exhaustive - thanks, and again, sorry for letting this slip.

[geppi]

Thanks.
Let me say that it's absolutely admirable that you keep this up over such a long time.

I can completely understand that larger PRs do take some time to get reviewed.
It was just that I started to worry if I had done something to get ghosted.

Please, in the future don't hesitate to just reply with a "sorry, I currently don't have the bandwidth to deal with this, please give me a ping in about one or two months".

You did this in your first comment September last year but then it went completely quiet which made me feel a little lost.

[mhammond]

Thanks! But the sad reality is just that I simply lost track of it, there were simply sitting as unread emails in my inbox. There are many PRs open, many of which I'm really quite ambivalent about, and sorting the wheat from the chaff is something I need to get better about and address. fwiw, please do feel free to ping me when I slip up.

[Avasam]

@mhammond To help sort through/prioritize PRs, feel free to take a look at #2704 . hopefully it can be helpful !

[geppi]

Uhh, emails. Github is already flooding my inbox with messages from the few repos I'm watching. The problem is that you get a message for each and every comment and commit. The Github notifications at least list the Issues/PRs that had changes as a summary.

@Avasam did create the Bookmarkable filters to reduce noise and help prioritize PRs in the Discussions which can probably help you sort the chaos.