Skip to content

Conversation

colin-kiegel
Copy link
Member

I changed my error handling to adopt some elements I liked about your implementation. :-)

best read in reverse:

  1. src/test_error/mod.rs <-- this is what it will feel like to use it
  2. src/test_error/error_codes.rs <-- this is how to define errors
  3. src/error/* <-- this is how it is implemented

What do you think?
Still need to clean up some comments - but otherwise functional!

@colin-kiegel
Copy link
Member Author

I will rework some details, because I am not yet happy with how I integrated the std::error::Error trait.

@colin-kiegel
Copy link
Member Author

I committed a new version - I think it's much better now.

The main players are

  • pub trait ErrorCode
  • pub struct Error<T: ErrorCode> {...}
  • macros: err!(), location!(), try_chain!()

ErrorCodes are like std::error::Error without error-chaining
Error<T: ErrorCode> are like std::error::Error with additional location support - in fact they implement std::error::Error, including error-chaining

This way we can separate it basically like this:

  • Everything error-specific is in enums implementing ErrorCode
  • Everything generic, like chaining and locations, is in Error<T: ErrorCode>

Every x: ErrorCode can be transformed into Error<ErrorCode> simply by x.at(location!())

By implementing std::error::Error we can improve cross-crate compatibility of our error handling system. This is from the std-library:

/// Base functionality for all errors in Rust.
#[stable(feature = "rust1", since = "1.0.0")]
pub trait Error: Debug + Display + Reflect {
    /// A short description of the error.
    ///
    /// The description should not contain newlines or sentence-ending
    /// punctuation, to facilitate embedding in larger user-facing
    /// strings.
    #[stable(feature = "rust1", since = "1.0.0")]
    fn description(&self) -> &str;

    /// The lower-level cause of this error, if any.
    #[stable(feature = "rust1", since = "1.0.0")]
    fn cause(&self) -> Option<&Error> { None }

Copy link
Member

Choose a reason for hiding this comment

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

Of course! 👍

@Nercury
Copy link
Member

Nercury commented Nov 27, 2015

I am still reading it and trying to understand how it works, which may not be a good sign :)
I think I need to see it in editor to click, will do that soon. Please bear with me :)

@colin-kiegel
Copy link
Member Author

Sure, take your time. Feel free to ask, if you have any questions. :-)

Copy link
Member

Choose a reason for hiding this comment

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

I suppose this is just an example, but if the call happened from twig template, I would like to see the location in template.

So there has to be another layer of .at which becomes confusing.

Copy link
Member

Choose a reason for hiding this comment

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

I would like to move these kind of problems to the template user, like:

Some runtime error at src/engine/mod.rs:38:12
        - caused by: Error running a template index.twig.html at src/core/mod.rs:20:4
        - caused by: Target requires 10 arguments, called with 4 at index.twig.html:11:2
        - caused by: Received invalid argument count at src/extension/core.rs:22:22

The second caused by is strictly more useful for template user, because that's the location where error most likely should be fixed.

I don't know how to do this, just writing my thoughts :)

Copy link
Member

Choose a reason for hiding this comment

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

Also - I don't know if you are aware of this - but parsing can happen at runtime, unless we exclude this one feature where included template depends on expression :)

Copy link
Member Author

Choose a reason for hiding this comment

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

If the Error happened in a template, I would suggest something like that

    err!(SomeErrorCode::Foo{
        ...,
        cursor: Cursor // <-- information about current lexer position and template name
    })

This error would print like

Target requires 10 arguments, called with 4 for `index.twig.html` line 11 column 2 at src/test_error/mod.rs:20:4\n

You are right, that these error-chains might obscure the most valuable part from a users perspective. I have to admit, that I was designing this part from a developer perspective. This is why I wanted these pseudo-backtraces.

Maybe we can find a way to have two or even three modes to print errors. Currently there are already different stringify-methods for errors: Display::fmt + Debug::fmt + ErrorCode::description. Where Debug might give even more information as my examples in some cases, because we can put really complex objects into these ErrorCodes. This might be useful. Display would also be useful in its current form. ErrorCode::description is only for compatibility with std::error::Error - and I am not yet 100% happy with it. Sadly the std Error-trait has a very restricting signature description(&self) -> &str, which makes it hard to add dynamic data without internal buffer strings. But it seems like this is how it was meant to be by the std Error-Trait - to use Display+Debug instead.

I would like to reserve the Debug::fmt of errors for local development - while Display::fmt should be something useful for production.

When we get bug reports I would like them to be as detailed as in the examples - with those pseudo-backtraces. But you are right - if the bug is in the template and not in the engine - it might be too much. So maybe we need some kind of verbosity-flag for displaying errors? I'm not sure, yet.

@colin-kiegel
Copy link
Member Author

I updated the branch according to your comments and my replies.

Most notably I added an example for an error with reference to a template position, which would currently look like this:

Target requires 10 arguments, called with 4 for `index.twig.html` line 11 column 2 at src/test_error/mod.rs:20:4\n

src/error/mod.rs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Maybe call ErrorCode -> TwigError? I understand that we are running of Error names here a bit :)

@Nercury
Copy link
Member

Nercury commented Nov 28, 2015

I think it looks fine, feel free to merge.
I will also be a bit occupied next week, but I will try to finish up the Lexer skeleton.
If anything, my email is visible in my github profile :)

@colin-kiegel
Copy link
Member Author

Ok, cool.

Today I ported my twig code to the new error handling - took me the whole day - but now it's done. More about it tomorrow. :-)

This is the commit: colin-kiegel/twig-rust@4a732d8

colin-kiegel added a commit that referenced this pull request Nov 29, 2015
@colin-kiegel colin-kiegel merged commit 4b3c75e into rust-web:master Nov 29, 2015
@colin-kiegel colin-kiegel deleted the pr_error_handling branch November 29, 2015 13:42
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.

2 participants