1424420 - Specialize functions in Parser<{Full,Syntax}ParseHandler, CharT> in a manner that doesn't require duplicate textual definitions when multiple CharT are possible

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Jeff Walden [:Waldo]]

Right now we have (with some handwavery here)

template<class ParseHandler, typename CharT>
class Parser
{
   ...
   Node importDeclaration();
   ...
};

template<>
Node
Parser<FullParseHandler, char16_t>::importDeclaration()
{
   ...full definition...
}

template<>
Node
Parser<SyntaxParseHandler, char16_t>::importDeclaration()
{
    return abortIfSyntaxParser();
}

This works to have specialized behavior for full/syntax parsing now.  But when we will also have Parser support for single-byte text, we can't just do, e.g.

template<typename CharT>
Node
Parser<FullParseHandler, CharT>::importDeclaration()
{
   ...full definition...
}

because this is function partial specialization.  And C++ no likey.

The solution I've reached is this:

1) Rename Parser to GeneralParser, and make it not final.
2) Introduce

  template<classParseHandler, typename CharT> class Parser;

  template<typename CharT>
  class Parser<SyntaxParseHandler, CharT> final
    : public GeneralParser<SyntaxParseHandler, CharT>
  {
     ...
     Node importDeclaration();
     ...
  };

  template<typename CharT>
  class Parser<FullParseHandler, CharT> final
    : public GeneralParser<FullParseHandler, CharT>
  { 
     ...
     Node importDeclaration();
     ...
  };

3) Now this *will* work (because class partial specialization *is* permitted, and you're not doing the bad thing to the function):

template<typename CharT>
Node
Parser<FullParseHandler, CharT>::importDeclaration()
{
   ...full definition...
}

template<typename CharT>
Node
Parser<SyntaxParseHandler, CharT>::importDeclaration()
{
   return abortIfSyntaxParser();
}

Of course there's a bit more trickiness than that -- sometimes a function that *remains* in GeneralParser, wants to call one that lives in the Parser specializations, and other things.  But those can be worked around.  The workarounds are:

GeneralParser functions wanting to call Parser functions:
Add a static downcast function to GeneralParser, then add a private inline function in GeneralParser that downcasts and calls accordingly.  So for importDeclaration you might have

template<class ParseHandler, typename CharT>
inline Node
GeneralParser<ParseHandler, CharT>::importDeclaration()
{
    return asFinalParser()->importDeclaration();
}

Parser wanting to use inherited functions/fields/types:
Because GeneralParser<PH, CharT> is a dependent name, every reference to a field/function in it has to be via a dependent name.  The solution everyone understands, is to use |this->| everywhere.  But that's a big mess, so instead this adds |using Base::tokenStream;| to the two specializations, eliminating the need to touch hundreds of lines of code.  The big list of |using|s isn't great, but it's way better than the alternative.

Multiple GP<...>::* function overloads with different access rules:
You can't |using Base::foo;| if |foo| is private.  I forwarded from Parser to GeneralParser in this case using an inline function -- or by renaming one of the underlying overloads.  (Note particularly that Parser::innerFunction is now two differently-named functions, which is way way way less confusing to read, entirely independent of this bug's aim.)

AutoAwaitIsKeyword:
I changed how you invoke AAIK so that it takes GeneralParser instead of Parser, so that users wouldn't have to pass a slightly weird asFinalParser().  That bumps line lengths a bit to squeeze in a GP<...>; it's possible I'll think of a slightly nicer way to skin that cat, but this *does* work now.

There are additional simplifications that can be done atop this -- if you specialize Parser::exportDeclaration, for example, then you can move *all* the different export-handling functions from GeneralParser into Parser<FullParseHandler, CharT> and kill off a bunch of dumb GeneralParser<ParseHandler, CharT>::* export functions that just do |asFinalParser()->exportGoop()|.  But this is a gonzo patch already, and such is way better deferred than piling on.  (I suppose I could have done it before this patch, too -- but I didn't think of it til I was most of the way into this, and the work to flip that is not worth the pain.)

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

Unfortunately, this patch is more or less not split-uppable.  Fortunately, a great deal of the changing is in whitespace or just adding "General" before "Parser".  I'll post a -w version after this for additional helpfulness.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Patch with -w

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Adjust AutoAwaitIsKeyword template parameters slightly so a full parser type need not be written out

This seems like adequate improvement on the AAIK system mentioned in comment 0.

Comment 4

[Tooru Fujisawa [:arai]]

I'll review them tomorrow.

Comment 5

[Tooru Fujisawa [:arai]]

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

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

::: js/src/frontend/Parser.h
@@ +1135,5 @@
> +    bool checkExportedNameForFunction(Node node);
> +    bool checkExportedNameForClass(Node node);
> +    inline bool checkExportedNameForClause(Node node);
> +
> +    bool trySyntaxParseInnerFunction(Node pn, HandleFunction fun, uint32_t toStringStart,

both "pn" and "node" are used for genera/syntax/full parsers, in declaration and definition.
it might be nice to align to either.

@@ +1141,5 @@
> +                                     FunctionSyntaxKind kind, GeneratorKind generatorKind,
> +                                     FunctionAsyncKind asyncKind, bool tryAnnexB,
> +                                     Directives inheritedDirectives, Directives* newDirectives);
> +
> +    bool skipLazyInnerFunction(Node node, uint32_t toStringStart, FunctionSyntaxKind kind,

same here (pn vs node).

Comment 6

[Jeff Walden [:Waldo]]

Better than |node|, I went with |funcNode| so as to clarify just what node it is at various use sites (especially when |node| would be forwarded in a big garbage-list argument set and it'd be not clear just what it was).  Made it a separate patch-atop rather than touch the mega-patch here with that mostly cosmetic change.

I expect to land this pretty much Any Day Now, along with several other bugs and changes, as one dozen-rev landing, once I get two last reviews from :Yoric in another bug.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f31a702d5de
Rename Parser to GeneralParser, and move all partially-specialized (by ParseHandler) functions in GeneralParser into a new Parser class inheriting from GeneralParser.  This permits these functions to be specialized without having two identical definitions when CharT isn't solely char16_t.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/49a259667582
Change the name of some |Node pn| in function-parsing functions to |Node funcNode|.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdb40755d36a
Adjust AutoAwaitIsKeyword template parameters slightly so a full parser type need not be written out.  r=arai

Comment 8

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/9f31a702d5de
https://hg.mozilla.org/mozilla-central/rev/49a259667582
https://hg.mozilla.org/mozilla-central/rev/bdb40755d36a