Improved error reporting

[ilyapuchka]

Added file name, line number and failed token highlighting in error message

Old:

`endfor` was not found.

New:

TemplateName:1:0: error: `endfor` was not found.
{% for name in names %}{{ name }}
^~~~~~~~~~~~~~~~~~~~~~~

@kylef this is an attempt to solve #82 to provide better error messages. I've added some tests to demonstrate new error messages. Let me know if you like this approach and if I should proceed. I plan to use similar approach to filters errors and other things (though they will require some other changes along the way).

[kylef]

This is a great start @ilyapuchka, really happy to see someone tackling this.

I've done a quick review and shared some thoughts, I will try and provide a full review tomorrow.

I'd appreciate breaking down the changes as much as possibly into logic chunks / separate pull requests if possible. It's much easier to review smaller diffs and only have to review smaller changes to the architecture at a time. My original plan was to break it out such as #82 which adds the internal source maps for the tokens, then to add the stack frames to the errors and then add the various reporters.

It looks like this is going towards storing a single token inside the error which won't allow us to properly express the multiple full stack trace (f.e when include and extends is used and multiple files are involved in the error). I'd like to see the structures capable of expressing the full information, of all the frames in an error however in the first MVP version of this it makes sense that only one frame would be present. I think it would also make sense to eventually add a frozen context in the stack frames to make it easier to debug.

What I mean by multiple frames is that the error would contain information about the steps involved getting to the erroneous block for example in the following case:

<h5>Comments</h5>

{% for comment in comments %}
  {% include "comment.html" %}
{% endfor %}

<h6>{{ comment.author }}</h6>

{{ comment }}

{% errblock %}

I'd expect to see an error which contains two frames, as the error was in a location included by the original file. This could possibly also include the context at both locations to help debugging.

[kylef]

Perhaps a better name would be originalContent so its clear without comment.

[kylef]

I'd prefer to keep this logic out of the error class itself. I'd keep TemplateSyntaxError as a POO without additional logic included.

I'd prefer to see the error containing all of the stack frames and contextual information and a separate renderer can be capable of rendering it in different ways.

There will be separate renders for ANSI, plain text, HTML, Bugsnag reporter, etc (eventually).

[kylef]

I'd move this over to the reporter too (#146 (comment)), and internally Stencil would use character(or possibly byte?)-level source maps everywhere.

[kylef]

As an example I just mocked up how a HTML/Bugsnag reporter might look like for the errors. This might help demonstrate the need for multiple frames. In this case it would be possible to integrate the actual Swift frames too into the error leading up to the TemplateSyntaxError.

[ilyapuchka]

@kylef yeah, I think providing more context would be really nice, but I guess it can be done additionally to associating error to specific token that failed to be parsed or node that failed to be rendered. I think that's the main pain point currently - it's hard to see where error comes from. Then we can add up context information to cover more complicated cases like one you described.

Regarding error reporter - I'll try to play with this idea 👍

[ilyapuchka]

Something like this maybe:

protocol ErrorReporter: class { // concrete type to use to be set in Environment
  init(context: Context) // some context type with template name and content just for now
  func report(error: TemplateSyntaxError, for token: Token) throws // we might need the same method for some other structures like filters or nodes, or we could use some common protocol for them
  func contextAwareError(context: Context) -> TemplateSyntaxError?
}

class SimpleErrorReporter: ErrorReporter {

  func report(error: TemplateSyntaxError, for token: Token) throws {
    throw error.contextAwareError(context: context, token: token) ?? error
  }

  func contextAwareError(context: Context) -> TemplateSyntaxError? {
    ...
  }
}

[ilyapuchka]

@kylef I've made more changes and now I think it looks more into direction of what you described in your previous comments.
This still does not cover errors that occur during rendering (only parsing is covered for now), and I'd like your input about stack trace output for implemented console error reporter.
Let me know what you think 😀

[ilyapuchka]

@kylef will be be able to look at this again?

[ilyapuchka]

@kylef I think this is ready now. I've also implemented capturing frames, i.e. for error from extended template reporter will print error like this:

invalid-base.html:2:24: error: Unknown filter 'unknown'
{% block body %}Body {{ target|unknown }} {% endblock %}
                        ^~~~~~~~~~~~~~

invalid-child-super.html:1:0: error: Unknown filter 'unknown'
{% extends "invalid-base.html" %}
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~

[ilyapuchka]

I'm closing this as I accidentally removed my fork of the repo from which I created a pr, so now it does not allow to update it 😓