gh-126615: Make `COMError` public and add to `ctypes` doc. by junkmd · Pull Request #126686 · python/cpython

junkmd · 2024-11-11T14:48:30Z

Please refer also to gh-126384 and gh-126610 for the behavior when a foreign function fails to call a COM method.

Issue: ctypes: Make COMError, mentioned in the "Function prototypes" section of documentation, public. #126615

📚 Documentation preview 📚: https://cpython-previews--126686.org.readthedocs.build/

"Raised when a COM method call failed." https://github.com/python/cpython/blob/2313f8421080ceb3343c6f5d291279adea85e073/Modules/_ctypes/_ctypes.c#L5548

Doc/library/ctypes.rst

picnixz · 2024-11-16T11:40:26Z

Doc/library/ctypes.rst

+
+.. exception:: COMError(hresult, text, details)
+
+   Windows only: This non-public exception is raised when a COM method call


You can use .. availability:: Windows (but put the directive after the class doc)

I followed the pattern in ctypes.rst because it frequently used sentences like Windows only: ....

Since this markup wasn't mentioned in the documentation guide, I overlooked it.
Is this a more modern approach?

It's widely used in the https://docs.python.org/3/library/socket.html module actually. But if it's not the pattern of ctypes maybe it's better to be consistent. We can make a follow-up PR to transform them into directives.

I removed the Availability directive and reverted to the original "Windows only: ...".

Once this PR is merged, I would like to work for replacing "Foo only: ..." with the .. availability:: Foo directive.

Referring to the socket.ioctl documentation, it seems that using :platform: Windows might be more appropriate than .. availability:: Windows. What do you think?

Alternatively, could it be that :platform: is intended to be used exclusively in module sections, as described in the documentation guide's "Module-specific markup"?

.. availability:: is generally the correct one to use. FWIW, we have some tooling to ensure it's in a correct format.
It's OK to not use it in this PR, to be consistent with the rest of the documentation. But, if you're looking for more to do in the ctypes docs, adding availability would make a great follow-up PR :)

Doc/library/ctypes.rst

picnixz · 2024-11-16T11:42:32Z

Doc/library/ctypes.rst

   are specified in the :attr:`!argtypes` tuple.

+
+.. exception:: COMError(hresult, text, details)


Should it be placed here or somewhere else? because it cuts the flow of the reading where paramflags is documented afterwards. Maybe we can put it before the functions?

I was also unsure whether this was the right place to put it.

On the other hand, placing it above prototype or above FOOFUNCTYPE might seem abrupt.

Might it be appropriate to create the Exceptions section, including ArgumentError?
However, in this scope, I think making such a change that involves the "Foreign functions" section is excessive.

Well.. honestly I think it makes sense to put it in an exception section. I think it could be fine for this small docs change (I mean, it's for our own convenience). We can ask @hugovk as a docs expert.

Yes, we often have an exceptions section further down, see for example:

https://docs.python.org/3/library/json.html#exceptions

If the community agrees, creating an "Exceptions" section is no problem at all.

For now, I think I'll try placing it at the end of the document, after the "Arrays and pointers" section.

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

Doc/library/ctypes.rst

in order to maintain consistency with other sections.

encukou · 2024-11-18T12:39:06Z

AFAICS, COMError is available in _ctypes, but not ctypes. So, documenting it as ctypes.COMError would be misleading.
I think we should re-export it from ctypes -- it's not very usable otherwise.

encukou · 2024-11-18T12:39:29Z

Doc/library/ctypes.rst

+
+   .. attribute:: details
+
+      The 5-tuple representing additional details about the error.


That's not very helpful. Could you document what the items are?

Based on the C implementation source code, these items are retrieved via methods of IErrorInfo¹: GetDescription, GetSource, GetHelpFile, GetHelpContext, and the ProgID obtained via using ProgIDFromCLSID and GetGUID.

However, this exception can be raised not only from foreign functions but also intentionally by passing variable-length tuples, as seen in test_win32.py and the comtypes package.

cpython/Lib/test/test_ctypes/test_win32.py

Lines 67 to 76 in f9c5573

def test_COMError(self):

from _ctypes import COMError

if support.HAVE_DOCSTRINGS:

self.assertEqual(COMError.__doc__,

"Raised when a COM method call failed.")

ex = COMError(-1, "text", ("details",))

self.assertEqual(ex.hresult, -1)

self.assertEqual(ex.text, "text")

self.assertEqual(ex.details, ("details",))

I will revise this part, taking these facts into account.

Footnotes

Reference: IErrorInfo implementation in comtypes. ↩

Since _ctypes is internal API that is subject to change, I think it would be best to change test_win32 and comtypes so that they raise the same 5-tuple as ctypes itself, and document that 5-tuple.

I wrote details of details.

I plan to improve test_win32 after improving this documentation.

For comtypes, I will investigate how much COMError.details is being used and then discuss the matter with the project's community to decide how to proceed.

As mentioned earlier, I would like to focus on improving the documentation in this PR.

encukou · 2024-11-18T12:46:08Z

Doc/library/ctypes.rst

   integer. *name* is name of the COM method. *iid* is an optional pointer to
   the interface identifier which is used in extended error reporting.

+   If *iid* is not specified, a :exc:`WindowsError` is raised if the COM method


WindowsError is now an alias of OSError.

Suggested change

If *iid* is not specified, a :exc:`WindowsError` is raised if the COM method

If *iid* is not specified, an :exc:`OSError` is raised if the COM method

Good catch.

Thank you.

junkmd · 2024-11-18T14:27:25Z

AFAICS, COMError is available in _ctypes, but not ctypes. So, documenting it as ctypes.COMError would be misleading.
I think we should re-export it from ctypes -- it's not very usable otherwise.

_CData cannot be imported from either _ctypes or ctypes, but it is documented as "non-public". I consider COMError to be something similar.

I DO agree that COMError has value as a public exception.
However, for now, I would like to separate scopes.

In this PR, I will focus solely on improving the documentation, and I plan to address the new tasks that arise here by opening separate issues.

encukou · 2024-11-19T12:29:23Z

I see where you're coming from, but I don't think this PR should be merged as-is.

It's true that ctypes._CData does not exist as documented. This has been the case since before Python 3. I think that's a bug in the documentation¹⁾; it should not be taken as a precedent.

While _CData is private API, but its documentation is useful since it describes public classes that inherit from _CData.

But for _ctypes.COMError, I don't see a way to use it without importing it. If you can't use it with except (or isinstance), knowing its attributes is not very helpful. AFAIK, documenting it would only encourage people to use private API.

So, I think it needs to become public at the same time as it's documented.

¹⁾ Tangentially: I don't think we have a good way to document private base classes. As with COMType, it would probably be easier to make _CData public than to document it properly as is.

junkmd · 2024-11-19T13:18:52Z

Understood.

If the maintainers approve making such a private API public, I have no strong objections.

I initially aimed to update the documentation first because I thought proposals to change production code would require extensive discussions from various perspectives and take longer to merge.

I’ll add from _ctypes import COMError line and the platform bridge to ctypes/__init__.py.

Since this is no longer an internal change, I’ll also add a NEWS entry.

Additionally, I’ll include a .. versionchanged:: directive to indicate that while this has existed as a private API in _ctypes, it became public starting with Python 3.14.

encukou

Thank you!
I do think this is a better way to do it -- if we want to talk about something in the docs, it's better if it has a public name.

Two more nitpicks:

encukou · 2024-11-20T10:31:03Z

Doc/library/ctypes.rst

+   .. versionchanged:: 3.14
+
+      This exception is now public.
+      Previously, this was private and only available in ``_ctypes``.


Let's not expose private API in the docs.

Suggested change

.. versionchanged:: 3.14

This exception is now public.

Previously, this was private and only available in ``_ctypes``.

.. versionadded:: next

You’re right, this information isn’t particularly useful in the documentation.

I’ll move it to the NEWS file as a supplement.

encukou · 2024-11-20T10:58:07Z

Doc/library/ctypes.rst

   the interface identifier which is used in extended error reporting.

+   If *iid* is not specified, an :exc:`OSError` is raised if the COM method
+   call fails. If *iid* is specified, a :exc:`.COMError` is raised instead.


I think (but I'm not 100% sure) that .COMError means Sphinx will search all namespaces, not just ctypes, for a COMError. So it would be better to use ctypes.COMError explicitly:

Suggested change

call fails. If *iid* is specified, a :exc:`.COMError` is raised instead.

call fails. If *iid* is specified, a :exc:`~ctypes.COMError` is raised instead.

encukou

Thank you!

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

bedevere-app bot added docs Documentation in the Doc dir skip news awaiting review labels Nov 11, 2024

bedevere-app bot mentioned this pull request Nov 11, 2024

ctypes: Make COMError, mentioned in the "Function prototypes" section of documentation, public. #126615

Closed

ZeroIntensity added topic-ctypes needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Nov 11, 2024

junkmd added 4 commits November 16, 2024 09:18

add COMError exception to Doc/library/ctypes.rst.

e70aa54

Add a mention of the difference in exceptions that can be raised.

181f512

Add a dot to refer to the object within the same module.

857fef5

Simplify descriptions of attributes.

90fb47f

junkmd force-pushed the gh_126615_comerror_doc branch from 58cc032 to 90fb47f Compare November 16, 2024 09:22

junkmd mentioned this pull request Nov 16, 2024

Methods to bridge the gap between Integer error codes and HRESULT values represented as signed 32-bit hexadecimal enthought/comtypes#665

Closed

Align with the docstring defined in the C implementation:

1f9d4ee

"Raised when a COM method call failed." https://github.com/python/cpython/blob/2313f8421080ceb3343c6f5d291279adea85e073/Modules/_ctypes/_ctypes.c#L5548

picnixz reviewed Nov 16, 2024

View reviewed changes

picnixz requested a review from encukou November 16, 2024 11:42

Apply suggestions from code review

b71e310

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

picnixz reviewed Nov 16, 2024

View reviewed changes

Doc/library/ctypes.rst Outdated Show resolved Hide resolved

junkmd added 2 commits November 16, 2024 12:21

Add the dot to make the reflink.

dafb5b2

Add the availability.

49d5e5f

junkmd requested a review from picnixz November 16, 2024 12:55

junkmd added 2 commits November 16, 2024 13:38

Create the "Exceptions" section.

1fa159a

Remove the availability directive

934c36a

in order to maintain consistency with other sections.

encukou reviewed Nov 18, 2024

View reviewed changes

junkmd added 2 commits November 18, 2024 15:04

WindowsError -> OSError

9461642

details of details.

705ffcc

junkmd requested a review from encukou November 18, 2024 15:14

Small fix.

b5a2170

junkmd changed the title ~~gh-126615: Add COMError to ctypes doc.~~ gh-126615: Make COMError public and add to ctypes doc. Nov 19, 2024

junkmd added 3 commits November 19, 2024 15:19

Make COMError public.

a16f1e2

Update docs.

e2617f3

Update test_COMError.

57d76c5

encukou reviewed Nov 20, 2024

View reviewed changes

junkmd added 4 commits November 20, 2024 12:18

Update the NEWS.

36f1607

.. versionchanged:: -> .. versionadded::

068ea2a

Update the prototype section.

3da9e15

Merge branch 'main' into gh_126615_comerror_doc

920bccc

junkmd requested a review from encukou November 20, 2024 12:29

encukou enabled auto-merge (squash) November 20, 2024 12:36

encukou removed needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes docs Documentation in the Doc dir labels Nov 20, 2024

encukou approved these changes Nov 20, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting review labels Nov 20, 2024

encukou merged commit 5b45025 into python:main Nov 20, 2024

bedevere-app bot removed the awaiting merge label Nov 20, 2024

junkmd deleted the gh_126615_comerror_doc branch November 20, 2024 13:19

ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025

pythongh-126615: ctypes: Make COMError public (pythonGH-126686)

1e1f7a4

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>


		.. exception:: COMError(hresult, text, details)

		Windows only: This non-public exception is raised when a COM method call

		are specified in the :attr:`!argtypes` tuple.


		.. exception:: COMError(hresult, text, details)


		.. attribute:: details

		The 5-tuple representing additional details about the error.

	def test_COMError(self):
	from _ctypes import COMError
	if support.HAVE_DOCSTRINGS:
	self.assertEqual(COMError.__doc__,
	"Raised when a COM method call failed.")

	ex = COMError(-1, "text", ("details",))
	self.assertEqual(ex.hresult, -1)
	self.assertEqual(ex.text, "text")
	self.assertEqual(ex.details, ("details",))

	If iid is not specified, a :exc:`WindowsError` is raised if the COM method
	If iid is not specified, an :exc:`OSError` is raised if the COM method

	call fails. If iid is specified, a :exc:`.COMError` is raised instead.
	call fails. If iid is specified, a :exc:`~ctypes.COMError` is raised instead.

Uh oh!

Comments

Conversation

junkmd commented Nov 11, 2024 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

picnixz Nov 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

encukou commented Nov 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junkmd Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junkmd commented Nov 18, 2024

Uh oh!

encukou commented Nov 19, 2024

Uh oh!

junkmd commented Nov 19, 2024

Uh oh!

encukou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

encukou left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

junkmd commented Nov 11, 2024 •

edited by github-actions bot

Loading

picnixz Nov 16, 2024 •

edited

Loading

junkmd Nov 18, 2024 •

edited

Loading