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

error handling #1

Open
colin-kiegel opened this issue Nov 22, 2015 · 7 comments
Open

error handling #1

colin-kiegel opened this issue Nov 22, 2015 · 7 comments
Assignees

Comments

@colin-kiegel
Copy link
Member

It would be great, if we find a common way here. That will make everything else a lot easier!

Ok, here is our starting point :-)

twig-rust

  • return err!(ParserErrorCode::Logic, "Unexpected end of token stream"), where filename:line:column and module-path is collected by the err!-macro automatically. It also supports formatting err!(..., "Hello {name}", name = "world"). This is something I find quite useful.
  • and I use try!( .... ) a lot, where try! will convert different error-types for me, because my error-types implement a couple of Into-traits, e.g. a SyntaxError implements Into, via this macro: impl_convert_exception!(SyntaxErrorCode, LexerErrorCode, LexerErrorCode::SyntaxError);

Definitions are in: src/error/* - but I am sure, it has room for improvement. It definitely needs some cleanup, due to boiler-plate when defining new error definitions.

twig-rs

I am keeping all errors in enums/structs and then converting them to string on actual display. There are 3 groups: template for lexer and parser errors, engine for loading/caching, and runtime which is a bit in flux. On top of that, there is master twig Error which can contain any of these children, and in case of engine/runtime, the chain of error causes. Of course, all chilren are implementing Into trait to be converted to "master" error.

I have found one idea useful: separating location from actual error. There is this At struct which wraps actual error inside. To convert to it, there is method on error named at, which requires location in file. So I do this:

TemplateError::ExpectedTokenTypeButReceived(
    (expected.into(), Received::EndOfStream)
).at(line)

Note that I only have line numbers, but I was planning to expand this to line/col later.

@colin-kiegel
Copy link
Member Author

Reopening (see #12 (comment) + following 2 comments).

Look at alternatives to provide

  • composable structured errors
  • with traces
  • implementing std::error::Error
  • in a rusty way
/// Runtime error with stack trace.
#[derive(Clone, Debug)]
pub struct Traced<T: Clone, Debug> {
    pub error: T,
    /// Trace entries which may have location and/or another error. Maybe another struct.
    pub trace: Vec<(Location, Option<T>)>,
}

@colin-kiegel
Copy link
Member Author

I did it quite similar to your suggestion - but instead of Vec<(Location, Option<T>)> the trace only is Vec<Location>.

We can use the inner error for rust-style error-composition MyError::IO(std::io::Error). Of course we loose information about when exactly such nesting of errors occurs along the backtrace, but I think this is totally acceptable. Presentation will be a lot easier, because we can separate inner error and trace presentation. The first thing the user will see is the inner error.

What do you think?

@Nercury
Copy link
Member

Nercury commented Dec 12, 2015

I am willing to try it :)

@colin-kiegel
Copy link
Member Author

Ok. :-)

Btw: I was not sure about one detail - should Traced<T: Error>
(a) deref to T: Error (current choice)
(b) implement Error by delegating to T: Error
(c) neither one
(d) don't care

I think Deref is usually reserved for smart pointers and Newtype-Tuple-Structs like struct Inches(i32);. On the other hand implementing Error would allow Traced<Traced<T: Error>> - which is not necessarily something bad, but it could happen by accident.

@Nercury
Copy link
Member

Nercury commented Dec 12, 2015

I would do no magic until really needed. So I think (c) until proven otherwise :)

@colin-kiegel
Copy link
Member Author

Ok, I will remove (a).

I had another thought about it. I think what we want is composable errors and giving the caller the flexibility to convert our traced errors into opaque trait objects. If a consumer of Twig wants to convert our Traced<TwigError> into Box<Error> with the following code, he will fail:

fn foo() -> Result<(), Box<Error>> {
    let _ = try!(twig::Engine::fail());
    // ...
}

The try macro will try to call <Box<Error> as From>::<Traced<TwigError>>::from()

  • the caller can't implement this trait
  • we can not implement this trait either (because From and Box are foreign to our crate)
  • But we can either implement Error for Traced<TwigError> or Traced<T: Error>. If we do that, than try will be able to call <Box<Error> as From>::<T: Error>::from() which is implemented by the std-lib.

So actually I think we should do (b).

@Nercury
Copy link
Member

Nercury commented Dec 13, 2015

Ok.

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

No branches or pull requests

2 participants