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.

Sign up for GitHub

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

Jump to bottom

Simplistic support for reporting lexing and parsing errors in template files #47

Merged
merged 4 commits into from
Dec 23, 2020
Merged

Simplistic support for reporting lexing and parsing errors in template files #47

merged 4 commits into from
Dec 23, 2020

Conversation

gasche
Copy link
Collaborator

@gasche gasche commented Dec 23, 2020

This PR is intended to make progress towards fixing #42 : indeed, if we want to use this for human-written templates, we should support the cases where the input is invalid and provide meaningful error messages.

The current PR does not implement meaningful error messages, but at least it documents the possibility of failure and gives the position where the error was detected by the lexer or parser.

It is not completely clear to me how much error-explanation cleverness you want in the main library. Arguably this would rather be the domain of "real applications" using the library down the chain, and the library could limit itself to giving information about the lexing/parsing state on error, to be formatted nicely by library users. This PR uses a different approach where the error type is abstract, and formatting is done by the library; this is more convenient in the short term, but less flexible in the long term.

Note: if we wanted good parsing error messages, we could not just use Menhir's support as-is, because currently most of the work is actually done by the lexer which "bundles" delimiters together with their payloads. So a first step would be to de-bundle the lexer, by instead emitting a token for the delimiter and one or several tokens for the payload, giving more grammar information to Menhir. (We could still keep some context-dependent lexing by having an explicit representation of the lexing state in the lexer: are we in-between delimiters?, etc.) This would be nice, but a much more invasive change than what I had the courage to do in a first PR.

@gasche
mustache_lexer: raise a proper Error instead of Failure
05af228 
An input such as "{{" would previously fail with an error from the
lexing runtime:

> Fatal error: exception Failure("lexing: empty token")

The reason is that the lexer is written in such a way that it forces
the lexing of input in places where no input may be available.

A long-term fix would be to write the lexer in a different, more
standard style, where we don't try to "bundle" different information
in a single tokan.

The current fix is simply to make all tokens nullable by raising an
explicit Error exception when reaching end-of-file.
@gasche gasche mentioned this pull request Dec 23, 2020
Merged
@rgrinberg
Copy link
Owner

It is not completely clear to me how much error-explanation cleverness you want in the main library.

Probably as much as possible. The consumers of such a templating library are likely using it to template html or simple configuration files. It would be great to give them a good experience out of the box. If this isn't flexible enough for some consumers, we can always add a more complex api on top.

if we wanted good parsing error messages, we could not just use Menhir's support as-is, because currently most of the work is actually done by the lexer which "bundles" delimiters together with their payloads. So a first step would be to de-bundle the lexer, by instead emitting a token for the delimiter and one or several tokens for the payload, giving more grammar information to Menhir

Sounds good to me.

rgrinberg
rgrinberg reviewed Dec 23, 2020
View reviewed changes
lib/mustache.ml
}
and error_kind = Parsing | Lexing of string

exception Parse_error of template_parse_error
Copy link
Owner

Choose a reason for hiding this comment

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

Should we register a printer for this exception?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, I added exception-registration logic in a new commit.

@rgrinberg rgrinberg merged commit 2a04640 into rgrinberg:master Dec 23, 2020
@gasche
Copy link
Collaborator Author

gasche commented Dec 25, 2020

Now that I have slightly more experience with the ocaml-mustache codebase, I see several things with this PR that are not quite right, and I think should be fixed.

  • the Invalid_template error is also a parse-time error and should be handled as such.
  • The pp_error name is too generic, as we probably also want better exception payloads for render-time ("runtime") errors, which would need a separate error type.

I hope to fix them shortly so that users don't see the intermediary state. I am planning to propose other improvements to error information and error reporting to the library, but those would change existing exceptions so they would break some compatibility -- unlike this PR.

@gasche gasche mentioned this pull request Dec 27, 2020
Closed
@psafont psafont mentioned this pull request Nov 24, 2023
Merged
psafont added a commit to psafont/opam-repository that referenced this pull request Nov 24, 2023
@psafont
[new release] mustache (3.2.0)
c00f813 
CHANGES:

* Remove the AST without locations: now all functions build an AST with locations;
  in particular, parsing always provide located error messages.
  To ease backward-compatibility, the smart constructors still use the
  same interface, using dummy locations by default, with
  a With_locations module for users who wish to explicitly provide
  locations.
  (@gasche, rgrinberg/ocaml-mustache#65)
* Support for "template inheritance" (partials with parameters)
  `{{<foo}} {{$param1}}...{{/param1}} {{$param2}}...{{/param2}} {{/foo}`
  following the widely-implemented semi-official specification
    mustache/spec#75
  (@gasche, 58)
* Partials are now supported in the `mustache` command-line tool (@gasche, rgrinberg/ocaml-mustache#57)
  They are interpreted as template inclusion: "{{>foo/bar}}" will include
  "foo/bar.mustache", relative to the current working directory.
* Improve error messages (@gasche, rgrinberg/ocaml-mustache#47, rgrinberg/ocaml-mustache#51, rgrinberg/ocaml-mustache#56)
  Note: the exceptions raised by Mustache have changed, this breaks
  compatibility for users that would catch and deconstruct existing
  exceptions.
* Add `render_buf` to render templates directly to buffers (@gasche, rgrinberg/ocaml-mustache#48)
* When a lookup fails in the current context, lookup in parents contexts.
  This should fix errors when using "{{#foo}}" for a scalar variable
  'foo' to check that the variable exists.
  (@gasche, rgrinberg/ocaml-mustache#49)
psafont added a commit to psafont/opam-repository that referenced this pull request Nov 24, 2023
@psafont
[new release] mustache (3.2.0)
3322429 
CHANGES:

* Remove the AST without locations: now all functions build an AST with locations;
  in particular, parsing always provide located error messages.
  To ease backward-compatibility, the smart constructors still use the
  same interface, using dummy locations by default, with
  a With_locations module for users who wish to explicitly provide
  locations.
  (@gasche, rgrinberg/ocaml-mustache#65)
* Support for "template inheritance" (partials with parameters)
  `{{<foo}} {{$param1}}...{{/param1}} {{$param2}}...{{/param2}} {{/foo}`
  following the widely-implemented semi-official specification
    mustache/spec#75
  (@gasche, 58)
* Partials are now supported in the `mustache` command-line tool (@gasche, rgrinberg/ocaml-mustache#57)
  They are interpreted as template inclusion: "{{>foo/bar}}" will include
  "foo/bar.mustache", relative to the current working directory.
* Improve error messages (@gasche, rgrinberg/ocaml-mustache#47, rgrinberg/ocaml-mustache#51, rgrinberg/ocaml-mustache#56)
  Note: the exceptions raised by Mustache have changed, this breaks
  compatibility for users that would catch and deconstruct existing
  exceptions.
* Add `render_buf` to render templates directly to buffers (@gasche, rgrinberg/ocaml-mustache#48)
* When a lookup fails in the current context, lookup in parents contexts.
  This should fix errors when using "{{#foo}}" for a scalar variable
  'foo' to check that the variable exists.
  (@gasche, rgrinberg/ocaml-mustache#49)
psafont added a commit to psafont/opam-repository that referenced this pull request Nov 27, 2023
@psafont
[new release] mustache (3.2.0)
5c0c035 
CHANGES:

* Remove the AST without locations: now all functions build an AST with locations;
  in particular, parsing always provide located error messages.
  To ease backward-compatibility, the smart constructors still use the
  same interface, using dummy locations by default, with
  a With_locations module for users who wish to explicitly provide
  locations.
  (@gasche, rgrinberg/ocaml-mustache#65)
* Support for "template inheritance" (partials with parameters)
  `{{<foo}} {{$param1}}...{{/param1}} {{$param2}}...{{/param2}} {{/foo}`
  following the widely-implemented semi-official specification
    mustache/spec#75
  (@gasche, 58)
* Partials are now supported in the `mustache` command-line tool (@gasche, rgrinberg/ocaml-mustache#57)
  They are interpreted as template inclusion: "{{>foo/bar}}" will include
  "foo/bar.mustache", relative to the current working directory.
* Improve error messages (@gasche, rgrinberg/ocaml-mustache#47, rgrinberg/ocaml-mustache#51, rgrinberg/ocaml-mustache#56)
  Note: the exceptions raised by Mustache have changed, this breaks
  compatibility for users that would catch and deconstruct existing
  exceptions.
* Add `render_buf` to render templates directly to buffers (@gasche, rgrinberg/ocaml-mustache#48)
* When a lookup fails in the current context, lookup in parents contexts.
  This should fix errors when using "{{#foo}}" for a scalar variable
  'foo' to check that the variable exists.
  (@gasche, rgrinberg/ocaml-mustache#49)
psafont added a commit to psafont/opam-repository that referenced this pull request Nov 28, 2023
@psafont
[new release] mustache (3.2.0)
480c69d 
CHANGES:

* Remove the AST without locations: now all functions build an AST with locations;
  in particular, parsing always provide located error messages.
  To ease backward-compatibility, the smart constructors still use the
  same interface, using dummy locations by default, with
  a With_locations module for users who wish to explicitly provide
  locations.
  (@gasche, rgrinberg/ocaml-mustache#65)
* Support for "template inheritance" (partials with parameters)
  `{{<foo}} {{$param1}}...{{/param1}} {{$param2}}...{{/param2}} {{/foo}`
  following the widely-implemented semi-official specification
    mustache/spec#75
  (@gasche, 58)
* Partials are now supported in the `mustache` command-line tool (@gasche, rgrinberg/ocaml-mustache#57)
  They are interpreted as template inclusion: "{{>foo/bar}}" will include
  "foo/bar.mustache", relative to the current working directory.
* Improve error messages (@gasche, rgrinberg/ocaml-mustache#47, rgrinberg/ocaml-mustache#51, rgrinberg/ocaml-mustache#56)
  Note: the exceptions raised by Mustache have changed, this breaks
  compatibility for users that would catch and deconstruct existing
  exceptions.
* Add `render_buf` to render templates directly to buffers (@gasche, rgrinberg/ocaml-mustache#48)
* When a lookup fails in the current context, lookup in parents contexts.
  This should fix errors when using "{{#foo}}" for a scalar variable
  'foo' to check that the variable exists.
  (@gasche, rgrinberg/ocaml-mustache#49)
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
@psafont @nberth
[new release] mustache (3.2.0)
9ecb162 
CHANGES:

* Remove the AST without locations: now all functions build an AST with locations;
  in particular, parsing always provide located error messages.
  To ease backward-compatibility, the smart constructors still use the
  same interface, using dummy locations by default, with
  a With_locations module for users who wish to explicitly provide
  locations.
  (@gasche, rgrinberg/ocaml-mustache#65)
* Support for "template inheritance" (partials with parameters)
  `{{<foo}} {{$param1}}...{{/param1}} {{$param2}}...{{/param2}} {{/foo}`
  following the widely-implemented semi-official specification
    mustache/spec#75
  (@gasche, 58)
* Partials are now supported in the `mustache` command-line tool (@gasche, rgrinberg/ocaml-mustache#57)
  They are interpreted as template inclusion: "{{>foo/bar}}" will include
  "foo/bar.mustache", relative to the current working directory.
* Improve error messages (@gasche, rgrinberg/ocaml-mustache#47, rgrinberg/ocaml-mustache#51, rgrinberg/ocaml-mustache#56)
  Note: the exceptions raised by Mustache have changed, this breaks
  compatibility for users that would catch and deconstruct existing
  exceptions.
* Add `render_buf` to render templates directly to buffers (@gasche, rgrinberg/ocaml-mustache#48)
* When a lookup fails in the current context, lookup in parents contexts.
  This should fix errors when using "{{#foo}}" for a scalar variable
  'foo' to check that the variable exists.
  (@gasche, rgrinberg/ocaml-mustache#49)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgrinberg rgrinberg rgrinberg left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gasche @rgrinberg