-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't discard null return value in function with output parameters #2905
Comments
Hmm, the behaviour is the same (and still wrong IMO) for Ruby too: #!/usr/bin/env ruby
require './outparam';
puts Outparam.TestOutput(true).class, Outparam.TestOutput(false).class outputs
|
OTOH JS behaves correctly: #!/usr/bin/env node
let outparam = require('outparam');
console.log(outparam.TestOutput(true));
console.log(outparam.TestOutput(false)); outputs 2 2-element arrays. |
There's a similar (maybe fundamentally the same) issue in SWIG/PHP: It was a while ago, but ISTR the problem is essentially that |
Thanks, this is indeed the same problem. I'm not sure of the best way to fix it yet, but it seems weird to me that both Using a different macro, e.g. some |
You might reasonably use the macros from |
Yes, you're right, of course, sorry for somehow not thinking about this. This means that my proposed solution still wouldn't work for a function like The real fix would need to distinguish between |
Null pointers returned from functions also using output parameters or C strings typemaps were lost because the generated code didn't distinguish between Py_None corresponding to them and Py_None indicating the return value of a "void" function. This doesn't seem to be easy to solve properly because the function return type and output parameters typemaps are independent and there is no way to customize the behaviour of the latter depending on the former. So we do it using a hack involving a global variable which is always 0 and is passed to SWIG_Python_AppendOutput() from non-void functions, but is shadowed by a local variable with the same name set to 1 in the void ones. This allows us to only discard Py_None if it corresponds to a void function return value. This is obviously ugly, but requires only minimal changes, doesn't impose much extra run-time overhead and is MT-safe (unlike any solution involving global variables). Also add a unit test checking for this for Python and for JS, where the same function already worked correctly (unlike in some other languages, including at least Ruby and PHP). Closes swig#2905.
Null pointers returned from functions also using output parameters or C strings typemaps were lost because the generated code didn't distinguish between Py_None corresponding to them and Py_None indicating the return value of a "void" function. This doesn't seem to be easy to solve properly because the function return type and output parameters typemaps are independent and there is no way to customize the behaviour of the latter depending on the former. So we do it using a hack involving a global variable which is always 0 and is passed to SWIG_Python_AppendOutput() from non-void functions, but is shadowed by a local variable with the same name set to 1 in the void ones. This allows us to only discard Py_None if it corresponds to a void function return value. This is obviously ugly, but requires only minimal changes, doesn't impose much extra run-time overhead and is MT-safe (unlike any solution involving changing global variables). Also add a unit test checking for this for Python and for JS, where the same function already worked correctly (unlike in some other languages, including at least Ruby and PHP). Closes swig#2905.
Null pointers returned from functions also using output parameters or C strings typemaps were lost because the generated code didn't distinguish between Py_None corresponding to them and Py_None indicating the return value of a "void" function. This doesn't seem to be easy to solve properly because the function return type and output parameters typemaps are independent and there is no way to customize the behaviour of the latter depending on the former. So we do it using a hack involving a global variable which is always 0 and is passed to SWIG_Python_AppendOutput() from non-void functions, but is shadowed by a local variable with the same name set to 1 in the void ones. This allows us to only discard Py_None if it corresponds to a void function return value. This is obviously ugly, but requires only minimal changes, doesn't impose much extra run-time overhead and is MT-safe (unlike any solution involving changing global variables). Also add a unit test checking for this for Python and for JS, where the same function already worked correctly (unlike in some other languages, including at least Ruby and PHP). Closes #2905.
Still needs fixing for scripting languages supporting argout. f3d4698...d940190 from #2907 contains the fix for Python, but still needs extending for Ruby, PHP etc. @vadz, perhaps you could add in this and create testcases? If not, I'll get to it later. |
I can do Ruby but I'm completely unfamiliar with PHP, so I'd rather leave this one to someone else. |
I can probably do PHP. I actually looked at this before for PHP but couldn't see an easy fix and concluded it was a low priority corner case as Python had essentially the same problem (#2027 (comment)) but if |
@wsfulton I looked at sorting this out for PHP but noticed your fix seems to mishandle the case where the first parameter outputs Python
I'd expect It seems FWIW, I tested this with cd39cf1 and it seems @vadz approach also mishandles this case in the same way (as does SWIG 4.2.1, tested with the Debian package). |
Having though about this more, I'm thinking even more strongly that we need to return
|
I can fix this with something like this: commit cbc03c3f062ee4fcf6f46e8edf7c9f4f62e2870f
Author: Vadim Zeitlin <vz-swig@zeitlins.org>
Date: 2024-06-17 20:51:23 +0200
Python: Fix handling multiple "out" parameters in void functions
The previous fix didn't work correctly for the void functions with more
than one output parameter if "None" was returned for any but the last
one of them.
Fix this by only taking "isvoid" flag into account for the first output
parameter, but not any others.
See #2905.
diff --git a/Lib/python/pyrun.swg b/Lib/python/pyrun.swg
index efe02e41f..9740e758c 100644
--- a/Lib/python/pyrun.swg
+++ b/Lib/python/pyrun.swg
@@ -117,10 +117,11 @@ SWIG_Python_SetConstant(PyObject *d, const char *name, PyObject *obj) {
/* Append a value to the result obj */
SWIGINTERN PyObject*
-SWIG_Python_AppendOutput(PyObject* result, PyObject* obj, int is_void) {
+SWIG_Python_AppendOutput(PyObject* result, PyObject* obj, int* is_void) {
if (!result) {
result = obj;
- } else if (result == Py_None && is_void) {
+ } else if (result == Py_None && *is_void) {
+ *is_void = 0;
SWIG_Py_DECREF(result);
result = obj;
} else {
diff --git a/Source/Modules/python.cxx b/Source/Modules/python.cxx
index 00c38173c..ad4320474 100644
--- a/Source/Modules/python.cxx
+++ b/Source/Modules/python.cxx
@@ -2769,6 +2769,11 @@ public:
Wrapper_add_local(f, "resultobj", "PyObject *resultobj = 0");
+ String *isvoid_decl = NewString("");
+ Printf(isvoid_decl, "int isvoid = %d", Cmp(returntype, "void") ? 0 : 1);
+ Wrapper_add_local(f, "isvoid", isvoid_decl);
+ Delete(isvoid_decl);
+
// Emit all of the local variables for holding arguments.
emit_parameter_variables(l, f);
@@ -3238,9 +3243,7 @@ public:
/* Substitute the cleanup code */
Replaceall(f->code, "$cleanup", cleanup);
-
- bool isvoid = !Cmp(returntype, "void");
- Replaceall(f->code, "$isvoid", isvoid ? "1" : "0");
+ Replaceall(f->code, "$isvoid", "&isvoid");
/* Substitute the function name */
Replaceall(f->code, "$symname", iname); but this breaks the new I thought about resetting @wsfulton Can I adjust the test or do you see some other way to do it? |
If we go that route, I think the I'm going to hold off actually implementing for PHP until we've agreed a solution to this Incidentally, while looking at this for PHP I noticed the |
Unassigning myself as I can't really usefully do the PHP part until we've agreed a solution to the @wsfulton I think it would be good for you to take a quick look at this before 4.3.0 - We could also add such a note for 4.3.0 just in case. |
Consider the following example:
and Python code to test it:
I'd expect this to output
but it actually outputs
because
SWIG_Python_AppendOutput()
replaces null values instead of appending to them and so when the function returnsnullptr
its output parameter is returned directly, as a string, instead of returning a list.This is pretty annoying as it means you can't use natural
in Python as this gives a run-time error if the function happens to return null for some value of
whatever
.This behaviour dates from a very long time ago, see 6e77a6b (fixes for swigrun example, 2005-12-07), but I don't think this was done intentionally or makes sense even if it was, and would like to fix this, but please let me know if you think it is and the current behaviour is actually expected.
The text was updated successfully, but these errors were encountered: