Repository: cython/cython
Pull request: Only used PyUnicode_Concat on unicode objects
Closes https://github.com/cython/cython/issues/3426

-------------------------

~(I'm getting a segmentation fault on the fstring tests with the current master, which is making it difficult to run this through a thorough set of tests. It happens with and without this PR. I'm not sure where this is due to something being messed up on my side or a real bug, but I'll look into and report it separately. Just noting it here because it means I couldn't test this PR with with "runtest.py string")~ Caching issues to do with Cython.inline - I'd clearly ended up with a broken version cached - ignore

Problem statement:
TypeError on list += str
When run via CPython the following function:

```
def appendinline():
    y = []
    y += 'ab'
    return y
```
returns `['a', 'b']`, but when compiled with Cython (0.29.13) the result is
```
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "concat.py", line 11, in concat.appendinline
    y += 'ab'
TypeError: must be str, not list
```
list +=, like extend, ordinarily accepts any iterable, not just a list.

The following functions work correctly when compiled with Cython, however:
```
def appendchars(x):
    x += 'hello'

def appendto(x):
    y = []
    y += x
    return y
```

The overly restrictive check seems to trigger only if both the target list and the argument to += are local to the function.

Hints from issue discussion:
I'm struggling to reproduce this (on Cython 0.29.13 -> master). Are you using any compiler directives/options that might be relevant?
I'm building a module with cythonize -i, using Cython 0.29.13 and then importing that module into CPython 3.7.6.

```
Thu Mar 12 17:46:26 cunkel@cunkel:~/misc
1017$ cat concat.py 
def appendchars(x):
    x += 'hello'

def appendto(x):
    y = []
    y += x
    return y

def appendinline():
    y = []
    y += 'ab'
    return y
Thu Mar 12 17:46:30 cunkel@cunkel:~/misc
1018$ cythonize -3 -i concat.py
Compiling /home/cunkel/misc/concat.py because it changed.
[1/1] Cythonizing /home/cunkel/misc/concat.py
running build_ext
building 'concat' extension
creating /home/cunkel/misc/tmpk25abxp1/home
creating /home/cunkel/misc/tmpk25abxp1/home/cunkel
creating /home/cunkel/misc/tmpk25abxp1/home/cunkel/misc
gcc -pthread -Wno-unused-result -Wsign-compare -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DNDEBUG -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -D_GNU_SOURCE -fPIC -fwrapv -fPIC -I/usr/include/python3.7m -c /home/cunkel/misc/concat.c -o /home/cunkel/misc/tmpk25abxp1/home/cunkel/misc/concat.o
gcc -pthread -shared -Wl,-z,relro -Wl,--as-needed -Wl,-z,now -g /home/cunkel/misc/tmpk25abxp1/home/cunkel/misc/concat.o -L/usr/lib64 -lpython3.7m -o /home/cunkel/misc/concat.cpython-37m-x86_64-linux-gnu.so
Thu Mar 12 17:46:36 cunkel@cunkel:~/misc
1019$ python
Python 3.7.6 (default, Jan 30 2020, 09:44:41) 
[GCC 9.2.1 20190827 (Red Hat 9.2.1-1)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import concat
>>> concat.__file__
'/home/cunkel/misc/concat.cpython-37m-x86_64-linux-gnu.so'
>>> concat.appendchars([])
>>> concat.appendto('abc')
['a', 'b', 'c']
>>> concat.appendinline()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "concat.py", line 11, in concat.appendinline
    y += 'ab'
TypeError: must be str, not list
>>> 
```

```
1024$ cython --version
Cython version 0.29.13
Fri Mar 13 11:39:48 cunkel@cunkel:~/misc
1025$ python --version
Python 3.7.6
```

I can try building Cython master from source and seeing if I can still reproduce.
Thanks - can now reproduce. It's the `-3` that makes the difference (i.e. it know what to do with bytes but not with unicode).

With `-3` the code gets translated to `__Pyx_PyUnicode_Concat` (which obviously expects unicode) while without it gets translated to `PyNumber_InPlaceAdd` which is pretty generic and can handle anything any type with suitable operators.
Thanks for the report @dscunkel and the analysis @da-woods. I think adding an additional type check in a new specialised macro might fix this. Either that, or disabling the optimisation if the LHS isn't known to be a unicode string, since this is actually an incorrect optimisation in general.

PR welcome.

Commit message: Only used PyUnicode_Concat on unicode objects

https://github.com/cython/cython/issues/3426

Patch to review:
diff --git a/Cython/Compiler/ExprNodes.py b/Cython/Compiler/ExprNodes.py
index 1915369df90..cc4205c6e48 100644
--- a/Cython/Compiler/ExprNodes.py
+++ b/Cython/Compiler/ExprNodes.py
@@ -11364,13 +11364,8 @@ def compute_c_result_type(self, type1, type2):
                 self, type1, type2)
 
     def py_operation_function(self, code):
-        is_unicode_concat = False
-        if isinstance(self.operand1, FormattedValueNode) or isinstance(self.operand2, FormattedValueNode):
-            is_unicode_concat = True
-        else:
-            type1, type2 = self.operand1.type, self.operand2.type
-            if type1 is unicode_type or type2 is unicode_type:
-                is_unicode_concat = type1.is_builtin_type and type2.is_builtin_type
+        type1, type2 = self.operand1.type, self.operand2.type
+        is_unicode_concat = type1 is unicode_type and type2 is unicode_type
 
         if is_unicode_concat:
             if self.operand1.may_be_none() or self.operand2.may_be_none():
diff --git a/tests/run/test_unicode_string_tests.pxi b/tests/run/test_unicode_string_tests.pxi
index 61cc6f2ace2..ba480eeecc4 100644
--- a/tests/run/test_unicode_string_tests.pxi
+++ b/tests/run/test_unicode_string_tests.pxi
@@ -993,6 +993,13 @@ class CommonTest(BaseTest):
         self.checkequal('\u019b\u1d00\u1d86\u0221\u1fb7',
                         '\u019b\u1d00\u1d86\u0221\u1fb7', 'capitalize')
 
+    def test_list_concat(self):
+        # https://github.com/cython/cython/issues/3426
+        y = []
+        y += 'ab'
+        self.assertEqual('a', y[0])
+        self.assertEqual('b', y[1])
+
 
 class MixinStrUnicodeUserStringTest:
     # additional tests that only work for


Code review comments:
diff_hunk: @@ -11364,13 +11364,8 @@ def compute_c_result_type(self, type1, type2):
                 self, type1, type2)
 
     def py_operation_function(self, code):
-        is_unicode_concat = False
-        if isinstance(self.operand1, FormattedValueNode) or isinstance(self.operand2, FormattedValueNode):
-            is_unicode_concat = True
-        else:
-            type1, type2 = self.operand1.type, self.operand2.type
-            if type1 is unicode_type or type2 is unicode_type:
-                is_unicode_concat = type1.is_builtin_type and type2.is_builtin_type
+        type1, type2 = self.operand1.type, self.operand2.type
+        is_unicode_concat = type1 is unicode_type and type2 is unicode_type
path: Cython/Compiler/ExprNodes.py
text: @user1:
Looks like the right place to fix this, but I think we should also allow `str_type` here if one side is `unicode_type`.

@author:
bytes + unicode doesn't work in Python 3 - try `u"ab" + b"cd"` and `b"cd" + u"ab"`.

In Python 2 it does work, and `PyUnicode_Concat` provides a quick path. I could probably add a `__Pyx_PyUnicode_ConcatStr` function (or similar) that goes to `PyUnicode_Concat` in Python 2 and `PyNumber_Add` in Python 3, but this seems a bit of a waste of time given that Python 2 support is soon to go.

@author:
However... there is one optimization it might actually be worth looking into here (although only tangentially related to this bug). CPython has a specific optimization to check the reference count of the RHS and append rather than concat if possible: https://github.com/python/cpython/blob/309d7cc5df4e2bf3086c49eb2b1b56b929554500/Python/ceval.c#L5354


@user1:
> bytes + unicode doesn't work in Python 3

Note: I wrote `str_type`, not `bytes_type`. They are different things in Cython. (Exactly for cases like this.)

diff_hunk: @@ -11364,13 +11364,8 @@ def compute_c_result_type(self, type1, type2):
                 self, type1, type2)
 
     def py_operation_function(self, code):
-        is_unicode_concat = False
-        if isinstance(self.operand1, FormattedValueNode) or isinstance(self.operand2, FormattedValueNode):
-            is_unicode_concat = True
path: Cython/Compiler/ExprNodes.py
text: @user1:
I wonder if removing this special case can lead to regressions. Doesn't seem very likely, given that f-strings are always built from unicode string parts. But I don't remember all the optimisations that apply to these parts along the way. Not all of them may be aware of the concrete result type in order to preserve it along the way.

@author:
As far as I could tell the result type of `FormattedValueNode` was permanently set to `unicode_type` anyway, so it seemed fairly safe. Also, it'd be (probably fairly small?) performance regression rather than than anything breaking (`PyNumber_Add` does work). I can definitely put it back in if wanted, but it seems more likely to cause bugs then fix them.

@user1:
> seems more likely to cause bugs then fix them.

`FormattedValueNode` should only originate from f-strings, and they always produce string parts that are then joined in one way or another. That seems safe. The question is just if the new "unicode or str" decision below succeeds for all of those parts, including operands that are no longer `FormattedValueNode`s themselves (but return a unicode string at runtime).

This PR seems like a fix that should go into 0.29.x, really, in which case I'd like to reduce the risk of regressions a bit more than strictly necessary. I don't think it hurts to keep the `isinstance()` tests, at least in that branch.