Python docstring: argument names conflicting with python keywords, are systematically turned into argN

[aundro]

Hi,

SWiG 2.0.12 used to rename python keyword-named C/C++ arguments, to an underscore-prefixed version, but SWiG 3.0.12 doesn't do than anymore (by default.)

Assuming an interface file such as:

%module test
%feature(autodoc,0);
%feature("compactdefaultargs");
%inline %{
int process(int from);
%}

using 2.0.12 we would get:

"""process(_from) -> int"""

But since the following changelist:

commit fdc6bbeda387fb49ab7dcdbb0f17e4645db3115c
Author: Vadim Zeitlin <vz-swig@zeitlins.org>
Date:   Sat Aug 9 16:33:23 2014 +0200

this has changed and we now get:

"""process(arg1) -> int"""

…which unfortunately is less useful.

Using the -keyword option, apparently gets us to the old behavior. But unfortunately this has side-effects, other than just making the documentation more useful (in particular, well, enabling keyword arguments, which isn't something we desire)

Am I mistaken thinking that, regardless of whether or not kwargs are enabled for a given wrapper, the docstring shouldn't change? (or in any case not become less useful)

Thanks!

[vadz]

Thanks for the report, I'll try to look at this, but it might not happen immediately.

Does the problem occur only with the specified features, i.e. autodoc==0 and compactdefaultargs on?

[aundro]

Hi @vadz, thank you very much for the help.

Regarding your question: it does happen in all circumstances. Here's a combined test:

aundro@flatiron:/tmp$ for CDA in "%feature(\"compactdefaultargs\");" ""; do for DLVL in 0 1 2 3; do sed s/DLVL/${DLVL}/ test.i.in > test.i.in2 && sed s/CDA/${CDA}/ test.i.in2 > test.i && /tmp/swig-3.0.12-install/bin/swig -c++ -python -o test-swig3.cpp test.i && echo && echo "compactdefaultargs=${CDA}, autodoc=${DLVL}" && grep "\->" test.py; done; done;

compactdefaultargs=%feature("compactdefaultargs");, autodoc=0
    """do_ea(arg1) -> int"""

compactdefaultargs=%feature("compactdefaultargs");, autodoc=1
    """do_ea(ea_t arg1) -> int"""

compactdefaultargs=%feature("compactdefaultargs");, autodoc=2
    do_ea(arg1) -> int

compactdefaultargs=%feature("compactdefaultargs");, autodoc=3
    do_ea(ea_t arg1) -> int

compactdefaultargs=, autodoc=0
    do_ea(arg1) -> int
    do_ea() -> int

compactdefaultargs=, autodoc=1
    do_ea(ea_t arg1) -> int
    do_ea() -> int

compactdefaultargs=, autodoc=2
    do_ea(arg1) -> int
    do_ea() -> int

compactdefaultargs=, autodoc=3
    do_ea(ea_t arg1) -> int
    do_ea() -> int
aundro@flatiron:/tmp$ 

[wsfulton]

@vadz any chance you can look at this soon before we release SWIG 4.0.0?

[vadz]

Give me until the end of the week to see if I can find time for this. If I can't, you probably shouldn't wait for me...

[vadz]

I could make an ugly (adding yet another boolean parameter to a function already taking 3 of them) but simple (and so hopefully unlikely to break anything else) change, but it only fixes the name of the parameter in the doc string, not in the function signature itself, i.e. now

def process(arg1):
    r"""process(_from) -> int"""
    ...

is generated.

Fixing the name of the actual argument is less trivial, as the parameter names are not necessarily unique (e.g. INOUT can be reused), so we can't just always use Swig_name_make() in makeParameterName(), for example.

So I'm going to submit this fix already, but I'm not sure if it really helps as now we have a mismatch between the docstring and the actual parameter and I guess this could be (even more?) problematic?

[vadz]

I spent a bit more time on this and here is an alternative solution (i.e. at most one of #1394 and #1395 should be applied) which results in

def process(_from):
    r"""process(_from) -> int"""
    ...

being generated, but it has its own problems: it assumes that only upper-case parameter names are special and can be non-unique (and hence need to be replaced with "argN"). This is not really the case in theory, i.e. nothing prevents you from using any (lower-case) "foo" twice in the parameter list neither, from SWIG point of view, but I think this shouldn't happen much in practice and, if it does, do we really need to support this, knowing that it's an error in both C and Python?

[wsfulton]

I'd say the parameter names in the docstring should match the actual argument names. So #1394 is not ideal as it addresses half the problem (just corrects the documentation name).

The base class implementation, that is, Language::makeParameterName will correct for duplicate names such as INPUT. I'd say that only changing them if they are upper case is too hacky (as in #1395). I'm not sure why in #1395 you don't look for duplicate names in the same way the base class implementation does.

I'm not sure why check_kwargs(n) is used on the Node to override the base class implementation. Do you have an insight into that?

Here are some simple tests to add in to the fix:

diff --git a/Examples/test-suite/autodoc.i b/Examples/test-suite/autodoc.i
index 97c05d7..ec7307a 100644
--- a/Examples/test-suite/autodoc.i
+++ b/Examples/test-suite/autodoc.i
@@ -148,3 +148,11 @@ bool is_python_builtin() { return true; }
 bool is_python_builtin() { return false; }
 #endif
 %}
+
+// Autodoc Python keywords
+%warnfilter(SWIGWARN_PARSE_KEYWORD) process;
+%feature(autodoc,0) process;
+%feature("compactdefaultargs") process;
+%inline %{
+int process(int from) { return from; }
+%}
diff --git a/Examples/test-suite/python/autodoc_runme.py b/Examples/test-suite/python/autodoc_runme.py
index 1350c6d..47f3a28 100644
--- a/Examples/test-suite/python/autodoc_runme.py
+++ b/Examples/test-suite/python/autodoc_runme.py
@@ -202,3 +202,7 @@ check(inspect.getdoc(banana), "banana(S a, S b, int c, Integer d)")
 check(inspect.getdoc(TInteger), "Proxy of C++ T< int > class.", "::T< int >")
 check(inspect.getdoc(TInteger.__init__), "__init__(TInteger self) -> TInteger", None, skip)
 check(inspect.getdoc(TInteger.inout), "inout(TInteger self, TInteger t) -> TInteger")
+
+check(inspect.getdoc(process), "process(_from) -> int")
+if process(_from=10) != 10:
+    raise RuntimeError("failed!")
diff --git a/Examples/test-suite/python/keyword_rename_runme.py b/Examples/test-suite/python/keyword_re
index 5646ce7..0bdd64b 100644
--- a/Examples/test-suite/python/keyword_rename_runme.py
+++ b/Examples/test-suite/python/keyword_rename_runme.py
@@ -1,4 +1,6 @@
 #!/usr/bin/env python
 import keyword_rename
 keyword_rename._in(1)
+keyword_rename._in(_except=1)
 keyword_rename._except(1)
+keyword_rename._except(_in=1)

[vadz]

OK, I'll try to do it properly. I still don't really understand what's the relationship between Language::makeParameterName() and Swig_name_make() or, rather, why there doesn't seem to be any, and why these 2 functions do similar but different things when it looks like the former should just use the latter and then I wouldn't even have to override it all for Python.

[wsfulton]

Language::makeParameterName() is intended for parameter naming. Swig_name_make is used for symbol naming and its primary use is in the parser for applying %rename. I think the use of it in python.cxx for parameter naming is probably not the original intention, but I can see that it is handy for dealing with parameters that are keywords. Language::makeParameterName() has a sub-optimal approach to dealing with names that are keywords (just change them to argN) and so the improvement that Python::makeParameterName() is trying to implement probably should be rolled up into Language::makeParameterName() at some point.

@vadz, I'm mostly concerned with fixing the regression for 4.0. From what I understand the documentation comment changed from _from to arg1 and the parameter name was always `arg1' before the regression. As mentioned before, the parameter name in the documentation comment ought to match the actual parameter name. If you find this too hard to sort out, then maybe we can just use #1394 for now.