[mypyc] Support str.replace

[97littleleaf11]

Description

• Support str.replace primitive

Test Plan

• Add a new test function in test-data/run-strings.test, but error occurred:

run-strings.test:156: error: "str" has no attribute "replace"

[TH3CHARLie]

Thanks for contributing! Looks pretty close, the error is caused by missing fixture. You can try to update mypyc/test-data/fixtures/ir.py and add function definition for str.replace. Use str.split as a close reference.

[97littleleaf11]

I got several compiler warnings, which would lead to pytest failure.
Then I added these flags to solve it: -Wno-nullability-completeness, -Wno-expansion-to-defined

• Pytest version: 6.2.1
• Compiler: Apple Clang 12.0.0

[TH3CHARLie]

LGTM! @JukkaL Please take a look if you have time

[thomasjohns]

Based on the signature here https://docs.python.org/3/c-api/unicode.html#c.PyUnicode_Replace, I think each these arg_types might need one additional str_primitive to account for the "self" argument.

I noticed after compiling:

message = 'hello world'.replace('world', 'mars')
print(message)

in build/opts.txt we get:

...
L3:
    r5 = 'hello world'
    r6 = 'world'
    r7 = 'mars'
    r8 = 'replace'
    r9 = CPyObject_CallMethodObjArgs(r5, r8, r6, r7, 0)
...

(I think because the arg types don't quite align in the mypyc/irbuild/ll_builder.py::matching_call_c check and mypyc/irbuild/ll_builder.py::translate_special_method_call will return None instead of the op for PyUnicode_Replace). With an additional str_primitive arg, I think we will get the desired:

...
L3:
    r5 = 'hello world'
    r6 = 'world'
    r7 = 'mars'
    r8 = PyUnicode_Replace(r5, r6, r7, -1)
...

Does that make sense?

[TH3CHARLie]

@thomasjohns Thanks for pointing it out! Yeah you are right, I've just suggested the changes and @97littleleaf11 please also include an IR test for this primitive. You can find examples of IR test in mypyc/test-data/irbuild-str.test

[TH3CHARLie]

Suggested change

	arg_types=[str_rprimitive, str_rprimitive],
	arg_types=[str_rprimitive, str_rprimitive, str_rprimitive],

[97littleleaf11]

@thomasjohns @TH3CHARLie Thanks for reviewing!
With the additional str_rprimitive on line 125 would cause a pytest error:

FAILED mypyc/test/test_run.py::TestRun::testStringOps

I checked the IR and found the issue:

a = "foofoofoo"
b = a.replace("foo", "bar", 3) 

      r1 = 'foofoofoo'                          (diff)
      a = r1                                    (diff)
      r2 = 'foo'                                (diff)
      r3 = 'bar'                                (diff)
      r4 = PyUnicode_Replace(a, r2, r3, 6)      (diff)

We cannot directly feed the int_rprimitive into PyUnicode_Replace .
Then I wrote a wrapped function in primitives/str_ops.c to fix it, using CPyStr_Split as a reference:

PyObject *CPyStr_Replace(PyObject *str, PyObject *old_substr, PyObject *new_substr, CPyTagged max_replace)
{
    Py_ssize_t temp_max_replace = CPyTagged_AsSsize_t(max_replace);
    if (temp_max_replace == -1 && PyErr_Occurred()) {
        PyErr_SetString(PyExc_OverflowError, "Python int too large to convert to C ssize_t");
            return NULL;
    }
    return PyUnicode_Replace(str, old_substr, new_substr, temp_max_replace);
}

It can pass the pytest now.
Should I make a new commit about it?

[TH3CHARLie]

I see. It's caused by mypyc's tagged integer which uses its lowest bit to represent tag and remaining bits to store the immediate value or the object pointer: https://github.com/python/mypy/blob/master/mypyc/doc/dev-intro.md#value-representation

So yes, you need a wrapper function and you find the correct way to do so. Please send a new commit to include your changes(both the IR test and the wrapper function)

[TH3CHARLie]

Thanks for the updates! LGTM! Only a small nit to patch and I believe it's good to go.

[TH3CHARLie]

add a newline at the end of IR test