Replace MultiError with BaseExceptionGroup

[agronholm]

This PR has two dependencies:

• ~#2210 (required)~
• ~#2212 (recommended but not strictly necessary)~ (skipped)

Closes #2211.

[codecov[bot]]

Codecov Report

Merging #2213 (80889b2) into master (af66bcf) will increase coverage by 0.01%. The diff coverage is 99.52%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2213      +/-   ##
==========================================
+ Coverage   98.98%   99.00%   +0.01%     
==========================================
  Files         119      117       -2     
  Lines       16165    16119      -46     
  Branches     3101     3122      +21     
==========================================
- Hits        16001    15958      -43     
+ Misses        114      113       -1     
+ Partials       50       48       -2     

Impacted Files	Coverage Δ
trio/_core/__init__.py	100.00% <ø> (ø)
trio/tests/test_highlevel_open_tcp_listeners.py	98.25% <75.00%> (-1.75%)	:arrow_down:
trio/__init__.py	100.00% <100.00%> (ø)
trio/_core/_multierror.py	100.00% <100.00%> (ø)
trio/_core/_run.py	100.00% <100.00%> (ø)
trio/_core/tests/test_multierror.py	100.00% <100.00%> (ø)
trio/_core/tests/test_run.py	100.00% <100.00%> (ø)
trio/_highlevel_open_tcp_listeners.py	100.00% <100.00%> (ø)
trio/_highlevel_open_tcp_stream.py	97.53% <100.00%> (+0.19%)	:arrow_up:
trio/tests/test_highlevel_open_tcp_stream.py	100.00% <100.00%> (ø)
... and 3 more

[agronholm]

This needs discussion on whether to drop MultiError cold turkey or to have it inherit from BaseExceptionGroup. It's not yet certain if inheriting it is possible in the first place.

[agronholm]

Here are my thoughts about deprecating MultiError. If we were to deprecate it and have it inherit from BaseExceptionGroup instead, what would be the migration path? There are two cases how it might be used by downstream users:

1. Raising them
2. Catching them (or their sub-exceptions)

To maintain backwards compatibility, both use cases would have to be covered. Trio itself would have to raise MultiError with warning suppression turned on to counter the deprecation warning we would have have its constructor emit. Then we would have to have the extra class methods (catch() and filter()) raise deprecation warnings as well. This only leaves one hole: cases where users catch MultiError directly. We can't emit a deprecation warning from the exceptions property because that would also happen for the users doing the right thing (catching BaseExceptionGroup instead).

Either way, a migration guide would probably be in order.

[smurfix]

Meh. Frankly I don't think we can achieve 100% backwards compatibility, which is especially bad since the affected code is on code paths that are typically not tested very well.

Thus I'd advise to spend the effort on a good migration guide that covers all the bases, preferably from multiple angles. Code that wants to be able to handle both BaseExceptionGroup and MultiError will just have to duplicate the affected error handlers for a year or two.

[agronholm]

Meh. Frankly I don't think we can achieve 100% backwards compatibility, which is especially bad since the affected code is on code paths that are typically not tested very well.

Thus I'd advise to spend the effort on a good migration guide that covers all the bases, preferably from multiple angles. Code that wants to be able to handle both BaseExceptionGroup and MultiError will just have to duplicate the affected error handlers for a year or two.

This seems to be the consensus (on Gitter as well). I will attempt to write a migration guide somewhere in the docs.

[agronholm]

I amended the documentation and the news fragment to point users in the right direction. While this is not strictly a "How to migrate from MultiError to BaseExceptionGroup", trio users tend to be more tech savvy than the average Python user. I hope that is enough.

[agronholm]

I managed to make the tests pass on py3.11, but the changes broke the style check job, possibly due to upgraded dependencies. Is it okay to include those fixes in this PR?

[richardsheridan]

You could fix it in #2241 and then merge or rebase master, what about that?

[agronholm]

Hm, I think you need to elaborate. Say I merge that PR, then what? Another PR to fix the changes required by the new Black version?

[richardsheridan]

I was thinking push commits to that dependabot branch until CI comes back green. That should fix the style change in not-beta black regarding the ** operator while keeping this PR is "clean" of black style changes.

On the other hand I have no problem personally with mixing purposes in PRs! 🤷 Also now that I think about it, I'm not sure you (or I) would have permissions to do that push...

[agronholm]

It's in the same repository so we should have push access. I'll try doing that.

[agronholm]

Ok, done. Any idea why I can't merge it? All the required checks pass.

[oremanj]

I think we should try harder to make this backwards-compatible. The big benefit of ExceptionGroups (except* syntax) will not be available to basically any of our users for ~10 more months, and asking them to take a breaking change with neither forward- nor backward-compatibility is a big deal. It becomes necessary for them to update their code and Trio in lockstep. In a lot of environments this is a major hassle.

I would propose a scheme like the following sketch:

class _MultiError(BaseException):
    @classmethod
    @deprecated(...)
    def catch(...):
        # implementation as before

    @classmethod
    @deprecated(...)
    def filter(...):
        # implementation modified to work on both MultiError and BaseExceptionGroup

    # remaining methods as before

enable_attribute_deprecations(__name__)
__deprecated_attributes__ = {"MultiError": DeprecatedAttribute(_MultiError, ...)}

class _TrioBaseExceptionGroup(BaseExceptionGroup, _MultiError):
    __new__ = BaseExceptionGroup.__new__
    __init__ = BaseExceptionGroup.__init__
    __repr__ = BaseExceptionGroup.__repr__
    def derive(self, excs): return _TrioBaseExceptionGroup(self.message, excs)

class _TrioExceptionGroup(ExceptionGroup, _MultiError):
    __new__ = ExceptionGroup.__new__
    __init__ = ExceptionGroup.__init__
    __repr__ = ExceptionGroup.__repr__
    def derive(self, excs): return _TrioExceptionGroup(self.message, excs)

And make Trio raise _TrioBaseExceptionGroup / _TrioExceptionGroup. Note these are internal names; we expect user code to catch them as either [Base]ExceptionGroup or MultiError.

Trio itself would have to raise MultiError with warning suppression turned on to counter the deprecation warning we would have have its constructor emit.

In the above sketch, the deprecation is attached to the MultiError name, not its constructor. Trio uses an alternate name _MultiError which does not produce the warning.

Then we would have to have the extra class methods (catch() and filter()) raise deprecation warnings as well.

I don't see the problem? These are in fact deprecated APIs, and users can upgrade these to use exceptiongroup.catch(). Note these need to be deprecated specifically, and can't ride the MultiError deprecation, because they'll be inherited in _TrioBaseExceptionGroup so we need to catch people using caught_exc.filter() -- not that I really expect anyone to do that...

This only leaves one hole: cases where users catch MultiError directly. We can't emit a deprecation warning from the exceptions property because that would also happen for the users doing the right thing (catching BaseExceptionGroup instead).

Luckily BaseExceptionGroup also has an exceptions property so there's no need to deprecate the property! We can just deprecate the MultiError name.

---

Does anyone see anything I'm missing in the above? If not, I'm happy to work on fleshing it out and updating this PR to be backwards-compatible if @agronholm isn't up for that.

[agronholm]

Was your plan for to not raise any deprecation warnings in the MultiError constructor?

[oremanj]

It's pretty hard to construct a MultiError without writing the name trio.MultiError. If we raise a deprecation warning when the name is accessed, I don't think we need another one on the constructor call.

[agronholm]

What do you mean by "accessing" the MultiError name?

[oremanj]

I mean that this construct

enable_attribute_deprecations(__name__)
__deprecated_attributes__ = {"MultiError": DeprecatedAttribute(_MultiError, ...)}

causes a deprecation warning to be raised at the point where someone writes trio.MultiError, regardless of whether they then do anything with it. It basically works by defining a __getattr__ on the module.

[agronholm]

Ok, it was by no means obvious to me what those functions did. I found enable_attribute_deprecations() in _deprecate.py, but now that we support only py3.7+ and up, we could just define that __getattr__ directly in trio/__init__.py I suppose?

[oremanj]

I figure since we already have the explicit facility we may as well use it, but I guess a module-level __getattr__ works too.

[agronholm]

If the existing facility works for this then I will defer changing that to another PR.

[agronholm]

I hope this is enough. I'm quite mentally exhausted now.

[oremanj]

Thank you for working on this!

We should have some tests that actually use except* on 3.11. Ideally we could rewrite them (maybe automatically?) to use with catch() on <3.11 (and on 3.11 too, for good measure) but getting that right is probably annoying so I will accept a 3.11-only version for now (using exec() or similar to avoid failing at test collection time on older versions). But we can't in good conscience advertise a feature that we're not testing.

BTW I think this is a headline feature, not just a deprecation/removal. Probably it deserves entries in both sections.

It would be nice to raise something that can be caught as ExceptionGroup if it doesn't contain any BaseExceptions. You could probably do this using two subclasses of MultiError and some cleverness in __new__. That doesn't need to be in this PR though, since our current users are used to all MultiErrors being BaseExceptions.

We need a plan for how we're going to eventually get rid of MultiError. It would be good to message that alongside the discussion of this transition so that people know what to expect. It doesn't have to be part of this PR but I'd like to have something before we release, and I'm happy to work on it. There should also be documentation published for the exceptiongroup backport package before a release occurs. If that's not feasible we can make up for it in our own documentation, but the current level of documentation isn't sufficient when core functionality like catch() isn't fully documented.

[agronholm]

We should have some tests that actually use except* on 3.11. Ideally we could rewrite them (maybe automatically?) to use with catch() on <3.11 (and on 3.11 too, for good measure) but getting that right is probably annoying so I will accept a 3.11-only version for now (using exec() or similar to avoid failing at test collection time on older versions). But we can't in good conscience advertise a feature that we're not testing.

Why do we need to duplicate the tests already performed in CPython and the exceptiongroup backport? As a side note, such tests can be left uncollected if they are isolated in their own specially named test file when we have an appropriate collection hook in place.

We need a plan for how we're going to eventually get rid of MultiError. It would be good to message that alongside the discussion of this transition so that people know what to expect.

It's been deprecated as it says in the release notes – I thought that was obvious enough? Users will see deprecation warnings which tell them to switch to (Base)ExceptionGroup. So, wait maybe 1-2 releases and then just replace it with (Base)ExceptionGroup. Done?

[agronholm]

BTW I think this is a headline feature, not just a deprecation/removal. Probably it deserves entries in both sections.

Do I create a new news fragment with the same issue ID, i.e. 2211.headline.rst?

[pquentin]

Do I create a new news fragment with the same issue ID, i.e. 2211.headline.rst?

Yes, please!

[pquentin]

Do I create a new news fragment with the same issue ID, i.e. 2211.headline.rst?

I actually found out that this won't work after all, one solution is to mention the deprecation in the headline feature: https://github.com/python-trio/trio/pull/2256

[njsmith]

From a quick skim of the doc changes here, it looks like with this PR, nurseries only raise an ExceptionGroup if multiple tasks raised an exception, and if only one task raises an exception then it is not wrapped. Is that correct?

Unfortunately one of the core design decisions for ExceptionGroup was to not try to support this behavior; we have to wrap unconditionally (and that's probably what's going to break users the most).

[agronholm]

From a quick skim of the doc changes here, it looks like with this PR, nurseries only raise an ExceptionGroup if multiple tasks raised an exception, and if only one task raises an exception then it is not wrapped. Is that correct?

Unfortunately one of the core design decisions for ExceptionGroup was to not try to support this behavior; we have to wrap unconditionally (and that's probably what's going to break users the most).

I'm confused. This was done explicitly for backwards compatibility with MultiError – are you saying that I shouldn't have preserved that?

[agronholm]

I'm going to need my last question answered before this PR can go forward. Anyone?

[agronholm]

@oremanj @njsmith I've made all the changes you requested in #2301 and Gitter.