Unicode incorrectly escaped on PyPy 7.3.1

[ShadowJonathan]

On PyPy (specifically, the wheel with _speedups.pypy36-pp73-x86_64-linux-gnu.so), when using markupsafe.escape(), this unicode below is incorrectly escaped;

Input: 'https://x?<ab c>&q"+%3D%2B"="fö%26=o"'

_native:
'https://x?<ab c>&q"+%3D%2B"="fö%26=o"'
_speedups (on pypy):
'https://x?<ab c>&q"+%3D%2B"="fö%26=o'

This was caught while testing PyPy support for synapse, in matrix-org/synapse#9123, in which one of the tests hooked behind here.

I am no expert in C, but I have a feeling one of the Unicode APIs used in _speedups.c is either being used incorrectly, or has an implementation mismatch compared to cPython

[davidism]

Going to need some help on this one, interactions of C and PyPy are beyond me. @mattip is there a PyPy person who might be able to spot something?

[mattip]

I thought the C-extension is not used on PyPy? I will take a look.

[davidism]

Wheels can now be built against PyPy, I actually tested it a while ago and only just pushed them since I was doing the other new builds.

I guess I'll have to delete the 1.1.1 wheels, I can't replace them even if we fix the issue, I'd have to make a 1.1.2 release. I probably won't do that, I'll just leave PyPy wheels disabled for 2.0 until we figure it out.

[davidism]

That said, @ShadowJonathan can you test with a build from master? 2.0 will remove Python 2 support, which removed about half the C code, so it should be much easier to reason about. Just want to make sure it's not already fixed somehow.

[ShadowJonathan]

I probably won't do that, I'll just leave PyPy wheels disabled for 2.0 until we figure it out.

I recommend just disabling building extensions on PyPy, instead of disabling wheels, as installing wheels is much faster than building from source (especially if that source tries to build extensions)

That said, @ShadowJonathan can you test with a build from master? 2.0 will remove Python 2 support, which removed about half the C code, so it should be much easier to reason about. Just want to make sure it's not already fixed somehow.

Currently trying that, but keep in mind that markupsafe is required by jinja2 instead of us directly, and it's possible they dont support version 2 (yet).

I thought the C-extension is not used on PyPy? I will take a look.

@mattip there's a .so file in the pypy wheel, so yeah, it is.

[ShadowJonathan]

Inserting "markupsafe @ git+https://github.com/pallets/markupsafe" does not build the wheel, as per the setup.py's internal instructions (not to build on pypy). It falls back to _native, which then just works as intended.

[mattip]

weird, when I do pypy setup.py bdist_wheel I get a pure-python wheel. I wonder where the .so is coming from?

[ShadowJonathan]

setup.py has

if os.environ.get("CIBUILDWHEEL", "0") == "1":
    run_setup(True)

which gives cibuildwheel a carta blanca to build any kind of wheel it wants, so the released wheels contain .so files.

[davidism]

You have to set CIBUILDWHEEL=1 right now, otherwise PyPy is ignored from setup.py. I was actually planning to remove that ignore before this issue was opened.

[ShadowJonathan]

@davidism i just did that, while installing from master, but the issue persists.

I think disabling building extensions on pypy at all would help a lot.

[davidism]

Yeah, I'll just adjust it for now so the extension isn't built directly or with cibuildwheel on PyPy. Unfortunately I'll just have to delete the 1.1.1 wheels and people will still have to install from the sdist in that case, but that was already the case until yesterday anyway.

[mattip]

Of course, the wheel building the so is no excuse, PyPy should not fail like that. But it would be more performant in pure python

[davidism]

Interesting, if you think PyPy wouldn't benefit from the C extension anyway then that's another reason to disable it for now. I'm also interested in, but don't know much about, the limited C API and HPy, so I'm happy to revisit it later if someone figures those out.