fix PyUnicodeDecodeError_Create

[mejrs]

Fixes #2770

[adamreichold]

Do we have a test that should have failed?

[samuelcolvin]

Do we have a test that should have failed?

pyo3/src/exceptions.rs

Line 993 in b485199

fn unicode_decode_error() {

??

[adamreichold]

Do we have a test that should have failed?

pyo3/src/exceptions.rs

Line 993 in b485199

fn unicode_decode_error() {

??

So are we not running this on the right PyPy version or why was this not visible in the CI?

[samuelcolvin]

It looks for a quick inspection like both pyo3 and pydantic-core (where I found the error), test with the same three pypy versions:

          - 'pypy3.7'
          - 'pypy3.8'
          - 'pypy3.9'

However it seems tests are not actually being run, either on merge with main or on pull requests, see this build. But it seems that changed yesterday, so not sure about the problem going back further.

Maybe @messense has some idea on what's going on in CI?

[messense]

We switched to use bors, full test matrix run in staging branch, like this one: https://github.com/PyO3/pyo3/actions/runs/3522833722

[samuelcolvin]

On that build it seems tests are not being run with pypy:

[messense]

I think tests like unicode_decode_error have never been ran on PyPy because PyPy lacks the auto-initialize feature because you can't embed a PyPy interpreter in Rust.

[samuelcolvin]

that would explain it.

[messense]

On that build it seems tests are not being run with pypy:

Yes, they are also not being run before the bors change: https://github.com/PyO3/pyo3/actions/runs/3507802969/jobs/5875825394

[messense]

I guess we need to add a test in the pytests extension module: https://github.com/PyO3/pyo3/tree/main/pytests

[adamreichold]

I guess we need to add a test in the pytests extension module: https://github.com/PyO3/pyo3/tree/main/pytests

The idea might be bonkers: But Rust unit tests are basically a automatically generated fn main and test case discovery based on #[test].

Could we somehow package ourselves up into an extension that exposes a single function to execute all the unit tests? Can we get at the main function generated by libtest somehow, maybe using nightly features?

I think the above is problematic as we would want to do this for all unit tests, not just this particular one. But pytests are less convenient to work during development and the effort to convert all unit tests into such integration tests seems quite large.

[davidhewitt]

Hmm, that's an interesting idea. The amount of dark magic involved scares me a little.

I think the best solution would be for PyPy to implement Py_InitializeEx so we could just run tests in the same way. I actually took a look at this one weekend a good many months ago, but being unfamiliar with the PyPy codebase I confess I decided I couldn't manage it in a weekend and don't have the capacity to look at that myself any more :p

I have mostly taken the approach that if it works on CPython it should work on PyPy. We at least link all tests on PyPy (even if we can't run them) which should demonstrate we're at least hitting valid PyPy APIs. There are of course edge cases like this, so I kinda feel like it's ok for us to just add pytests specifically for those until we've found someone willing to help PyPy implement Py_InitializeEx.

[messense]

I think the best solution would be for PyPy to implement Py_InitializeEx

The good news is that there is an active py_initialize branch: https://foss.heptapod.net/pypy/pypy/-/commits/branch/py_initialize/pypy/module/cpyext

Tracking issue: https://foss.heptapod.net/pypy/pypy/-/issues/3837

[birkenfeld]

I think the above is problematic as we would want to do this for all unit tests, not just this particular one. But pytests are less convenient to work during development and the effort to convert all unit tests into such integration tests seems quite large.

Actually, IMO having tests separately (in whatever form...) would speed things up, since edits to the inline #[test]s force cargo test to recompile the whole pyo3 crate, followed by all the non-inline test executables, which takes quite a bit of time.

If, as a nice side-effect, the same tests can also be compiled in a way that works on PyPy, so much the better.

[davidhewitt]

@adamreichold my opinion here is let's merge this for 0.18?

Regarding the wider issue of testing on PyPy - I've offered to help out on their branch. So in the short term I'd rather we didn't change anything in PyO3, and hopefully in the longer term testing on PyPy should "just work" the same as it does in CPython 😄

[adamreichold]

@adamreichold my opinion here is let's merge this for 0.18?

Agreed.

[davidhewitt]

Rebased, added changelog entry, and will merge.

[davidhewitt]

bors r+

[mejrs]

Wouldn't this be a result of undefined behavior? i.e. isn't this a soundness fix?

[davidhewitt]

Ah, yes, it's not super clear from the Python docs. It looks like it might be, so I'll amend this in a follow-up PR.

[bors]

Build succeeded:

• conclusion