python-trio / trio

Trio – a friendly Python library for async concurrency and I/O
https://trio.readthedocs.io
Other
6.12k stars 335 forks source link

Use native traceback mutation on python 3.7 #405

Open njsmith opened 6 years ago

njsmith commented 6 years ago

Since https://github.com/python/cpython/pull/4793 was merged, it looks like traceback objects will support Python-level instantiation and mutation in 3.7. We should use that instead of the current ctypes horrors.

We should also hassle the PyPy devs about implementing this. (In some ways this is even more interesting, because we're more aggressive about dropping support for old PyPy versions.)

davidism commented 4 years ago

We should also hassle the PyPy devs about implementing this.

Is there an assumption that they won't make this assignable when they support 3.7 even though it's documented now?

I'm currently fixing Jinja's traceback rewriting due to the changes in 3.7 and an issue with ctypes on debug builds (pallets/jinja#1050, pallets/jinja#1051, pallets/jinja#1104), and it would be nice if I can assume any 3.7 means tb_next is assignable. PyPy's docs for tproxy now list it as deprecated.

pquentin commented 4 years ago

Another argument in favor of this is that we're currently using PyPy transparent proxies for concat_tb:

https://github.com/python-trio/trio/blob/4f65ff2923d20340648a59d1ee00786fcbdbafbb/trio/_core/_multierror.py#L249-L356

They're documented and Trio uses them since 2017-02. However, in 2018-05, @mattip marked them as deprecated. It does not look like they're planning to remove them right now, but migrating away from tputil is probably still desirable.

njsmith commented 4 years ago

@davidism Hey David, sorry I missed your message.

Is there an assumption that they won't make this assignable when they support 3.7 even though it's documented now?

They probably will eventually, since the CPython 3.7 test suite has tests for it, and PyPy will eventually import those tests when adding 3.7 support. But they might do it quicker if we hassle them :-)

When I talked to the PyPy devs about this originally, they told me they knew that jinja2 depends on the deprecated "transparent proxies" code, and they weren't planning to break jinja2, so even though it was deprecated I could expect at least this specific case to keep working. That's why I felt OK using it in Trio :-). But I'm sure they wouldn't mind if we all switched to something more sensible.

I'm currently fixing Jinja's traceback rewriting due to the changes in 3.7 and an issue with ctypes on debug builds (pallets/jinja#1050, pallets/jinja#1051, pallets/jinja#1104), and it would be nice if I can assume any 3.7 means tb_next is assignable. PyPy's docs for tproxy now list it as deprecated.

Oh yeah, uh... I actually fixed this already when I was cribbing from jinja2. I probably should have sent you a note at some point, sorry :-). It's actually very simple to fix, though it needs some explanation:

https://github.com/python-trio/trio/blob/ae557cc16a893cce0505da8a0b30122a727470f2/trio/_core/_multierror.py#L303-L304

The idea is that we know a Python object is just a bare PyObject_HEADER without any additional fields, so whatever size the object struct is, that's the size of a PyObject_HEADER. And if you don't care about accessing the fields inside it (refcount etc.), then you don't need to actually spell out those fields for ctypes; you just need a blob of "something" the right size so ctypes can figure out where the subsequent fields you actually care about are located. So if we tell ctypes that the first field is a byte array with object().__sizeof__() elements in it, then everything Just Works, on every Python version. And you even get to delete the gross hacks that try to guess how CPython is compiled by looking for sys.getobjects or whatever.

mattip commented 4 years ago

Ping @arigo

davidism commented 4 years ago

Thanks for the explanation. I was scared off by the use of _ctypes, so I ended up using py_object and pythonapi, since it was just as opaque to me as everything else, and at least seemed like the "intended" way. Here's the implementation I ended up with: https://github.com/pallets/jinja/blob/e08dadd220563609a67bc32d4d1c0abe91699231/src/jinja2/debug.py#L238. Support for 3.7+ and pypy is above that. I was able to greatly simplify the entire traceback rewriting implementation.