try_trait_v2 changed the location of the From::from caller so the stack trace is different.

[artemii235]

Initial report #84277 (comment)

We have a problem in our project related to the new question mark desugaring. We use the track_caller feature in From::from implementation of the error types to collect stack traces with generics and auto and negative impl traits magic implemented by @sergeyboyko0791 (https://github.com/KomodoPlatform/atomicDEX-API/blob/mm2.1/mm2src/common/mm_error/mm_error.rs).

After updating to the latest nightly toolchain this stack trace collection started to work differently. I've created a small project for the demo: https://github.com/artemii235/questionmark_track_caller_try_trait_v2

cargo +nightly-2021-05-17 run outputs Location { file: "src/main.rs", line: 18, col: 23 } as we expect.

cargo +nightly-2021-07-18 run outputs Location { file: "/rustc/c7331d65bdbab1187f5a9b8f5b918248678ebdb9/library/core/src/result.rs", line: 1897, col: 27 } - the from_residual implementation that is now used for ? desugaring.

Is there a way to make the track caller work the same way as it was before? Maybe we can use some workaround in our code?

Thanks in advance for any help!

[artemii235]

As per #84277 (comment), applying #[track_caller] to Result::from_residual will bloat a lot of callers. Can this be applied conditionally? If From::from has #[track_caller] then Result::from_residual will have it too, otherwise not.

[thomaseizinger]

So, if I understand you correctly, then what your code is trying to achieve is to create a stacktrace of all invocations of ?.

To my knowledge, this is tracked as "error return traces" here: rust-lang/project-error-handling#9

[nicholastmosher]

I am seeing this same problem using the Location feature from eyre/color-eyre, which appeared after updating to 1.54

[artemii235]

So, if I understand you correctly, then what your code is trying to achieve is to create a stacktrace of all invocations of ?.

Yes, and we actually achieved it in a different way. As I can see the RFC mentioned in rust-lang/project-error-handling#9 is not merged so it has no guarantee to be implemented.

I think this issue can be considered as broken backward compatibility because our use case worked fine before the try_trait_v2 implementation. Hopefully, there is a workaround that won't take too much effort.

[BGR360]

@artemii235 I want to do a very similar thing to your MmError, but directly leveraging the try_trait_v2. For your reference, the workaround that I am experimenting with at my work is to simply write a new Result<T, E> type that wraps std::result::Result<T, E> and implement the FromResidual myself with #[track_caller].

We considered if we could do it by just patching the std library to add #[track_caller] to from_residual, but we want to capture invocations of ? even when the residual is the same type as the output. So we sort of have to do the chaining inside of from_residual rather than inside some impl From.

In your case, it looks like MmError punts on being able to detect ? events when the error type doesn't change, so if your suggestion is accepted, sounds like that will help you.

[nikomatsakis]

Can somebody describe to me what the original code was trying to do? Was it to dynamically reconstruct a stack trace around the ? operator?

[BGR360]

That is correct, to the best of my knowledge.

[sergeyboyko0791]

@nikomatsakis exactly. The main goal is to track sequentially the places where one error E1 converts to another E2.
The location is tracked every time the ? operator calls the from function tagged as track_location:

https://github.com/KomodoPlatform/atomicDEX-API/blob/mm2.1/mm2src/common/mm_error/mm_error.rs#L131

[yaahc]

@artemii235 I want to do a very similar thing to your MmError, but directly leveraging the try_trait_v2. For your reference, the workaround that I am experimenting with at my work is to simply write a new Result<T, E> type that wraps std::result::Result<T, E> and implement the FromResidual myself with #[track_caller].

We considered if we could do it by just patching the std library to add #[track_caller] to from_residual, but we want to capture invocations of ? even when the residual is the same type as the output. So we sort of have to do the chaining inside of from_residual rather than inside some impl From.

In your case, it looks like MmError punts on being able to detect ? events when the error type doesn't change, so if your suggestion is accepted, sounds like that will help you.

I think this is the correct solution going forward that people should reach for when implementing error types that utilize track caller. As Mara mentioned in #88302 (comment) we don't consider track_caller results to be part of our stable API surface, similar to Debug output. That plus @dtolnay indicated that the old track caller support on Result caused significant performance loss in serde compared to manual matches means we likely won't add #[track_caller] back to the new try trait impls for Result and will instead rely on users to implement custom try types when needed as described by @BGR360.

@BGR360 if you plan on publishing a crate for this lmk, otherwise I'll add this to the todo list for the error handling project group.

[BGR360]

@yaahc I'm still pretty new to Rust so I don't really feel qualified to author a general purpose crate for this, nor do I really trust myself to maintain it for the community 😁 . I would be happy to help contribute to the development of a crate, though, and/or to participate in discussion around this topic.

What work would you be adding to the project-error-handling todo list for this? The creation of a new crate? Or some sort of new std library feature?

[yaahc]

What work would you be adding to the project-error-handling todo list for this? The creation of a new crate? Or some sort of new std library feature?

The former, creating a new crate. If you're interested in working on it we'd be happy to help design/review/maintain it. If you want I'd recommend creating a topic for continuing the conversation about it in https://rust-lang.zulipchat.com/#narrow/stream/257204-project-error-handling.

[BGR360]

Topic created: https://rust-lang.zulipchat.com/#narrow/stream/257204-project-error-handling/topic/track_caller.20error.20crate

[artemii235]

I think this is the correct solution going forward that people should reach for when implementing error types that utilize track caller. As Mara mentioned in #88302 (comment) we don't consider track_caller results to be part of our stable API surface, similar to Debug output. That plus @dtolnay indicated that the old track caller support on Result caused significant performance loss in serde compared to manual matches means we likely won't add #[track_caller] back to the new try trait impls for Result and will instead rely on users to implement custom try types when needed as described by @BGR360.

@yaahc Custom result type will work great for a new project, but we already have a lot of code relying on how ? was desugared before. We may need significant refactoring to adapt it for the new result.

Also, we like a lot that we stick with a standard Result. If we extract some of our code to the crate that other people will use they won't need to import the custom result to pattern match over it. Imagine that you depend on crates, each of which has its own result, you will have a lot of the following:

match crate_a_fn {
    CrateAResult::Ok(...) => do something,
    CrateAResult::Err(...) => do something else,
}

match crate_b_fn {
    CrateBResult::Ok(...) => do something,
    CrateBResult::Err(...) => do something else,
}

...

I don't like how it looks 🙂 (my personal taste, though).

With a custom result, we also need a lot of boilerplate: implement all the methods from std (unwrap, map, and others), traits (e.g. TryFuture), etc.

As per serde performance loss - is there a way to conditionally enable #[track_caller] for Result::from_residual? Maybe via feature/compiler flags/Cargo.toml field? It will be turned off by default so we will be able to decide whether we want to sacrifice performance but get traces without custom result types. I had another similar idea previously: #87401 (comment)

[yaahc]

As per serde performance loss - is there a way to conditionally enable #[track_caller] for Result::from_residual? Maybe via feature/compiler flags/Cargo.toml field? It will be turned off by default so we will be able to decide whether we want to sacrifice performance but get traces without custom result types. I had another similar idea previously: #87401 (comment)

We could add a cfg the same way we did for disabling allocation APIs recently but this would require rebuilding std and is nightly only afaik. @BGR360 has been doing some experimenting with specialization in https://rust-lang.zulipchat.com/#narrow/stream/257204-project-error-handling/topic/track_caller.20error.20crate that looks promising, so we may be able to let error types opt in to #[track_caller] information without lowering perf for other crates that don't need or want that information.

The more we've looked into this though the less convinced I am that we should default to not including #[track_caller] information. I'm wondering now if it would be reasonable to have std::result::Result default to using #[track_caller] and then leave crates like serde who need the extra perf to use custom result types. @BGR360's work may make the question moot but I'm curious to know what others think, particularly @dtolnay wrt serde.

[ArtemGr]

Yet another example of carrying a (string) trace with track_caller: ArtemGr/gstuff.rs@cc3c638

[Wolvereness]

This issue bit me hard updating my Rust version. I use a very minimal approach; all I care about is the exact spot in my code that broke normal flow. Errors that I expect get handled normally without ?, and unexpected ones (ones that should basically never occur) I let bubble up. If I need more context, I can go manually implement an error-message for that location. I use this for work to quickly develop applications that don't panic (log error, keep going). My total error-type implementation is less than 50 lines of code that can be trivially copy+pasted into new projects.

So, I would like to make an argument in favor of #[track_caller] for ?.

? is the easy-way-out of a function. The other easy-way-out is panic!. One of these two things lets the developer specify that the final error message includes a location, the other does not. The act of blocking the built-in Result type from the same functionality that panic! provides strongly encourages developers to opt for panic! (read: unwrap and expect) instead. I believe this is a mistake.

The performance implication of certain core features (like checked indexes) should almost always be left to the developer using said features. The performance implication of ? for Result thus should be carefully weighed. So, unless the performance penalty is so great that the general use of ? for Result is discouraged for most users, then I believe the benefit to error handling is worth the inconvenience to those seeking high-performance.

It should not be encouraged of users to completely discard core's Result or ? in favor of a custom implementation just to get a line number where an error occurred. The only reasonable approach I believe exists in this context is instead encouraging a procedural macro to be applied to every function that uses ? for users that want to track an error location, which carries its own pitfalls including the development overhead. Similarly I also dread the prospect of building my own patched Rust, which is absolutely the path of least resistance to my particular use-cases.

Edit:

So, I gave this more thought and figured out an alternative. A trait can be used to still provide an almost ergonomic alternative.

pub trait TrackError<T, E> {
  fn track_error(self) -> Result<T, (E, Location)>;
}

impl<T, E> TrackError<T, E> for Result<T, E> where MyError: From<(E, Location)> {
  #[track_caller]
  fn track_error(self) -> Result<T, (E, Location)> {
    match self {
      Ok(value) => Ok(value),
      Err(error) => Err((error, Location::caller()),
    }
  }
}

Just wrote this off the top of my head, but having it or similar should let a caller do something like:

fn some_failable() -> Result<(), MyError> {
  let _stuff = do_failable()
    .track_error()?;
  Ok(())
}

[yaahc]

I've opened #91752 to attempt to resolve the current discussion.

[Noratrieb]

Can this be closed now?

[jyn514]

I'm going to close this since we added back track_caller in #91752; feel free to re-open if you're still running into issues.