1347711 - Refactor error reporting out of TokenStream

Status: RESOLVED INVALID

(Core :: JavaScript Engine, enhancement)

Description

[Shu-yu Guo [:shu]]

One of the things needed to make TokenStream only deal with tokenizing, and thus easier to templatize to different char types.

We should audit and refactor TokenStream::reportError and the surrounding methods to top-level functions in the js::frontend namespace in a new file, like frontend/ErrorReport.{cpp,h}. Some of these methods use the position of the current token; these should be refactored to take a position explicitly.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

There seems to be some interleaving between Parser.{h, cpp}'s error reporting and TokenStream.{h, cpp}. I'm refactoring only the latter for the moment, but if we need to do it for the parser, too, we can do that in a followup bug.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

I'm working on it atm, but it's a little ugly, as we need to pass much more than a position. For the moment, we also need a `JSContext*`, a `ReadOnlyCompileOptions` and for one of the functions a `StrictModeGetter`. I wonder if we shouldn't rather move these methods to a non-templatized `TokenStreamBase` class.

What do you think, shu?

Comment 3

[Shu-yu Guo [:shu]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #2)
> I'm working on it atm, but it's a little ugly, as we need to pass much more
> than a position. For the moment, we also need a `JSContext*`, a
> `ReadOnlyCompileOptions` and for one of the functions a `StrictModeGetter`.
> I wonder if we shouldn't rather move these methods to a non-templatized
> `TokenStreamBase` class.
> 
> What do you think, shu?

Hm, that sounds fine to me but then also steps on Waldo's part of the refactoring. I think he did mention the error reporting refactoring ended up blocking whatever he was doing. (We thought it'd be an orthogonal refactor.)

Comment 4

[Shu-yu Guo [:shu]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #1)
> There seems to be some interleaving between Parser.{h, cpp}'s error
> reporting and TokenStream.{h, cpp}. I'm refactoring only the latter for the
> moment, but if we need to do it for the parser, too, we can do that in a
> followup bug.

Yeah, ultimately I'd want there to be a single set of error reporting functions. It's such a mess right now. :(

Comment 5

[David Teller [:Yoric] - still alive but not very active]

The alternative would be to pass as argument a `TokenStream` to `reportError` & co. Is this better?

Comment 6

[Shu-yu Guo [:shu]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #5)
> The alternative would be to pass as argument a `TokenStream` to
> `reportError` & co. Is this better?

If by TokenStream you mean the non-base class, then no, that'd run into the same problem we are now: duplicating all the error code if they have to be templatized on both TokenStream<char> and TokenStream<char16_t>, when we get to specializing the TokenStream.

Comment 7

[Shu-yu Guo [:shu]]

If there's too much state to pass around, perhaps a frontend::ErrorReporter class that Parser and BCE could hold on to for error reporting instead. As long as we separate it out of TokenStream.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Right now, I can't see how to design such an `ErrorReporter`, so I'm sorely tempted to go towards `TokenStreamBase`.

Waldo, would it conflict with your work if I create a class `TokenStreamBase` holding only the necessary data for error reporting?

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1347711 - Refactor error reporting out of TokenStream;

Prior to a large refactoring/templatization on TokenStream, let's get
error reporting out of the way.

Review commit: https://reviewboard.mozilla.org/r/121962/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121962/

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8849134 [details]
Bug 1347711 - Refactor error reporting out of TokenStream;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121962/diff/1-2/

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8849134 [details]
Bug 1347711 - Refactor error reporting out of TokenStream;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121962/diff/2-3/

Comment 12

[Jeff Walden [:Waldo]]

I was taking the slightly different tack of separating everything in TokenStream that's character-type-*agnostic* into TokenStreamBase.  TokenStream<CharT> would be the outermost final class, and it would inherit from TokenStreamBase that contains most everything but CharBuffer and TokenBuf and such.  (And, it happens, a bunch of different stuff like getTokenInternal and anything that indirectly would call it, and of course all the error-reporting stuff and anything that calls it, so the split isn't nearly as clean as one might wish.)

This sounds fairly conflicty with that plan.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Yes, my original idea was to move error reporting in TokenStreamBase *because* it's chartype-agnostic.

Anyway, the patch I have uploaded on mozreview moves error reporting out of TokenStream entirely, so I suspect that there should be no meaningful conflict.

Comment 14

[Shu-yu Guo [:shu]]

Comment on attachment 8849134 [details]
Bug 1347711 - Refactor error reporting out of TokenStream;

https://reviewboard.mozilla.org/r/121962/#review124188

Asked for some pretty big changes about the abstract base class, so clearing review for now.

::: js/src/frontend/ErrorReport.h:25
(Diff revision 3)
> +
> +// Additional data on an error.
> +//
> +// This (almost) purely virtual class provides signatures to access the
> +// context data maintained by TokenStream, without having to depend on
> +// the full implementation of TokenStream.

I'm not a fan of an abstract base class here for the sake of providing accessors to error data.

Most of the state that's pulled off the TokenStream is used for filling out the various fields of CompileError (fields on JSErrorReport) in |reportCompileErrorNumberVA|. 

One obvious alternative is to limit the error handling code in TokenStream to be a method that fills out a CompileError, or that returns a UniquePtr<CompileError>. Waldo is in favor of that too, I believe.

::: js/src/frontend/ErrorReport.h:26
(Diff revision 3)
> +// Additional data on an error.
> +//
> +// This (almost) purely virtual class provides signatures to access the
> +// context data maintained by TokenStream, without having to depend on
> +// the full implementation of TokenStream.
> +class ErrorContext {

Style nit: { on new line

::: js/src/frontend/ErrorReport.h:41
(Diff revision 3)
> +
> +    // The options with which the source is compiled.
> +    virtual const JS::ReadOnlyCompileOptions& options() const = 0;
> +
> +    // `true` if we are operating in strict mode, `false` otherwise.
> +    virtual bool strictMode() const = 0;

I feel like computing the JSREPORT_{WARNING,ERROR,STRICT} flags should be done at the calls instead of being figured out in helper functions. Then we can cut down on the # of error reporting functions themselves.

::: js/src/frontend/ErrorReport.h:109
(Diff revision 3)
> +
> +//---- Specialized error reporters
> +
> +void reportInvalidEscapeError(ErrorContext* context, InvalidEscapeType type);
> +
> +bool reportTokenError(ErrorContext* context, unsigned errorNumber, ...);

This is a good opportunity to add the MOZ_MUST_USE attribute to these return booleans and audit the callsites that ignore the value. I'm also not 100% sure why they're bool instead of void -- I think because depending on strict error options, some errors turn into warnings and thus return true instead of false.

Also style nit: capitalize top-level functions.

Comment 15

[Shu-yu Guo [:shu]]

Waldo, if we limit TokenStream's error handling to precomputing some location information and putting it into JSErrorReport and refactor all the reporter functions themselves out of TokenStream, hopefully that should be fairly orthogonal?

Comment 16

[Jeff Walden [:Waldo]]

I think my sense for how this should look, is that these bits of random computation inside TokenStream::reportCompileErrorNumberVA that depend on TokenStream guts, should be computed by the *caller* of each place that reports an error, in whatever the right way is.  So instead of currently having an argument list of

  (TokenStream* this, UniquePtr<JSErrorNotes> notes, uint32_t offset,
   unsigned flags, unsigned errorNumber, va_list args)

have 

  (unsigned errorNumber, unsigned flags,
   struct ErrorLocation* { uint32_t line; uint32_t column; const char* filename; },
   UniquePtr<char[]> lineContext, UniquePtr<JSErrorNotes> notes, va_list args)

where the caller computes the relevant new parameters to pass in.  The parser could ask the TokenStream to fill in an ErrorLocation, it could ask the TokenStream to compute a line of context (applying all the windowing it wants to that), then pass those things to throw the ultimate error.  Errors spawned in the tokenizer can separately compute those things.  Same for the bytecode emitter.

Generally, it just seems like an inordinate amount of overhead here to have an abstract class and subclasses of it and vtables and all that sort of thing.

Comment 17

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8849134 [details]
Bug 1347711 - Refactor error reporting out of TokenStream;

https://reviewboard.mozilla.org/r/121962/#review124188

> I'm not a fan of an abstract base class here for the sake of providing accessors to error data.
> 
> Most of the state that's pulled off the TokenStream is used for filling out the various fields of CompileError (fields on JSErrorReport) in |reportCompileErrorNumberVA|. 
> 
> One obvious alternative is to limit the error handling code in TokenStream to be a method that fills out a CompileError, or that returns a UniquePtr<CompileError>. Waldo is in favor of that too, I believe.

I have tried gathering the logics into a `TokenStream::compileError()`, but this basically means that we keep all of the error reporting inside `TokenStream`, so I believe that it's not really useful.

> I feel like computing the JSREPORT_{WARNING,ERROR,STRICT} flags should be done at the calls instead of being figured out in helper functions. Then we can cut down on the # of error reporting functions themselves.

Agreed. I figured we could do this in a followup, but we can do this now if you prefer.

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8849134 [details]
Bug 1347711 - Refactor error reporting out of TokenStream;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121962/diff/3-4/

Comment 19

[Shu-yu Guo [:shu]]

Comment on attachment 8849134 [details]
Bug 1347711 - Refactor error reporting out of TokenStream;

https://reviewboard.mozilla.org/r/121962/#review124578

Per IRC conversations, it's unfortunate the error handling ended up 1) more entangled than I'd thought and 2) not amenable to being refactored in parallel. It'll be less painful all around to have this be done by Waldo's TokenStream refactor.

Comment 20

[Shu-yu Guo [:shu]]

Sorry for the wasted time, all. :(