Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow fail messages to be caught, introduce Any trait #9967

Merged
merged 1 commit into from
Oct 28, 2013

Conversation

Kimundi
Copy link
Member

@Kimundi Kimundi commented Oct 19, 2013

Summary

This PR allows the cause of a failure to be received in a task's future result and as the Err case of task::try, and also implements dynamic typing in form of an Any trait.

Task failure

  • fail! and related macros now accept 5 kinds of types: ~str, &'static str, std::send_str::SendStr, ~std::any::Any and ~T where T: Any + Send + 'static
  • std::task::TaskResult got transformed into an internal enum std::rt::task::UnwindResult, and it's Failure variant now contains a value of enum type UnwindReason:
    • UnwindReasonStr(SendStr) maps to failing with a value of type ~str, &'static str or SendStr.
    • UnwindReasonAny(~Any) maps to failing with an ~Any or ~T with T: Send + 'static.
    • UnwindReasonLinked maps to failing because the task got killed by linked failure.
  • Instead, std::task::TaskResult is now a typedef for Result<(), ~Any>, and both TaskBuilder's future_result() and task::try now work with a value of this type.
  • future_result() no longer returns a Port<TaskResult>, instead it returns a wrapper TaskResultPort that implements GenericPort and Peekable, and which lazily allocates a ~ box and casts to ~Any in case of failure with a SendStr or linked failure (for the latter case a unit struct LinkedFailure got added.)
  • Because fail! collapses both ~str and &'static str into a SendStr, checking if a task error value is a string will just require a .is::<SendStr>() check, with ~str and &'static str only being possible in case of an explicit fail!(~~"...") or fail!(~ (&"...") as ~Any):

Any

  • In order to allow failing with arbitrary data, the Any trait got implemented.
  • It is being used in form of a trait object, usually ~Any or &Any.
  • &Any, ~Any and &mut Any have a few extension methods implemented on them:
    • is<T>(self) -> bool returns true if the Any object contains type T
    • as_ref<T>(self) -> Option<&T> returns a reference to the contained type, if it is T
    • as_mut<T>(self) -> Option<&mut T> returns a mutable reference to the contained type, if it is T
    • move<T>(self) -> Option<~T> allows to get the ~T out of an ~Any again.
  • Any currently uses type descriptors as a type id for comparisons, which is
    • not reliable, as it is not guaranteed that every type has only one type descriptor.
    • But safe, as no two types share the same type descriptor.
  • The implementation also a few transmutes, mostly to cast a *Void of the wrapped type into it's actual type &T, &mut T or ~T.

Other changes

  • std::unstable::UnsafeArc::try_unwrap no longer uses Either, bringing us one step closer to removing that type.
  • A few of the touched modules got their import lines sorted.
  • A few stylistic cleanups here and there.

@Kimundi
Copy link
Member Author

Kimundi commented Oct 19, 2013

cc @brson and @catamorphism

FailCauseAny(~Any),

/// Failed because of linked failure
FailCauseLinked
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can linked failure bubble up the failure cause? Presumably this is well-defined, since every task has exactly one parent, so ownership of the FailCause can be transferred to it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way it is currently implemented, it's actually more like N tasks can cause M tasks to fail due to linked failure. I attempted this at first, but it lead me down a rabbit hole of having a linked list of atomically reference counted fail causes, just so that every tasks that listens to task failure can receive them. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, can you link two arbitrary tasks for linked failure? (Not just parent/child tasks?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the hierarchy is still a tree, but a failure notifies all ancestors, which in combination with having multiple children that might all have failed leads to a directed graph of failure propagations. :)

At the moment this is not a problem because that part of the task unwinding system uses a boolean success flag, which means all the various combinations just lead to a few ANDs and ORs, but a lot of refactoring would be necessary if you wanted to use a sendable type there.

@brson
Copy link
Contributor

brson commented Oct 21, 2013

We should probably discuss ~Any in the weekly meeting before merging this.

@brson
Copy link
Contributor

brson commented Oct 22, 2013

Looks good! I think that I too would like tasks to just always fail with an ~Any. Yes, it means that all failure requires an allocation, and that might be especially tricky for OOM failures, but exceptions are exceptional, so this doesn't strike me as something that needs to be optimal, and the simplicity of just catching ~Any is attractive.

In the future we will want to consider whether this should actually be a special dynamic ~Error type with some standard error-related methods.

@huonw
Copy link
Member

huonw commented Oct 23, 2013

(Needs a rebase.)

@Kimundi
Copy link
Member Author

Kimundi commented Oct 23, 2013

So, how should I proceed?

In case of changing this PR to only use ~Any, which is motivated to make the try interface simpler, I see a few possibilities on how to do that:

On the future_result() and try side:

  1. Only change future_result (and try) to expose a ~Any instead of a FailCause in case of an error, keeping the three possible failure types LinkedFailure, SendStr and ~Any for internal failure propagation.
    • This would mean only future_result() needs to allocate the ~Any box in case of one of the string types or struct LinkedFailure;
    • The user would also be able to propagate or manually allocate an ~Any for failure.
    • User handling of task failure would only deal with ~Any
    • The OOM case could possibly be solved by this, as the allocation for LinkedFailure and SendStr could happen in the task that received the failure, rather than the one that initiated it.
  2. Remove the FailCause enum completely
    • This would mean that the runtime needs to allocate a lot of struct LinkedFailure; ~LinkedFailure as ~Any during unwinding.
    • Same for the FailCauseStr enum variant, it would need to allocate during the fail!() call.
    • The user would still just need to deal with an ~Any, but the runtime would allocate more during unwinding.

On the fail!() side:

  1. Keep the number of possible failure types restricted to &'static str, ~str, SendStr and ~Any.
    • This would allow for fail!("foo"), fail!(~"foo"), but needs fail!(~Foo as ~Any) for the ~Any case (Possibly only fail!(~Foo) in the future, if trait object coercion gets implemented).
    • This would mean user code needs to be explicit if it wants to fail with ~Any, unless trait object coercion gets implemented.
  2. Allow failing with any type passed to fail!() directly. fail!("foo"), fail!(~"foo") and fail!(Foo) would all work, but all allocate a new ~ box and cast it to a ~Any
    • This would mean that failure always involves a hidden allocation at the initiation of the failure, rather than at the evaluation of the result, which would be more problematic and opaque.
    • This would also mean users could not fail with or propagate a provided ~Any, as every fail!() would wrap it in a new layer of ~Any
    • This would also also pose problems for some kinds of generic code I'm working on which tries to expose an interface that allows the failure message to either be provided eagerly, or computed lazy, which involves an implementation of a trait on both a specific T, and a fn() -> T. Fail allowing any type would mean this would conflict with each other. (The Api is intended for Option::expect to allow both opt.expect("message") and opt.expect(|| expensive_computed_message()) that should be called only in case of an actual None value.)

Personally, I think if the user only needing to worry about ~Any is desirably, then I would choose the first variant on both sides:

future_result() and try only exposes a ~Any, but internally there are still fast paths for the (currently) usual case of failing with a string and because of linked failure.

User code that wants to fail also needs to explicitly allocate the ~ box if it wants to fail with an ~Any, which would be good for being explicit with the allocation, and which allows easy propagation of failure values (just fail! again with the received value).

@pnkfelix
Copy link
Member

cc me

@Kimundi
Copy link
Member Author

Kimundi commented Oct 23, 2013

After looking at it more deeply, for the case where the ~Any gets allocated on the future_result/try side there are still two options:

  1. Only allocate the ~Any in try
    • pro: Allocation happens in the parent task
    • con: future_result still exposes the FailCause enum
    • However, that might be fine as future_result would simply become more low level, it already exposes a different enum than try today.
    • And there can still be a to_any(self) -> Result<(), ~Any> method on TaskResult to do that allocating conversion anyway.
  2. Allocate the ~Any in the on_exit handler of spawn_raw
    • pro: both future_result and try would give a ~Any in the case of an error
    • con: Context in which the allocation happens seems unclear, but I think it's in the context of the failing task or the scheduler, which would both be bad in case of OOM

@brson
Copy link
Contributor

brson commented Oct 24, 2013

fail! needs to continue to be convenient to use - having to cast to ~Any would not be good - so whatever it takes there is fine.

I'd prefer not to expose FailureResult to the public interface, so I guess my preference is to allocate it in spawn_raw.

The issue of what happens on OOM is academic at this point since what actually happens is the runtime aborts. Implementation details can be finessed when the time comes.

@Kimundi
Copy link
Member Author

Kimundi commented Oct 24, 2013

I found an easier solution for returning the ~Any in future_result and try:

Instead of allocating the ~ box early, and during unwinding, it now only gets allocated in future_results recv method, thus still allows for a non-allocating unwinding in the future, as well as for possible optimizations.

For this, I implemented a TaskResultPort that implements the comm::GenericPort trait and wraps the internal non-allocating fail message propagation type as a Result<(), ~Any>.

@Kimundi
Copy link
Member Author

Kimundi commented Oct 25, 2013

I updated this PR to return a ~Any in case of task failure. Following is an updated description of the content of this PR:

Summary

This PR allows the cause of a failure to be received in a task's future result and as the Err case of task::try, and also implements dynamic typing in form of an Any trait.

Task failure

  • fail! and related macros now accept 5 kinds of types: ~str, &'static str, std::send_str::SendStr, ~std::any::Any and ~T where T: Send + 'static
  • std::task::TaskResult got transformed into an internal enum std::rt::task::UnwindResult, and it's Failure variant now contains a value of enum type UnwindReason:
    • UnwindReasonStr(SendStr) maps to failing with a value of type ~str, &'static str or SendStr.
    • UnwindReasonAny(~Any) maps to failing with an ~Any or ~T with T: Send + 'static.
    • UnwindReasonLinked maps to failing because the task got killed by linked failure.
  • Instead, std::task::TaskResult is now a typedef for Result<(), ~Any>, and both TaskBuilder's future_result() and task::try now work with a value of this type.
  • future_result() no longer returns a Port<TaskResult>, instead it returns a wrapper TaskResultPort that implements GenericPort and Peekable, and which lazily allocates a ~ box and casts to ~Any in case of failure with a SendStr or linked failure (for the latter case a unit struct LinkedFailure got added.)
  • Because fail! collapses both ~str and &'static str into a SendStr, checking if a task error value is a string will just require a .is::<SendStr>() check, with ~str and &'static str only being possible in case of an explicit fail!(~~"...") or fail!(~ (&"...") as ~Any):

Any

  • In order to allow failing with arbitrary data, the Any trait got implemented.
  • It is being used in form of a trait object, usually ~Any or &Any.
  • &Any, ~Any and &mut Any have a few extension methods implemented on them:
    • is<T>(self) -> bool returns true if the Any object contains type T
    • as_ref<T>(self) -> Option<&T> returns a reference to the contained type, if it is T
    • as_mut<T>(self) -> Option<&mut T> returns a mutable reference to the contained type, if it is T
    • move<T>(self) -> Option<~T> allows to get the ~T out of an ~Any again.
  • Any currently uses type descriptors as a type id for comparisons, which is
    • not reliable, as it is not guaranteed that every type has only one type descriptor.
    • But safe, as no two types share the same type descriptor.
  • The implementation also a few transmutes, mostly to cast a *Void of the wrapped type into it's actual type &T, &mut T or ~T.

Other changes

  • std::unstable::UnsafeArc::try_unwrap no longer uses Either, bringing us one step closer to removing that type.
  • A few of the touched modules got their import lines sorted.
  • A few stylistic cleanups here and there.

@brson
Copy link
Contributor

brson commented Oct 25, 2013

I would rather future_result just return a Port and not it's own type, but otherwise I'm happy with this so r+.

@Kimundi
Copy link
Member Author

Kimundi commented Oct 25, 2013

@brson: Hm, good catch about the ~T one.
Any is implemented for all T, so that's probably why the compiler didn't complain?

About TaskResultPort: It seems to me that this situation is exactly what the GenericPort trait is for, no? I'm drawing parallels to Iterator here: Port and Chan adapters analogue to Iterator adapters might even be useful in the general case.

@Kimundi
Copy link
Member Author

Kimundi commented Oct 25, 2013

After the last rebase, make check now errors for me with

run codegen [x86_64-unknown-linux-gnu]: x86_64-unknown-linux-gnu/stage2/bin/compiletest

running 9 tests
test [codegen] codegen/iterate-over-array.rs                ... metric: clang-codegen-ratio: 0.620253 (+/- 0.001)
test [codegen] codegen/scalar-function-call.rs              ... metric: clang-codegen-ratio: 0.8125 (+/- 0.001)
test [codegen] codegen/single-return-value.rs               ... metric: clang-codegen-ratio: 1.3 (+/- 0.001)
test [codegen] codegen/small-dense-int-switch.rs            ... metric: clang-codegen-ratio: 0.711111 (+/- 0.001)
test [codegen] codegen/stack-alloc-string-slice.rs          ... metric: clang-codegen-ratio: 0.619048 (+/- 0.001)
test [codegen] codegen/static-method-call-multi.rs          ... metric: clang-codegen-ratio: 0.805556 (+/- 0.001)
test [codegen] codegen/static-method-call.rs                ... metric: clang-codegen-ratio: 0.8 (+/- 0.001)
test [codegen] codegen/virtual-method-call-struct-return.rs ... metric: clang-codegen-ratio: 0.83871 (+/- 0.001)
test [codegen] codegen/virtual-method-call.rs               ... metric: clang-codegen-ratio: 1.1 (+/- 0.001)

using metrics ratcher: tmp/check-stage2-T-x86_64-unknown-linux-gnu-H-x86_64-unknown-linux-gnu-codegen-metrics.json
[codegen] codegen/iterate-over-array.rs.clang-codegen-ratio: regressed by 4.26%
result of ratchet: 0 matrics added, 0 removed, 0 improved, 1 regressed, 8 noise
left ratchet file untouched

test result: FAILED. 0 passed; 0 failed; 0 ignored; 9 measured

I'm not sure how to deal with that/what caused it.
I guess it might be caused by me changing task::begin_unwind?

Some code cleanup, sorting of import blocks

Removed std::unstable::UnsafeArc's use of Either

Added run-fail tests for the new FailWithCause impls

Changed future_result and try to return Result<(), ~Any>.

- Internally, there is an enum of possible fail messages passend around.
- In case of linked failure or a string message, the ~Any gets
  lazyly allocated in future_results recv method.
- For that, future result now returns a wrapper around a Port.
- Moved and renamed task::TaskResult into rt::task::UnwindResult
  and made it an internal enum.
- Introduced a replacement typedef `type TaskResult = Result<(), ~Any>`.
bors added a commit that referenced this pull request Oct 28, 2013
# Summary

This PR allows the cause of a failure to be received in a task's future result and as the `Err` case of `task::try`, and also implements dynamic typing in form of an `Any` trait.

# Task failure

- `fail!` and related macros now accept 5 kinds of types: `~str`, `&'static str`, `std::send_str::SendStr`, `~std::any::Any` and `~T` where `T: Any + Send + 'static`
- `std::task::TaskResult` got transformed into an internal enum `std::rt::task::UnwindResult`,  and it's `Failure` variant now contains a value of enum type `UnwindReason`:
  - `UnwindReasonStr(SendStr)` maps to failing with a value of type `~str`, `&'static str` or `SendStr`.
  - `UnwindReasonAny(~Any)` maps to failing with an `~Any` or `~T` with `T: Send + 'static`.
  - `UnwindReasonLinked` maps to failing because the task got killed by linked failure.
- Instead, `std::task::TaskResult` is now a typedef for `Result<(), ~Any>`, and both `TaskBuilder`'s `future_result()` and `task::try` now work with a value of this type.
- `future_result()` no longer returns a `Port<TaskResult>`, instead it returns a wrapper `TaskResultPort` that implements `GenericPort` and `Peekable`, and which lazily allocates a `~` box and casts to `~Any` in case of failure with a `SendStr` or linked failure (for the latter case a unit struct `LinkedFailure` got added.)
- Because `fail!` collapses both `~str` and `&'static str` into a `SendStr`, checking if a task error value is a string will just require a `.is::<SendStr>()` check, with `~str` and `&'static str` only being possible in case of an explicit `fail!(~~"...")` or `fail!(~ (&"...") as ~Any)`:

# Any

- In order to allow failing with arbitrary data, the `Any` trait got implemented.
- It is being used in form of a trait object, usually `~Any` or `&Any`.
- `&Any`, `~Any` and `&mut Any` have a few extension methods implemented on them:
  - `is<T>(self) -> bool` returns true if the `Any` object contains type `T`
  - `as_ref<T>(self) -> Option<&T>` returns a reference to the contained type, if it is `T`
  - `as_mut<T>(self) -> Option<&mut T>` returns a mutable reference to the contained type, if it is `T`
  - `move<T>(self) -> Option<~T>` allows to get the `~T` out of an `~Any` again.
- `Any` currently uses type descriptors as a type id for comparisons, which is 
    - not reliable, as it is not guaranteed that every type has only one type descriptor.
    - But safe, as no two types share the same type descriptor.
- The implementation also a few `transmute`s, mostly to cast a `*Void` of the wrapped type into it's actual type `&T`, `&mut T` or `~T`.

# Other changes

- `std::unstable::UnsafeArc::try_unwrap` no longer uses `Either`, bringing us one step closer to removing that type.
- A few of the touched modules got their import lines sorted.
- A few stylistic cleanups here and there.
@bors bors closed this Oct 28, 2013
@bors bors merged commit fa8e71a into rust-lang:master Oct 28, 2013
@bluss
Copy link
Member

bluss commented Oct 28, 2013

I'm sorry if this has already been brought up, but could Any be removed from the prelude? Having it there gives every rust value three new methods by default, and it feels kind of "dirty", letting it be opt-in would be an improvement I think.

@Kimundi
Copy link
Member Author

Kimundi commented Oct 28, 2013

@blake2-ppc Hm, good point.

@bblum
Copy link
Contributor

bblum commented Nov 10, 2013

why are we trying to remove the Either type? the point of it is to allow users to avoid open-coding short sum types when they don't care about naming the arms - are we trying to discourage this practice? removing it incurs a bunch of boilerplate, as exhibited here.

@catamorphism
Copy link
Contributor

@bblum We've discussed the Either type in a team meeting before. The feeling among the core team is, as far as I can tell, that (like boolean return values) Either is a bad code smell. Some folks even suggested removing the type from the standard library. I'm not sure I'd go that far, but I agree that it's confusing to use Either when it would be almost as easy to write a one-off data type that provides better documentation.

I don't expect this decision to be revisited.

flip1995 pushed a commit to flip1995/rust that referenced this pull request Dec 1, 2022
Remove blank lines when needless_return returns no value

fix rust-lang/rust-clippy#9416

changelog: [`needless_return`] improve result format

r? `@llogiq`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants