remove reference to value/error when unwrapping outcome, and add .peek() method

[graingert]

Fixes #47

currently unwraping exceptions leaves a refcycle that keeps the entire traceback and any locals alive. This PR resolves that by clearing the error field when the Error is unwrapped.

For the previous behaviour of Value (eg in trio) if you receive a large bytes object in a Value that bytes object stays alive long after it's needed. This PR resolves that by clearing the value field when the Value is unwrapped.

this is a breaking change and should probably be released with a major version bump, eg outcome==2.0.0

[TeamSpen210]

This might need to be a new method for backwards compatibility, since the attributes are public, users would be able to still read them after unwrapping. Unless we're fine with that breaking change.

[graingert]

could just call it InvalidatingOutcome, InvalidatingError and InvalidatingValue

[TeamSpen210]

A parallel class hierarchy seems problematic, they'd be incompatible with the current ones. More thinking like unwrap_and_clear(), that's all we need.

[CoolCat467]

Would using a weak reference not be acceptable?

[graingert]

Would using a weak reference not be acceptable?

I think that would be more complicated and it would be odd that unwrapped values are sometimes there sometimes not and not everything can be weakref'd

[richardsheridan]

I'm +1 on "just delete the attribute on unwrap". It's understandable and minimal and the only issue is backwards compatibility, so I'd say slap a unwrap_and_clear() method on version 1.4.0 and make a 2.0.0 release for the cleaner implementation to free more users from refcycles.

[RazerM]

I'm -1 on deleting public attributes during unwrap. I don't like that accessing an attribute/property may raise, but if we go that route then I'd much prefer AlreadyUsedError over AttributeError.

The way I've always used outcome is to only .unwrap() or .send(), so I would be fine with an outcome 2.0 that doesn't have the attributes, but I know Trio does use them.

[graingert]

I was off sick and didn't have time to focus on this PR, I'd like to pick it back up again

[TeamSpen210]

I do wonder, since this is a breaking change, maybe it'd be better to make it entirely breaking - change the properties to peek_value() and peek_error() or something. That way you get type checker or runtime errors at all call sites. Though it would cause a bit of churn in cases where users only accessed attributes and never unwraped. So not sure.

[CoolCat467]

I don't see any immediate issues. Would raise the possibility of sharing peek and return, return/unwrap just being peek and then unset, but as is is still fine.

[graingert]

ok let's add peek

[graingert]

the trio test suite (and typecheck) pass unmodified when run with this branch python-trio/trio#3385

[A5rocks]

Just realized codecov isn't working: https://github.com/python-trio/outcome/actions/runs/21510106791/job/61974795642?pr=45#step:4:658

I'll submit a PR to fix that, but IMO it doesn't really matter.

EDIT: see #48

[oremanj]

I like this API, but there is still the fundamental issue that you can't raise the same exception object twice because it has only one traceback. (That's why unwrap() was one-per-customer in the first place.) This problem doesn't apply to inspecting the exception object after unwrapping, as long as you don't raise it.

Two options I see:

• no peek(), unwrap() clears the attribute, unwrap_preserve() retains it, each Outcome can have at most one call to one of those methods
• keep the current API, document that Error.peek() raises a shallow copy of the exception (same type and args, without the cause/context/traceback - as created by copy.copy()), and then do that

[TeamSpen210]

What I was meaning with the peek() addition is to force a breaking change - remove value and error, that way all code that could be broken immediately fails.

[graingert]

What I was meaning with the peek() addition is to force a breaking change - remove value and error, that way all code that could be broken immediately fails.

I don't want to break the API too much because trio depends on "outcome" with no version range

[graingert]

I like this API, but there is still the fundamental issue that you can't raise the same exception object twice because it has only one traceback. (That's why unwrap() was one-per-customer in the first place.) This problem doesn't apply to inspecting the exception object after unwrapping, as long as you don't raise it.

Two options I see:

* no `peek()`, `unwrap()` clears the attribute, `unwrap_preserve()` retains it, each Outcome can have at most one call to one of those methods

* keep the current API, document that `Error.peek()` raises a shallow copy of the exception (same type and args, without the cause/context/traceback - as created by `copy.copy()`), and then do that

I've gone for option 2, Error.peek() raises a copy.copy(self.error)

[A5rocks]

I don't really get the use case for peek in a world where we aren't using it to rename the properties...

[graingert]

rename the properties?

I'm happy to remove peek but I'd want everyone to agree

[A5rocks]

From what I understood, @TeamSpen210 mentioned them to force existing users to use .peek() s.t. existing users of .value and .error get typecheckers warning about it. (and my reading was that they would return an error value, not raise)

Since compatibility with existing Trio (given we don't have an upper bound) is probably more important, we can't do that -- in which case, having them basically duplicates the properties. Not sure if that's what @CoolCat467 / @oremanj read from earlier comments.

[TeamSpen210]

Yeah that was my thought, they'd act the same as the property, but be a different name so you get instant feedback (type checker or runtime exception).

[graingert]

yeah we can't do this as it would break compatibility with trio which does not have a maximum version constraint on outcome

[TeamSpen210]

If we're not removing the properties, no real reason to have peek - main reason I proposed a method is to communicate that it's now a mutating operation, not something you can do multiple times, but if the properties exist that's just misleading.

[graingert]

We need the properties because they are used by trio

[graingert]

I've made an alternative approach that's fully backwards compatible #49