1501928 - Remove MUST_MATCH_TOKEN* macros in Parser

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Tooru Fujisawa [:arai]]

Parser.cpp uses many MUST_MATCH_TOKEN* macros, which is too complicated.
We should remove them and maybe replace with template magic method etc.

this might help bug 1501798

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Add GeneralParser::mustMatchToken and use it instead of simple variants of MUST_MATCH_TOKEN_*.

Added GeneralParser::mustMatchToken method, that gets the next token and checks if it matches to the expected token, and reports error if not.
This will become template method in Part 2 and 3.

at first I was thinking just expanding the macro in callsites, but I guess this is simpler, at least for Part 1.
(maybe Part 2 and 3 cases are not)

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Remove MUST_MATCH_TOKEN_FUNC* macros.

Made the 1st parameter (expected token) of GeneralParser::mustMatchToken a template,
which receives either:
  * a lambda that receives the actual token and returns bool (match or not)
  * an expected TokenKind

then, given method cannot be specialized inside unspecialized class,
Added ConditionMatchHelper function and left specialization to it.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Remove MUST_MATCH_TOKEN_MOD_WITH_REPORT* macros.

Made the last parameter (error number) of GeneralParser::mustMatchToken a template,
which receives either:
  * a lambda that receives the actual token and reports error
  * an error number

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 9021424 [details] [diff] [review]
Part 3: Remove MUST_MATCH_TOKEN_MOD_WITH_REPORT* macros.

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

If I could think of different names for all these different flavors of overload (and in the prior patches here), I would suggest them, but I'm coming up empty thinking about it super-quickly.

::: js/src/frontend/Parser.cpp
@@ +10900,5 @@
>      }
>  
> +    if (!mustMatchToken(TokenKind::RightCurly, TokenStream::Operand, [&](TokenKind actual) {
> +                reportMissingClosing(JSMSG_CURLY_AFTER_LIST, JSMSG_CURLY_OPENED, openedPos);
> +            }))

I'd prefer if all these explicitly listed out the things they capture, with [this] and explicitly |this->|-qualifying the function call being performed in the lambda actually being a member function call, not a standalone call.

Also -- I think we tend to align this like so:

    if (!mustMatchToken(TK::RC, TS::O, [this, &openedPos](TK actual) {
                            this->rMC(...);
                        }))
    {
        return null();
    }

with the brace matching up with the first column of the contents of the enclosing function call arguments and the body indented the usual four from that.

::: js/src/frontend/Parser.h
@@ +935,5 @@
>      }
>  
> +    template<typename ConditionT, typename ErrorReportT>
> +    MOZ_MUST_USE bool mustMatchToken(ConditionT condition, Modifier modifier,
> +                                     ErrorReportT errorReport);

I'm not horribly enamored of templating this all the way up so far.  Maybe have the fully-templated version named as mustMatchTokenInternal, then add

  MOZ_MUST_USE bool mustMatchToken(TokenKind expected, Modifier modifier, unsigned errorNumber) {
    return mustMatchToken([expected](TokenKind token) { return token == expected; },
                          modifier,
                          [this, errorNumber]() { return this->error(errorNumber); });
  }

  MOZ_MUST_USE bool mustMatchToken(TokenKind expected, unsigned errorNumber) {
    return mustMatchToken(expected, TokenStream::None, errorNumber);
  }

and similar very-clear overloads for the simple cases, and leave the goofier must-be-template cases segregated off to the side where you don't have to deeply grok template parameter meanings to understand/use it.

Comment 5

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b537bec9ef64ce6cf60aa1d13110c5a84b780ba0
Bug 1501928 - Part 1: Add GeneralParser::mustMatchToken and use it instead of simple variants of MUST_MATCH_TOKEN_*. r=Waldo

https://hg.mozilla.org/integration/mozilla-inbound/rev/5511494bb3e23dc14a8c4388ea25a897d3406dba
Bug 1501928 - Part 2: Remove MUST_MATCH_TOKEN_FUNC* macros. r=Waldo

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c2f329b5f3ff9534fe11999daf1c8d823ef76f7
Bug 1501928 - Part 3: Remove MUST_MATCH_TOKEN_MOD_WITH_REPORT* macros. r=Waldo

Comment 6

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb5fb585b24e
Backed out 3 changesets for causing parser build bustages CLOSED TREE

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Part 3.1: Stop using mustMatchToken in GeneralParser::functionFormalParametersAndBody to avoid ICE

This specific case causes ICE on linux64 SM(H) build

https://treeherder.mozilla.org/logviewer.html#?job_id=209363158&repo=mozilla-inbound&lineNumber=4473
> [task 2018-11-02T07:12:32.463Z] /builds/worker/checkouts/gecko/js/src/frontend/Parser.cpp: In instantiation of 'js::frontend::GeneralParser<ParseHandler, Unit>::functionFormalParametersAndBody(js::frontend::InHandling, js::frontend::YieldHandling, js::frontend::GeneralParser<ParseHandler, Unit>::CodeNodeType*, js::frontend::FunctionSyntaxKind, const mozilla::Maybe<unsigned int>&, bool)::<lambda(js::frontend::TokenKind)> [with ParseHandler = js::frontend::FullParseHandler; Unit = mozilla::Utf8Unit]':
> [task 2018-11-02T07:12:32.464Z] /builds/worker/checkouts/gecko/js/src/frontend/Parser.cpp:4235:36:   required from 'struct js::frontend::GeneralParser<ParseHandler, Unit>::functionFormalParametersAndBody(js::frontend::InHandling, js::frontend::YieldHandling, js::frontend::GeneralParser<ParseHandler, Unit>::CodeNodeType*, js::frontend::FunctionSyntaxKind, const mozilla::Maybe<unsigned int>&, bool) [with ParseHandler = js::frontend::FullParseHandler; Unit = mozilla::Utf8Unit; js::frontend::GeneralParser<ParseHandler, Unit>::CodeNodeType = js::frontend::CodeNode*]::<lambda(enum class js::frontend::TokenKind)>'
> [task 2018-11-02T07:12:32.464Z] /builds/worker/checkouts/gecko/js/src/frontend/Parser.cpp:4234:28:   required from 'bool js::frontend::GeneralParser<ParseHandler, Unit>::functionFormalParametersAndBody(js::frontend::InHandling, js::frontend::YieldHandling, js::frontend::GeneralParser<ParseHandler, Unit>::CodeNodeType*, js::frontend::FunctionSyntaxKind, const mozilla::Maybe<unsigned int>&, bool) [with ParseHandler = js::frontend::FullParseHandler; Unit = mozilla::Utf8Unit; js::frontend::GeneralParser<ParseHandler, Unit>::CodeNodeType = js::frontend::CodeNode*]'
> [task 2018-11-02T07:12:32.464Z] /builds/worker/checkouts/gecko/js/src/frontend/Parser.cpp:11266:16:   required from here
> [task 2018-11-02T07:12:32.464Z] /builds/worker/checkouts/gecko/js/src/frontend/Parser.cpp:4238:29: internal compiler error: in dependent_type_p, at cp/pt.c:22674
> [task 2018-11-02T07:12:32.464Z]                              }))
> [task 2018-11-02T07:12:32.464Z]                              ^

https://github.com/gcc-mirror/gcc/blob/45dd06cef49fe00a7839d7dff312b09e88910a51/gcc/cp/pt.c#L22674
> bool
> dependent_type_p (tree type)
> {
>   /* If there are no template parameters in scope, then there can't be
>      any dependent types.  */
>   if (!processing_template_decl)
>     {
>       /* If we are not processing a template, then nobody should be
> 	 providing us with a dependent type.  */
>       gcc_assert (type);
>       gcc_assert (TREE_CODE (type) != TEMPLATE_TYPE_PARM || is_auto (type));
>       return false;

the last gcc_assert above.

apparently the lambda in the last parameter causes it.
I've replaced that single callsite with the result of previous macro expansion.
do you think we should avoid using template as the last parameter everywhere?
(perhaps this is the error you mentioned before?)

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 9022057 [details] [diff] [review]
Part 3.1: Stop using mustMatchToken in GeneralParser::functionFormalParametersAndBody to avoid ICE

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

If this is necessary, then it is necessary.

IMO writing out a getToken and subsequent test for must-match behavior is not particularly onerous.  At the same time, I am not super-concerned about every instance of this pattern looking identical.  If this works to remove all the other macro-uses, one deviation is not the end of the world IMO.  But do whatever you think is best.

Comment 9

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b07f54650288bf8664ca7a1ef67ca747ff46fdbb
Bug 1501928 - Part 1: Add GeneralParser::mustMatchToken and use it instead of simple variants of MUST_MATCH_TOKEN_*. r=Waldo

https://hg.mozilla.org/integration/mozilla-inbound/rev/6af00f47b82911306268990c6c45a3e5b9dc622d
Bug 1501928 - Part 2: Remove MUST_MATCH_TOKEN_FUNC* macros. r=Waldo

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0ff603045c080877553eb2d3a45abd8ef58a1d0
Bug 1501928 - Part 3: Remove MUST_MATCH_TOKEN_MOD_WITH_REPORT* macros. r=Waldo

Comment 10

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/b07f54650288
https://hg.mozilla.org/mozilla-central/rev/6af00f47b829
https://hg.mozilla.org/mozilla-central/rev/c0ff603045c0