BUG: Python wrapping needs std::string pass by value in some cases

[aylward]

std::string * isn't wrapped in the SWIG wrapping used by ITK. As a result, the returned object will be unknown if std::string is passed by reference or returned by reference in certain cases.

This commit fixes this issue for SpatialObjectProperty member functions.

Here is the behavior before these changes:

Here is the behavior with these changes:

[N-Dekker]

This change might cause a performance penalty when the string is large. What about using C++17 std::string_view? string_view is designed to be passed by-value, and it supports both std::string and const char* arguments efficiently (avoiding dynamic memory allocation).

[aylward]

Agreed - but I considered the issue of having it throw a warning message in python (see the commit message examples) versus the chance that the "name" of an object would be a long string that could cause copy and/or memory issues, and I thought getting rid of the python message was more likely/prevalent/important. Perhaps this could be considered a necessary evil?

We could also start changing things to string_view throughout the code, but perhaps that is better as a separate pull request that did such a change on a large portion of ITK, rather than hiding/embedding it as a change one function at a time?

[N-Dekker]

ITK does have const std::string & parameters in many other places. Are all of these problematic for SWIG?

[dzenanz]

@thewtex Does nonobind have the same problem with returning string references?

[aylward]

Great question!!!

@blowekamp - wanted flag this for you, since the message trail is getting "long".

[thewtex]

nanobind may have improved behavior, but I do not know the case for const std::string &.

[N-Dekker]

If you do pass std::string by-value, please consider std::move here, to avoid extra memory reallocation:

    m_Name = std::move(name);

[aylward]

Cool idea. Will do as an amend :)

[blowekamp]

I would expect "out" type map for const std::string & and std::string to be returning a native Python str object.

I am would be surprised if the "in" type is not being handled correctly.

[aylward]

I would expect "out" type map for const std::string & and std::string to be returning a native Python str object.

I am would be surprised if the "in" type is not being handled correctly.

Me too!! I haven't followed what logic/heuristic is causing this in swig. I tried a ton of options from the web, but the function continues to throw the error message when python wrapped - see the details given in the pull request - I created a small test to verify this odd SWIG behavior - I show how a very short bit of code can reproduce this error without the proposed change, and how the proposed change fixes it.

[blowekamp]

I would expect "out" type map for const std::string & and std::string to be returning a native Python str object.
I am would be surprised if the "in" type is not being handled correctly.

Me too!! I haven't followed what logic/heuristic is causing this in swig. I tried a ton of options from the web, but the function continues to throw the error message when python wrapped - see the details given in the pull request - I created a small test to verify this odd SWIG behavior - I show how a very short bit of code can reproduce this error without the proposed change, and how the proposed change fixes it.

I don't see an error message or a "thrown" object in the commit message or subject. I see <Swig Object of type X at A> but that is the default printer for a SWIG wrapped object.

[aylward]

I would expect "out" type map for const std::string & and std::string to be returning a native Python str object.
I am would be surprised if the "in" type is not being handled correctly.

Me too!! I haven't followed what logic/heuristic is causing this in swig. I tried a ton of options from the web, but the function continues to throw the error message when python wrapped - see the details given in the pull request - I created a small test to verify this odd SWIG behavior - I show how a very short bit of code can reproduce this error without the proposed change, and how the proposed change fixes it.

I don't see an error message or a "thrown" object in the commit message or subject. I see <Swig Object of type X at A> but that is the default printer for a SWIG wrapped object.

Exactly! - and that message appears on the console whenever the Set function is called. As a result, when making changes to a dictionary, that message is printed every time and the console gets spammed. Here is a change of no major impact that eliminates that message - yup, not a error message (my mistake for calling it that, but in my application, it makes the user think they are seeing an error, my users aren't developers - so, it is "evil" spam. I can redirect all output from wrapping/python, but then I miss messages that I want to appear on the console. So, this PR fixes unwanted behavior that is an unwanted side effect (a subjective error :) ).

[blowekamp]

OK. There is no "error" message, just extra console output.

Why do you think there is a memory leak as written in the commit message?

Regarding the printing of the Object to the console, when there is a return value not assigned to a variable then interactive consoles will print that value. If however the results are assigned to a variable then it will not be printed. Why are there loops of GetName where the results are being unused?

Forgive me for all the questions, I am trying to undertand the underlying issue and it there are wide spread wrapping issues that need to be further investigated.

[aylward]

OK. There is no "error" message, just extra console output.

Yes - just spam.

Why do you think there is a memory leak as written in the commit message?

Sorry -my mistake - I'm prepping another commit that deal with a memory leak, not this one - I confused the messages. It is only printing spam and returning <swig objects> that cannot be easily used.

Regarding the printing of the Object to the console, when there is a return value not assigned to a variable then interactive consoles will print that value. If however the results are assigned to a variable then it will not be printed.

Agreed. But this message is being printed when the Set function, which returns void, is called. I have no idea why. Yes, it is perhaps a bigger issue with SWIG.

Regarding the GetName() - it is problematic to return "<Swig object....>" instead of a str that contains the answer. As-is, we cannot call "GetName()" from python - we will end up with a SWIG object that has no easy way of being cast to a str that python can use. The GetName() function is useless from python.

Why are there loops of GetName where the results are being unused?

Sorry - what are you referring to? What loops in the code? Ah - you think in my application's code I'm calling GetName and it is causing the messages because I'm not using the return value - LOL. Nope, my loops are calling SetTagStringValue() (see cell 6 in the sample code) - which prints the spam message even though the Set function return void! So, I am calling a function that returns void, yet it is spamming the console.

Forgive me for all the questions, I am trying to undertand the underlying issue and it there are wide spread wrapping issues that need to be further investigated.

I suspect that there are wider wrapping issues at play - but I do not have the cycles for trying to debug such things further.

I fully admit that with perfect wrapping, none of this should be happening - but I assume I'm hitting a corner case in ITK's wrapping. If y'all could figure it out, that would be outstanding! Perhaps, in the meantime, could we go with this. No backward compatibility is broken :)

[dzenanz]

/azp run ITK.macOS

[blowekamp]

@aylward The commit message reads:

std::string * doesn't have a desctructor in the SWIG wrapping used
by ITK. As a result, memory leak warnings will be thrown if
std::string is passed by reference or returned by reference in
certain cases.

This commit fixes this issue for SpatialObjectProperty member
functions.

This sounded like a serious issue and got my attention.

I undertand return "std::string" for return values over pointers and references at this allows SWIG to map the results to a native python type.

After reproducing this example I did see the following message when exiting ipython:
swig/python detected a memory leak of type 'std::string *', no destructor found.

Clearly std::string * is bad.

It was initially not clear to me the issue with SetTagStringValue. This odd behavior is odd:

In [1]: import itk

In [2]: so = itk.TubeSpatialObject[3].New()
prop
In [3]: prop = so.GetProperty()

In [4]: prop.SetTagStringValue("Name", "foo")
Out[4]: <Swig Object of type 'std::string *' at 0x12c947600>

In [5]: r =  prop.SetTagStringValue("Name", "foo")

In [6]: print(r)
<Swig Object of type 'std::string *' at 0x105113270>

For some reason SWIG thinks SetTagStringValue is returning a std::string *.

SimpleITK consistently takes const std::string & as inputs and returns std::string as outputs and has not seen these issues.

[aylward]

Why do you think there is a memory leak as written in the commit message?

Sorry -my mistake - I'm prepping another commit that deal with a memory leak, not this one - I confused the messages. It is only printing spam and returning that cannot be easily used.

Fixed in an amended commit

[thewtex]

👍

I think the idea @N-Dekker brings for broader string_view is a good one, but a topic for a separate PR when used more broadly throughout the toolkit.

[aylward]

SWIG converts return by value to return by pointer:
https://www.swig.org/Doc1.3/SWIG.html#SWIG_nn23

• so, our handling of string is really wonky and could produce a memory leak :(

I wonder if there is a rogue swig template specification, see this:
https://www.swig.org/Doc1.3/Arguments.html#Arguments_nn2

• this would cause a function in C++ that returns a void to wrap to a python function that returns a value - hence causing the SetTagValueString to appear to return a value in python.

[blowekamp]

The docs you are linking to are 1.3, the current release is 4.2.1. I think the general points are still correct.

However, the documentation is about returning pointers is for general custom structures/objects. SWIG provides a std library of typemaps for C++ std library types such as std::string.

https://www.swig.org/Doc4.2/Library.html#Library_std_string

This typemap enables std::string to interface with a native (Python str) object in most cases were write access to the wrapped object is not needed.

I wonder if there is a rogue swig template specification, see this: https://www.swig.org/Doc1.3/Arguments.html#Arguments_nn2

• this would cause a function in C++ that returns a void to wrap to a python function that returns a value - hence causing the SetTagValueString to appear to return a value in python.

Maybe.

[N-Dekker]

Please std::move here as well 😺, when you pass the value "by value" . As in:

m_StringDictionary[tag] = std::move(value);

[N-Dekker]

Do I understand correctly that the SWIG problem occurred with the parameter value, but not with tag? If so, what would be the general guideline on when to pass std::string by-value, and when to pass it by const &?

[aylward]

I have no idea. :) We have only 1 data point at this time. SWIG seems to be somehow converting this function to a new version that returns a std::string * (rather than returning a void as defined by the C++ code). In another comment,, I reference documentation (albeit from an old version of SWIG, but perhaps still relevant) that suggests that SWIG supports wrapping templates that could be doing this conversion for Set functions that take multiple args. Furthermore, other documentation explains why returning std::string & might be converted to std::string * in the Get cases. I suspect there is a bug in our SWIG wrapping specification for these reasons and because new SWIG should handled std::string fine and these wonkynesses shouldn't be happening. I'll file an issue.