Closed Bug 802845 Opened 9 years ago Closed 9 years ago

Compiler should emit Exceptions on errors for easier debugging

Categories

(L20n :: JS Library, defect, P2)

x86_64
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: stas, Assigned: stas)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
Blocks: 802850
No longer blocks: 802843
Assignee: nobody → stas
Priority: -- → P2
Target Milestone: --- → 1.0
Attached patch Patch (obsolete) — Splinter Review
Attached patch Patch (obsolete) — Splinter Review
Attachment #684767 - Attachment is obsolete: true
Attachment #684768 - Attachment is obsolete: true
Attached wrong files, sorry.
Attached patch WIP patch (obsolete) — Splinter Review
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.
Attachment #684773 - Flags: feedback?(gandalf)
Blocks: 801663
Attached file tests (obsolete) —
Duplicate of this bug: 803399
Bug 803399 (which I duped) was blocking-parkcity+; carrying the + over to here.
Flags: blocking-parkcity+
Blocks: 802843
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.
Attached patch PatchSplinter Review
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.
Attachment #684773 - Attachment is obsolete: true
Attachment #684773 - Flags: feedback?(gandalf)
Attachment #688882 - Flags: review?(gandalf)
Attachment #688882 - Flags: feedback?(l10n)
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
(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 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.
Attachment #688882 - Flags: feedback?(l10n)
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?
Attachment #688882 - Flags: review?(gandalf) → review+
(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.
https://github.com/l20n/l20n.js/commit/1464bcb2a25b83f30066aad18cbcca73272eb612

\o/
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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.
Attachment #687487 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.