802845 - Compiler should emit Exceptions on errors for easier debugging

Status: RESOLVED FIXED

(L20n :: JS Library, defect, P2)

Description

[Staś Małolepszy :stas]

If an entity or a macro don't compile or don't toString correctly, the compiler should emit an error for easier debugging and continue gracefully.

Comment 1

[Staś Małolepszy :stas]

Attachment: Patch

Comment 2

[Staś Małolepszy :stas]

Attachment: Patch

Comment 3

[Staś Małolepszy :stas]

Attached wrong files, sorry.

Comment 4

[Staś Małolepszy :stas]

Attachment: WIP patch

Here's a WIP patch.

I think we need some other kind of exception to emit at the bottom of the patch. Right now, I emit COMPILE_ERROR for both the fatal and non-fatal case.

Then, there's a question of what to actually do on a fatal error:

- invalidate current ctx, and create the subctx
- don't invalidate the current one, but still create the subctx as it will likely come in handy
- do nothing (let the subctx be created synchronously on first broken get() call
- doesn't really matter, because fatal errors are mostly theoretical:  you won't see them unless you either explicitly state you want them (with recover=false) or unless you don't use an emitter.

Not sure if we should tackle this in this bug, or just land this as-is and then work in bug 801663.

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: tests

Comment 7

[Staś Małolepszy :stas]

Bug 803399 (which I duped) was blocking-parkcity+; carrying the + over to here.

Comment 8

[Axel Hecht [:Pike]]

I was wondering, what's the benefit of having errors be signaled outside of the AST?

Like, for editor writers, we have to fold them back in, and I wonder why customers of the library that ask for errors wouldn't want them in the AST.

Also, adding an event emitter is currently the magically "show me errors", not sure if that's supposed to be a 1-1 mapping.

Comment 9

[Staś Małolepszy :stas]

Attachment: Patch

This patch is 80% there.  I need to write more tests and update the existing ones.  Sadly, because of changes to the business logic and API, some of the existing tests are now broken.  

Conceptually, I've identified three types of exceptional behaviors in the compiler.

  A. index defined on the entity didn't resolve (perhaps a reference to a missing entity, or an illegal operation in an expression)
     
     A.1.  emit & throw the RuntimeError
     A.2.  catch it in hashLiteral (I'll probaby rename this to HashValue)
     A.3.  emit & throw IndexError
    (A.4.) catch it in the ctx
    (A.5.) if possible, create subctx
          (A.5.1.) recurse into subctx.get()
    (A.6.) else, return the ID of the requested entity

  B. index resolved fine, but member look-up on a hash or array (propExpr or index) didn't succeed (no such key)

     B.1.  if the default  (or as I like to call it now: catch-all) value is set, use it; this is not an error
     B.2.  else, emit & throw IndexError
    (A.4.) catch it i the ctx
    (...   continue with A.5., A.5.1. and A.6.)

  C. complex string didn't resolve a component
     C.1.  emit & throw the RuntimeError
     C.2.  catch it in stringLitral
     C.3.  emit & throw the ValueError
    (C.4.) catch it in the ctx
    (C.5.) if possible, create subctx
          (C.5.1.) recurse into subctx.get()
    (C.6.) else, return e.source

The context can catch IndexErrors and ValueErrors and act accordingly.

B.1. is I think the most interesting behavior.  It makes the `*key` notation be more about a catch-all value to which the hash defaults.  But the hash will not default to it if resolving the property or the index fails.

The steps in parenthesis above are not implemented in this patch.  They belong in the context (bug 801663).

I didn't test with all the tests that Gandalf submitted in his attachement because I'll first need to take care of  bug 816890 and 802839.  See bug 802843 for tracking.

Comment 10

[Staś Małolepszy :stas]

Perhaps the best documentation right now can be found in the tests:

https://bugzilla.mozilla.org/attachment.cgi?id=688882&action=diff#a/tests/compiler/errors.js_sec1

Comment 11

[Staś Małolepszy :stas]

(In reply to Axel Hecht [:Pike] from comment #8)
> I was wondering, what's the benefit of having errors be signaled outside of
> the AST?

The vast majority off errors which are not syntax errors (caught in the parser) are runtime errors, not even compilation errors.  A reference to an undeclared variable by developer, illegal unary or binary operation whose operands are not known during parsing and/or compilation, a reference to an entity which is not defined in the current context (because addResource() didn't work) etc.

> Also, adding an event emitter is currently the magically "show me errors",
> not sure if that's supposed to be a 1-1 mapping.

The compiler's emitter won't be available from outside of the context by default.  The context can catch/listen to compiler's error events and re-throw/re-emit them for debugging.

Comment 12

[Axel Hecht [:Pike]]

Comment on attachment 688882 [details] [diff] [review]
Patch

Review of attachment 688882 [details] [diff] [review]:
-----------------------------------------------------------------

The internals of this patch are beyond me, tbh.

From an editor point of view, it's important that I can resurrect the source position of parse errors, and insert them back into the AST, because I'll need them there. Quite generally speaking, not having an AST symbol for a piece of text is a problem that I'd need to work around.

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 688882 [details] [diff] [review]
Patch

Review of attachment 688882 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/compiler.js
@@ +221,5 @@
> +    function reset() {
> +      _env = {};
> +      _globals = {};
> +      return this;
> +    }

shouldn't this happen on each compile() ?

@@ +271,5 @@
>      // `toString` is the only method that is supposed to be used by the L20n's 
>      // context.
>      Entity.prototype.toString = function toString(ctxdata) {
> +      try {
> +        return this._resolve(ctxdata);

using try... catch in hot functions is risky. How often do you expect the exception will be thrown here?

Comment 14

[Staś Małolepszy :stas]

(In reply to Zbigniew Braniecki [:gandalf] from comment #13)
> Comment on attachment 688882 [details] [diff] [review]
> Patch
> 
> Review of attachment 688882 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: lib/compiler.js
> @@ +221,5 @@
> > +    function reset() {
> > +      _env = {};
> > +      _globals = {};
> > +      return this;
> > +    }
> 
> shouldn't this happen on each compile() ?

It could; right now there are no cases when we compile an AST and we want it to extend an existing _env.

That said, I don't call reset() anywhere else than in tests, either, so I don't think this is needed.  Each context will need its own compiler instance anyways.


> 
> @@ +271,5 @@
> >      // `toString` is the only method that is supposed to be used by the L20n's 
> >      // context.
> >      Entity.prototype.toString = function toString(ctxdata) {
> > +      try {
> > +        return this._resolve(ctxdata);
> 
> using try... catch in hot functions is risky. How often do you expect the
> exception will be thrown here?

This is *the* try...catch block on the compiler.  It's where all Runtime and Value Errors are caught and passed further to the context.  We throw whenever there's an unresolved reference to an entity, variable or macro, or when an expression throws because of a type mismatch or something else.  It should be rare and only when we get on the error path.  I don't use exceptions otherwise.

Comment 15

[Staś Małolepszy :stas]

https://github.com/l20n/l20n.js/commit/1464bcb2a25b83f30066aad18cbcca73272eb612

\o/

Comment 16

[Staś Małolepszy :stas]

Attachment: tests.lol updated to 1.0 syntax

I don't mean to revive this bug, but I just fixed bug 884229 which happened to be the last one that caused the tests.lol file attached here to fail. \o/

For posterity, I attach an updated version of the file which I used for testing.