1502792 - Add error reporting functions for BinAST similar to regular parser

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Tooru Fujisawa [:arai]]

we need error reporting functions that receive error number and format parameters,
and also variants for strict error, warning, etc.

Comment 1

[Tooru Fujisawa [:arai]]

I'm trying to use the similar structure as regular Parser/TokenStream, in BinAST code, and now I'm wondering about the situation in regular parser.

I thought we were trying to decouple Parser and TokenStream, regarding error reporting, but don't remember correctly.
Currently Parser calls TokenStream classes' {computeErrorMetadata,compileWarning,reportExtraWarningErrorNumberVA,reportStrictModeErrorNumberVA}, and directly calls ReportCompileError.

Are we going to remove dependency for report*VA and compileWarning? or remove call to ReportCompileError and use TokenStream classes for normal error?

Comment 2

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #1)
> Currently Parser calls TokenStream classes'
> {computeErrorMetadata,compileWarning,reportExtraWarningErrorNumberVA,
> reportStrictModeErrorNumberVA}, and directly calls ReportCompileError.

to be clear, some error reporting goes into TokenStream (compileWarning,reportExtraWarningErrorNumberVA,
> reportStrictModeErrorNumberVA) and some others (methods that calls ReportCompileError directly) don't,
and I guess that was not the expected state.

Comment 3

[Eric Faust [:efaust]]

what's the difference between this stuff and the current ErrorReporter impls that we already have in BinASTParser? Are we not expanding expressions well, or something?

Comment 4

[Tooru Fujisawa [:arai]]

there are the following differences
  * BinAST parser lacks error,warning,strictModeError(?) etc which receives error number and args
  * BinAST parser inherits ErrorReporter, while in regular parser TokenStream does
    (who is responsible for finally handling error differs)

Comment 5

[Eric Faust [:efaust]]

We care about semantic equivalence. 

I agree we should add warnings and strict errors (if they're not there). The reason they're not there already is because I didn't think it was worth it at the time, but we clearly need them.

I am not off-hand swayed by the idea that one class vs the other owns the functions and that being a big deal. Is the problem that you don't want to re-implement them for streaming compilation? That would make sense.

Comment 6

[Jeff Walden [:Waldo]]

(In reply to Tooru Fujisawa [:arai] from comment #1)
> I'm trying to use the similar structure as regular Parser/TokenStream, in
> BinAST code, and now I'm wondering about the situation in regular parser.
> 
> I thought we were trying to decouple Parser and TokenStream, regarding error
> reporting, but don't remember correctly.
> Currently Parser calls TokenStream classes'
> {computeErrorMetadata,compileWarning,reportExtraWarningErrorNumberVA,
> reportStrictModeErrorNumberVA}, and directly calls ReportCompileError.
> 
> Are we going to remove dependency for report*VA and compileWarning? or
> remove call to ReportCompileError and use TokenStream classes for normal
> error?

Honestly at this point I don't know.  :-|  The virtual-inheritance games that have been added to all this are kind of calcifying in place a bad setup, IMO.  I don't know that there's a clear, coherent way to get rid of the fugly, precisely.  And I'm not really thinking hard about it right now, with UTF-8 compilation falling into place well at this point and being more important than purity concerns of lesser importance.

Comment 7

[Tooru Fujisawa [:arai]]

(In reply to Eric Faust [:efaust] from comment #5)
> We care about semantic equivalence. 
> 
> I agree we should add warnings and strict errors (if they're not there). The
> reason they're not there already is because I didn't think it was worth it
> at the time, but we clearly need them.
> 
> I am not off-hand swayed by the idea that one class vs the other owns the
> functions and that being a big deal. Is the problem that you don't want to
> re-implement them for streaming compilation? That would make sense.

to add better error reporting (including strict mode error which blocks bug 1500836) to BinAST,
we'll need to re-implement some of those error reporting wrapper over ErrorReporter (or as ErrorReporter method?) anyway,
and I wanted to avoid adding similar but slightly-different code between regular and BinAST parsers, which becomes the source of confusion (at least for me),
(at first I hoped to make those wrapper methods a mixin or something to avoid code duplication, but that's not required goal here)

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Add ErrorReportMixin which implements error reporting methods, and make them available in Parser and Tokenizer.

Moved most of error reporting methods from Parser/TokenStream to newly added  ErrorReportMixin class, which is now a base class for ParserBase and ErrorReporter.

now same error reporting functions can be used in Parser and TokenStream, and also BinASTParser (unfortunately BinTokenReaderBase is not yet subclass of it)

will ask review once blocking bugs are fixed.

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Add ErrorReportMixin which implements error reporting methods, and make them available in Parser and Tokenizer.

the following error reporting functions are moved (or added) to
the newly created ErrorReportMixin class:
  * error
  * errorWithNotes
  * errorAt
  * errorWithNotesAt
  * errorNoOffset
  * errorWithNotesNoOffset
  * errorWithNotesAtVA
  * warning
  * warningWithNotes
  * warningAt
  * warningWithNotesAt
  * warningNoOffset
  * warningWithNotesNoOffset
  * warningWithNotesAtVA
  * extraWarning
  * extraWarningWithNotes
  * extraWarningAt
  * extraWarningWithNotesAt
  * extraWarningNoOffset
  * extraWarningWithNotesNoOffset
  * extraWarningWithNotesAtVA
  * strictModeError
  * strictModeErrorWithNotes
  * strictModeErrorAt
  * strictModeErrorWithNotesAt
  * strictModeErrorNoOffset
  * strictModeErrorWithNotesNoOffset
  * strictModeErrorWithNotesAtVA
  * compileWarning

ErrorReportMixin is the base class of ErrorReporter.
the class hierarchy is the following


                    TokenStreamCharsShared
                      ^
                      |
                    TokenStreamCharsBase<Unit>
                      ^
                      |
                    SpecializedTokenStreamCharsBase<Unit>
                      ^
                      |
                    GeneralTokenStreamChars<Unit, AnyCharsAccess>
  StrictModeGetter    ^
    ^                 |
    |               TokenStreamChars<Unit, AnyCharsAccess>
  ErrorReportMixin    ^
    ^    ^            |                          TokenStreamShared
    |    |            |                            ^    ^
    |  ErrorReporter  |                            |    |
    |    ^     ^      |                            |  TokenStreamAnyChars
    |    |     |      |                            |    ^
    |    |   TokenStreamSpecific<Unit, AnyCharsAccess>  |
    |    |     ^                                        |
    |    |     |     +----------------------------------+
    |    |     |     |
    |    |   TokenStream
    |    |                   JS::AutoGCRooter
    |    |                     ^    ^
    |    |                     |    |
    |    |  BCEParserHandle    |    |
    |    |    ^                |    |
    |    |    |  BinASTParserBase   |
    |    |    |    ^                |
    |    |    |    |                |
    |    |    |    |                |
    |  BinASTParser<Tok>            |
    |                               |
    |    +--------------------------+
    |    |
  ParserBase
    ^
    |
  PerHandlerParser<ParseHandler>
    ^
    |
  GeneralParser<ParseHandler, Unit>
    ^
    |
  Parser<ParseHandler, Unit>


Class that inherits ErrorReportMixin needs to implement the following virtual methods:
  * for StrictModeGetter
    * strictMode
  * for ErrorReportMixin
    * options
    * getContext
    * computeErrorMetadata

And the following virtual methods are remaining in ErrorReporter:
  * lineAndColumnAt
  * currentLineAndColumn
  * isOnThisLine
  * lineAt
  * columnAt
  * hasTokenizationStarted
  * getFilename

As it's clear from the hierarchy above, TokenStreamAnyChars doesn't have access to ErrorReportMixin's methods, and it still has their own error reporting methods.

other places are changed to use ErrorReportMixin's methods (mostly TokenStream* and BinAST parsers.

there's one more case in testBinTokenReaderTester, which is just a dummy reporter class that is necessary to just instantiate BinAST tokenizer.
it does nothing, but just checks if the error reporting methods are not called.

Comment 10

[Tooru Fujisawa [:arai]]

Comment on attachment 9024916 [details] [diff] [review]
Add ErrorReportMixin which implements error reporting methods, and make them available in Parser and Tokenizer.

apparently there's some issue

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Add ErrorReportMixin which implements error reporting methods, and make them available in Parser and Tokenizer.

fixed some bugs.

details in comment #9

Comment 12

[Tooru Fujisawa [:arai]]

Comment on attachment 9024993 [details] [diff] [review]
Add ErrorReportMixin which implements error reporting methods, and make them available in Parser and Tokenizer.

I'll re-post rebased one, given filename/indent and some more things are already changed.

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Add ErrorReportMixin which implements error reporting methods, and make them available in Parser and Tokenizer.

Rebased onto the format, and also bug 1508063 which introduced another class to BinASTParser.

Comment 14

[Jeff Walden [:Waldo]]

Comment on attachment 9031285 [details] [diff] [review]
Add ErrorReportMixin which implements error reporting methods, and make them available in Parser and Tokenizer.

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

This all looks largely sensible.  Just, nits.

::: js/src/frontend/BinASTParserPerTokenizer.cpp
@@ +762,5 @@
>  
>  template <typename Tok>
> +JSContext* BinASTParserPerTokenizer<Tok>::getContext() const {
> +  return cx_;
> +}

Fine enough, but defining such ridiculously simple definition inline in the header at declaration site in a followup seems like it would be good.

::: js/src/frontend/ErrorReporter.h
@@ +15,5 @@
>  
>  #include "js/CompileOptions.h"
>  #include "js/UniquePtr.h"
>  
> +#include "vm/ErrorReporting.h"  // for ErrorMetadata

No blank line above this.  Also I'd remove "for " and technically this should have ReportCompile{Error,Warning} in it too.

@@ +65,5 @@
> +  // ==== error ====
> +  //
> +  // Reports an error.
> +  //
> +  // Methods ends with "At" are for an error at given offset.

"Methods ending with" for all parts of this comment.

@@ +72,5 @@
> +  //
> +  // Methods ends with "NoOffset" are for an error that doesn't correspond to
> +  // any offset. NoOffset is passed to computeErrorMetadata for them.
> +  //
> +  // Other methods except errorWithNotesAtVA are for an error at current

at the current

@@ +78,5 @@
> +  //
> +  // Methods contains "WithNotes" can be used if there are error notes.
> +  //
> +  // errorWithNotesAtVA is the actual implementation for all of above.
> +  // This can be called if the caller already has va_list.

has a va_list

@@ +85,5 @@
> +    va_list args;
> +    va_start(args, errorNumber);
> +
> +    errorWithNotesAtVA(nullptr, mozilla::AsVariant(Current()), errorNumber,
> +                       args);

With changes noted elsewhere, all these sorts of things will be passing &args.

@@ +138,5 @@
> +    va_end(args);
> +  }
> +  void errorWithNotesAtVA(UniquePtr<JSErrorNotes> notes,
> +                          const ErrorOffset& offset, unsigned errorNumber,
> +                          va_list args) {

va_list* not va_list as noted elsewhere.

@@ +156,5 @@
> +  // Returns true if the warning is reported.
> +  // Returns false if the warning is treated as an error, or an error occurs
> +  // while reporting.
> +  //
> +  // See the comment for error section for the detail of arguments.

Something like "See the comment on the error section for details on what the arguments and function names indicate for all these functions."  Ditto on the other sections below.

@@ +230,5 @@
> +    return result;
> +  }
> +  MOZ_MUST_USE bool warningWithNotesAtVA(UniquePtr<JSErrorNotes> notes,
> +                                         const ErrorOffset& offset,
> +                                         unsigned errorNumber, va_list args) {

Soooooooooo...

The reason functions like this were taking va_list* is that technically it's not legal to copy va_lists around *unless* you use the std::va_copy function to do it.  And of course even passing std::va_copy(args) will *itself* induce a copy, so that too isn't correct.  The only safe way to pass around is by va_list* or va_list&.  Please revert/adjust all these places passing va_list by value to pass by pointer instead (again).

(Of course the truly right approach to this is to have this goo be a std::initializer_list<>-initialized type that stores pointer and length, but that change is a bit bigger and is not on anyone's beaten path yet.  So just keep doing it the slightly-less-broken way for now.)

@@ +323,5 @@
> +  }
> +  MOZ_MUST_USE bool extraWarningWithNotesAtVA(UniquePtr<JSErrorNotes> notes,
> +                                              const ErrorOffset& offset,
> +                                              unsigned errorNumber,
> +                                              va_list args) {

va_list* not va_list, as noted elsewhere.

@@ +421,5 @@
> +  }
> +  MOZ_MUST_USE bool strictModeErrorWithNotesAtVA(UniquePtr<JSErrorNotes> notes,
> +                                                 const ErrorOffset& offset,
> +                                                 unsigned errorNumber,
> +                                                 va_list args) {

va_list* not va_list as noted elsewhere.

::: js/src/frontend/TokenStream.cpp
@@ +585,5 @@
> +void TokenStreamAnyChars::reportErrorNoOffset(unsigned errorNumber, ...) {
> +  va_list args;
> +  va_start(args, errorNumber);
> +
> +  reportErrorNoOffsetVA(errorNumber, args);

&args

@@ +591,5 @@
> +  va_end(args);
> +}
> +
> +void TokenStreamAnyChars::reportErrorNoOffsetVA(unsigned errorNumber,
> +                                                va_list args) {

va_list*

@@ +1456,5 @@
>  
>  template <typename Unit, class AnyCharsAccess>
> +JSContext* TokenStreamSpecific<Unit, AnyCharsAccess>::getContext() const {
> +  return anyCharsAccess().cx;
> +}

Define at declaration site, please.

@@ +1461,5 @@
> +
> +template <typename Unit, class AnyCharsAccess>
> +bool TokenStreamSpecific<Unit, AnyCharsAccess>::strictMode() const {
> +  return anyCharsAccess().strictMode();
> +}

Same.

::: js/src/wasm/AsmJS.cpp
@@ +1453,5 @@
>          // an exception is set and execution will halt.  Thus it's safe
>          // and correct to ignore the return value here.
> +        Unused << ts.compileWarning(std::move(metadata), nullptr,
> +                                    JSREPORT_WARNING, JSMSG_USE_ASM_TYPE_FAIL,
> +                                    args);

The odds of this sensible formatting being correct according to our new Google overlords seems low, but you tell me.  :-)

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Use va_list* instead of va_list in ReportCompile{Error,Warning} parameter type.

as a preparation to use va_list* in ErrorReporter, fixed ReportCompile{Error,Warning}.

remaining part that uses va_list should be handled in other bugs.

Comment 16

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a78a7d20a1f7d99d8680140a20353664179b6f1
Bug 1502792 - Part 1: Use va_list* instead of va_list in ReportCompile{Error,Warning} parameter type. r=jwalden

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee32392ef5b3ce7e5f72cf052458a2dd139be386
Bug 1502792 - Part 2: Add ErrorReportMixin which implements error reporting methods, and make them available in Parser and Tokenizer. r=jwalden

Comment 17

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/8a78a7d20a1f
https://hg.mozilla.org/mozilla-central/rev/ee32392ef5b3