From 60a695381ea840361c1f3a3b393ea01aaca349af Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Thu, 18 Jul 2024 13:55:48 +0200 Subject: [PATCH] Fix Python docstring for overloads with some Doxygen comments The existing code didn't work correctly if the last element of the overload set didn't have a Doxygen comment: in this case, no docstring was generated at all. Fix this by trying to find any overload with a Doxygen comment, as Python docstring is common for all of them: add a helper function to do it and use it for both all kinds of ordinary functions (global, member and static) and __init__ functions generated for C++ ctors, as docstring was generated in the same wrong way for all of them in different places. Note that currently the overloads without Doxygen comments are not documented at all at Python level, as saying "there exist other overloads but there is no documentation for them" doesn't seem very useful and there doesn't seem anything else that we could do. Add a new unit test for testing all the different combinations of overloaded functions with and without Doxygen comments. --- Examples/test-suite/common.mk | 1 + Examples/test-suite/doxygen_overloads.i | 95 +++++++++++++++++++ .../python/doxygen_overloads_runme.py | 83 ++++++++++++++++ Source/Modules/python.cxx | 48 +++++++--- 4 files changed, 215 insertions(+), 12 deletions(-) create mode 100644 Examples/test-suite/doxygen_overloads.i create mode 100644 Examples/test-suite/python/doxygen_overloads_runme.py diff --git a/Examples/test-suite/common.mk b/Examples/test-suite/common.mk index 4f28e73d8..492c1f2fa 100644 --- a/Examples/test-suite/common.mk +++ b/Examples/test-suite/common.mk @@ -730,6 +730,7 @@ DOXYGEN_TEST_CASES += \ doxygen_ignore \ doxygen_misc_constructs \ doxygen_nested_class \ + doxygen_overloads \ doxygen_parsing \ doxygen_parsing_enums \ doxygen_translate \ diff --git a/Examples/test-suite/doxygen_overloads.i b/Examples/test-suite/doxygen_overloads.i new file mode 100644 index 000000000..8a5966ebb --- /dev/null +++ b/Examples/test-suite/doxygen_overloads.i @@ -0,0 +1,95 @@ +%module doxygen_overloads + +%inline %{ + +void overloadWithNoDoc(int) { } +void overloadWithNoDoc(double) { } + +/// Doc for first overload. +void overloadWithFirstDoc(int) { } +void overloadWithFirstDoc(double) { } + +void overloadWithSecondDoc(int) { } +/// Doc for second overload. +void overloadWithSecondDoc(double) { } + +/// Doc for both overloads, first. +void overloadWithBothDocs(int) { } +/// Doc for both overloads, second. +void overloadWithBothDocs(double) { } + +/// Doc for some overloads, first. +void overloadWithSomeDocs(int) { } +void overloadWithSomeDocs(double) { } +/// Doc for some overloads, third. +void overloadWithSomeDocs(char) { } + +/// Doc for some other overloads, first. +void overloadWithSomeOtherDocs(int) { } +/// Doc for some other overloads, second. +void overloadWithSomeOtherDocs(double) { } +void overloadWithSomeOtherDocs(char) { } + + +// Also test different kinds of member functions. + +struct S { + /// Doc for first static overload. + static void staticOverloadWithFirstDoc(int) { } + static void staticOverloadWithFirstDoc(double) { } + + /// Doc for first member overload. + void memberOverloadWithFirstDoc(int) { } + void memberOverloadWithFirstDoc(double) { } +}; + +// Class ctors are handled differently from the other functions, so check them too. + +struct ClassWithNoDoc { + ClassWithNoDoc(int) { } + ClassWithNoDoc(double) { } +}; + +struct ClassWithFirstDoc { + /// Doc for first ctor. + ClassWithFirstDoc(int) { } + ClassWithFirstDoc(double) { } +}; + +struct ClassWithSecondDoc { + ClassWithSecondDoc(int) { } + /// Doc for second ctor. + ClassWithSecondDoc(double) { } +}; + +struct ClassWithBothDocs { + /// Doc for both ctors, first. + ClassWithBothDocs(int) { } + /// Doc for both ctors, second. + ClassWithBothDocs(double) { } +}; + +struct ClassWithSomeDocs { + /// Doc for some ctors, first. + ClassWithSomeDocs(int) { } + ClassWithSomeDocs(double) { } + /// Doc for some ctors, third. + ClassWithSomeDocs(char) { } +}; + +struct ClassWithSomeOtherDocs { + /// Doc for some other ctors, first. + ClassWithSomeOtherDocs(int) { } + /// Doc for some other ctors, second. + ClassWithSomeOtherDocs(double) { } + ClassWithSomeOtherDocs(char) { } +}; + + +#ifdef SWIGPYTHON_BUILTIN +bool is_python_builtin() { return true; } +#else +bool is_python_builtin() { return false; } +#endif + +%} diff --git a/Examples/test-suite/python/doxygen_overloads_runme.py b/Examples/test-suite/python/doxygen_overloads_runme.py new file mode 100644 index 000000000..d5c3cdc62 --- /dev/null +++ b/Examples/test-suite/python/doxygen_overloads_runme.py @@ -0,0 +1,83 @@ +import doxygen_overloads +import inspect +import comment_verifier + +if inspect.getdoc(doxygen_overloads.overloadWithNoDoc) is not None: + raise Exception("No docstring expected for overloadWithNoDoc.") + +comment_verifier.check(inspect.getdoc(doxygen_overloads.overloadWithFirstDoc), + "Doc for first overload.") + +comment_verifier.check(inspect.getdoc(doxygen_overloads.overloadWithSecondDoc), + "Doc for second overload.") + +comment_verifier.check(inspect.getdoc(doxygen_overloads.overloadWithBothDocs), + r"""*Overload 1:* +Doc for both overloads, first. + +| + +*Overload 2:* +Doc for both overloads, second.""") + +comment_verifier.check(inspect.getdoc(doxygen_overloads.overloadWithSomeDocs), + r"""*Overload 1:* +Doc for some overloads, first. + +| + +*Overload 2:* +Doc for some overloads, third.""") + +comment_verifier.check(inspect.getdoc(doxygen_overloads.overloadWithSomeOtherDocs), + r"""*Overload 1:* +Doc for some other overloads, first. + +| + +*Overload 2:* +Doc for some other overloads, second.""") + +comment_verifier.check(inspect.getdoc(doxygen_overloads.S.staticOverloadWithFirstDoc), + "Doc for first static overload.") + +comment_verifier.check(inspect.getdoc(doxygen_overloads.S.memberOverloadWithFirstDoc), + "Doc for first member overload.") + +# As mentioned in doxygen_parsing_runme.py, docstrings for __init__ can't be specified when using "-builtin", so skip these checks in this case. +if not doxygen_overloads.is_python_builtin(): + # Do not check for ClassWithNoDoc ctor docstring, as Python provides a standard one + # ('Initialize self. See help(type(self)) for accurate signature.') if none is explicitly specified. + + comment_verifier.check(inspect.getdoc(doxygen_overloads.ClassWithFirstDoc.__init__), + "Doc for first ctor.") + + comment_verifier.check(inspect.getdoc(doxygen_overloads.ClassWithSecondDoc.__init__), + "Doc for second ctor.") + + comment_verifier.check(inspect.getdoc(doxygen_overloads.ClassWithBothDocs.__init__), + r"""*Overload 1:* +Doc for both ctors, first. + +| + +*Overload 2:* +Doc for both ctors, second.""") + + comment_verifier.check(inspect.getdoc(doxygen_overloads.ClassWithSomeDocs.__init__), + r"""*Overload 1:* +Doc for some ctors, first. + +| + +*Overload 2:* +Doc for some ctors, third.""") + + comment_verifier.check(inspect.getdoc(doxygen_overloads.ClassWithSomeOtherDocs.__init__), + r"""*Overload 1:* +Doc for some other ctors, first. + +| + +*Overload 2:* +Doc for some other ctors, second.""") diff --git a/Source/Modules/python.cxx b/Source/Modules/python.cxx index cbf0e4e1c..69b7493cd 100644 --- a/Source/Modules/python.cxx +++ b/Source/Modules/python.cxx @@ -1490,6 +1490,26 @@ public: ); } + /* ------------------------------------------------------------ + * find_overload_with_docstring() + * + * This function should be called with the node pointing to the + * last element of an overload set and returns an overload with + * a docstring or null if there are none. + * + * The idea is that, because the Python docstring is shared by + * all overloads, it's this function return value and not the + * node itself which needs to be passes to docstring() later. + * ------------------------------------------------------------ */ + Node* find_overload_with_docstring(Node* n) { + for (Node* node_with_doc = n; node_with_doc; node_with_doc = Getattr(node_with_doc, "sym:previousSibling")) { + if (have_docstring(node_with_doc)) + return node_with_doc; + } + + return NULL; + } + /* ------------------------------------------------------------ * build_combined_docstring() * @@ -2432,8 +2452,12 @@ public: if (!fast || olddefs) { /* Make a wrapper function to insert the code into */ Printv(f_dest, "\n", "def ", name, "(", parms, ")", returnTypeAnnotation(n), ":\n", NIL); - if (have_docstring(n)) - Printv(f_dest, tab4, docstring(n, AUTODOC_FUNC, tab4, true), "\n", NIL); + + // When handling the last overloaded function in an overload set (and we're only called for the last one if the function is overloaded at all), we need to + // output the docstring if any of the overloads has any documentation, not just this last one. + if (Node* node_with_doc = find_overload_with_docstring(n)) + Printv(f_dest, tab4, docstring(node_with_doc, AUTODOC_FUNC, tab4, true), "\n", NIL); + if (have_pythonprepend(n)) Printv(f_dest, indent_pythoncode(pythonprepend(n), tab4, Getfile(n), Getline(n), "%pythonprepend or %feature(\"pythonprepend\")"), "\n", NIL); if (have_pythonappend(n)) { @@ -4748,14 +4772,14 @@ public: if (!have_addtofunc(n)) { if (!fastproxy || olddefs) { Printv(f_shadow, "\n", tab4, "def ", symname, "(", parms, ")", returnTypeAnnotation(n), ":\n", NIL); - if (have_docstring(n)) - Printv(f_shadow, tab8, docstring(n, AUTODOC_METHOD, tab8), "\n", NIL); + if (Node* node_with_doc = find_overload_with_docstring(n)) + Printv(f_shadow, tab8, docstring(node_with_doc, AUTODOC_METHOD, tab8), "\n", NIL); Printv(f_shadow, tab8, "return ", funcCall(fullname, callParms), "\n", NIL); } } else { Printv(f_shadow, "\n", tab4, "def ", symname, "(", parms, ")", returnTypeAnnotation(n), ":\n", NIL); - if (have_docstring(n)) - Printv(f_shadow, tab8, docstring(n, AUTODOC_METHOD, tab8), "\n", NIL); + if (Node* node_with_doc = find_overload_with_docstring(n)) + Printv(f_shadow, tab8, docstring(node_with_doc, AUTODOC_METHOD, tab8), "\n", NIL); if (have_pythonprepend(n)) { fproxy = 0; Printv(f_shadow, indent_pythoncode(pythonprepend(n), tab8, Getfile(n), Getline(n), "%pythonprepend or %feature(\"pythonprepend\")"), "\n", NIL); @@ -4846,8 +4870,8 @@ public: String *callParms = make_pyParmList(n, false, true, kw); Printv(f_shadow, "\n", tab4, "@staticmethod", NIL); Printv(f_shadow, "\n", tab4, "def ", symname, "(", parms, ")", returnTypeAnnotation(n), ":\n", NIL); - if (have_docstring(n)) - Printv(f_shadow, tab8, docstring(n, AUTODOC_STATICFUNC, tab8), "\n", NIL); + if (Node* node_with_doc = find_overload_with_docstring(n)) + Printv(f_shadow, tab8, docstring(node_with_doc, AUTODOC_STATICFUNC, tab8), "\n", NIL); if (have_pythonprepend(n)) Printv(f_shadow, indent_pythoncode(pythonprepend(n), tab8, Getfile(n), Getline(n), "%pythonprepend or %feature(\"pythonprepend\")"), "\n", NIL); if (have_pythonappend(n)) { @@ -4953,8 +4977,8 @@ public: } Printv(f_shadow, "\n", tab4, "def __init__(", parms, ")", returnTypeAnnotation(n), ":\n", NIL); - if (have_docstring(n)) - Printv(f_shadow, tab8, docstring(n, AUTODOC_CTOR, tab8), "\n", NIL); + if (Node* node_with_doc = find_overload_with_docstring(n)) + Printv(f_shadow, tab8, docstring(node_with_doc, AUTODOC_CTOR, tab8), "\n", NIL); if (have_pythonprepend(n)) Printv(f_shadow, indent_pythoncode(pythonprepend(n), tab8, Getfile(n), Getline(n), "%pythonprepend or %feature(\"pythonprepend\")"), "\n", NIL); Printv(f_shadow, pass_self, NIL); @@ -4981,8 +5005,8 @@ public: String *callParms = make_pyParmList(n, false, true, allow_kwargs); Printv(f_shadow_stubs, "\ndef ", symname, "(", parms, ")", returnTypeAnnotation(n), ":\n", NIL); - if (have_docstring(n)) - Printv(f_shadow_stubs, tab4, docstring(n, AUTODOC_CTOR, tab4), "\n", NIL); + if (Node* node_with_doc = find_overload_with_docstring(n)) + Printv(f_shadow_stubs, tab4, docstring(node_with_doc, AUTODOC_CTOR, tab4), "\n", NIL); if (have_pythonprepend(n)) Printv(f_shadow_stubs, indent_pythoncode(pythonprepend(n), tab4, Getfile(n), Getline(n), "%pythonprepend or %feature(\"pythonprepend\")"), "\n", NIL); Printv(f_shadow_stubs, tab4, "val = ", funcCall(subfunc, callParms), "\n", NIL);