Support Promise<T> syntax in the IDL parser.

Support Promise<T> syntax in the IDL parser.

The Web IDL spec specifies Promise<T> syntax. This CL makes the IDL parser
recognize the syntax.

BUG=421539
Committed: https://crrev.com/6ce2b8e162baa9573aaa1819ac610ef854cca9f8
Cr-Commit-Position: refs/heads/master@{#300255}

[yhirano, 2014-10-15 07:05:41]

yhirano@chromium.org changed reviewers:
+ jl@opera.com, noelallen@chromium.org

[Jens Widell, 2014-10-15 07:22:17]

LGTM

Let's wait for Noel to have a look too.

https://codereview.chromium.org/653343002/diff/1/tools/idl_parser/idl_lexer.py
File tools/idl_parser/idl_lexer.py (right):

https://codereview.chromium.org/653343002/diff/1/tools/idl_parser/idl_lexer.p...
tools/idl_parser/idl_lexer.py:97: 'Promise' : 'PROMISE',
Maybe add a removal of it to idl_ppapi_lexer.py as well? Though, to be honest, I
don't know how important that is, or if that lexer/parser combo is even used.

I'm sure Noel knows more about that.

https://codereview.chromium.org/653343002/diff/1/tools/idl_parser/idl_parser.py
File tools/idl_parser/idl_parser.py (right):

https://codereview.chromium.org/653343002/diff/1/tools/idl_parser/idl_parser....
tools/idl_parser/idl_parser.py:905: # [97]
Supposed to be [87]. Looks like a lot of these production numbers are wrong now,
though. Renumber and reorder?

Keeping the numbers synced could be a good idea since it (often) gives you a
rather quick way to check if and where we're missing some update in the spec.

Also, extensions/non-compliances are typically summarized in this comment rather
than (just) in the implementation. So make it "[87] Add unqualified 'Promise'"
or something such?

[noelallen1, 2014-10-15 18:12:32]

I would have expected your test to fail.  In any case, it's not complete in that
you want to keep the type of promise for the specified case.

https://codereview.chromium.org/653343002/diff/1/tools/idl_parser/idl_lexer.py
File tools/idl_parser/idl_lexer.py (right):

https://codereview.chromium.org/653343002/diff/1/tools/idl_parser/idl_lexer.p...
tools/idl_parser/idl_lexer.py:97: 'Promise' : 'PROMISE',
On 2014/10/15 07:22:17, Jens Widell wrote:
> Maybe add a removal of it to idl_ppapi_lexer.py as well? Though, to be honest,
I
> don't know how important that is, or if that lexer/parser combo is even used.
> 
> I'm sure Noel knows more about that.

Not sure what you mean by removal.

https://codereview.chromium.org/653343002/diff/1/tools/idl_parser/idl_parser.py
File tools/idl_parser/idl_parser.py (right):

https://codereview.chromium.org/653343002/diff/1/tools/idl_parser/idl_parser....
tools/idl_parser/idl_parser.py:905: # [97]
On 2014/10/15 07:22:17, Jens Widell wrote:
> Supposed to be [87]. Looks like a lot of these production numbers are wrong
now,
> though. Renumber and reorder?
> 
> Keeping the numbers synced could be a good idea since it (often) gives you a
> rather quick way to check if and where we're missing some update in the spec.
> 
> Also, extensions/non-compliances are typically summarized in this comment
rather
> than (just) in the implementation. So make it "[87] Add unqualified 'Promise'"
> or something such?

Agreed to both.  If you change to a different version of the spec, then please
update the numbers and change the link above so we can have a consistent
reference.  Also, a comment above helps us find this:
# [87] Add unqualified Promise

Next time someone updates the spec, having these notes helps us verify the
feature is still an exception or perhaps it has made it into the spec in a
slightly different form.

https://codereview.chromium.org/653343002/diff/1/tools/idl_parser/test_parser...
File tools/idl_parser/test_parser/interface_web.idl (right):

https://codereview.chromium.org/653343002/diff/1/tools/idl_parser/test_parser...
tools/idl_parser/test_parser/interface_web.idl:190: *      Promise(Promise)
Shouldn't this be Promise(void) or
    Promise()
       PrimitiveType(void)

[Jens Widell, 2014-10-15 18:16:47]

https://codereview.chromium.org/653343002/diff/1/tools/idl_parser/idl_lexer.py
File tools/idl_parser/idl_lexer.py (right):

https://codereview.chromium.org/653343002/diff/1/tools/idl_parser/idl_lexer.p...
tools/idl_parser/idl_lexer.py:97: 'Promise' : 'PROMISE',
On 2014/10/15 18:12:32, noelallen1 wrote:
> On 2014/10/15 07:22:17, Jens Widell wrote:
> > Maybe add a removal of it to idl_ppapi_lexer.py as well? Though, to be
honest,
> I
> > don't know how important that is, or if that lexer/parser combo is even
used.
> > 
> > I'm sure Noel knows more about that.
> 
> Not sure what you mean by removal.

A bunch of keywords are deleted in IDLPPAPILexer's constructor, with the comment
"Remove JS types". If promises are considered a "JS type", then it might make
sense to remove it too.

But I don't know. I'm sure you do. :-)

[noelallen1, 2014-10-15 19:42:03]

Gotcha...

Yes, PPAPI use PPAPI types, not JS types.

So if you do remove 'Promise' from the PPAPI Lexicon, then you will need to stub
out the appropriate parse definitions in PPAPI Parser as well by using:

  def p_PromiseType(self, p):
    """ """
    pass

Otherwise it will complain about an undefined type.

[yhirano, 2014-10-16 05:25:47]

https://codereview.chromium.org/653343002/diff/1/tools/idl_parser/idl_lexer.py
File tools/idl_parser/idl_lexer.py (right):

https://codereview.chromium.org/653343002/diff/1/tools/idl_parser/idl_lexer.p...
tools/idl_parser/idl_lexer.py:97: 'Promise' : 'PROMISE',
On 2014/10/15 18:16:47, Jens Widell wrote:
> On 2014/10/15 18:12:32, noelallen1 wrote:
> > On 2014/10/15 07:22:17, Jens Widell wrote:
> > > Maybe add a removal of it to idl_ppapi_lexer.py as well? Though, to be
> honest,
> > I
> > > don't know how important that is, or if that lexer/parser combo is even
> used.
> > > 
> > > I'm sure Noel knows more about that.
> > 
> > Not sure what you mean by removal.
> 
> A bunch of keywords are deleted in IDLPPAPILexer's constructor, with the
comment
> "Remove JS types". If promises are considered a "JS type", then it might make
> sense to remove it too.
> 
> But I don't know. I'm sure you do. :-)

Done.

https://codereview.chromium.org/653343002/diff/1/tools/idl_parser/idl_parser.py
File tools/idl_parser/idl_parser.py (right):

https://codereview.chromium.org/653343002/diff/1/tools/idl_parser/idl_parser....
tools/idl_parser/idl_parser.py:905: # [97]
On 2014/10/15 18:12:32, noelallen1 wrote:
> On 2014/10/15 07:22:17, Jens Widell wrote:
> > Supposed to be [87]. Looks like a lot of these production numbers are wrong
> now,
> > though. Renumber and reorder?
> > 
> > Keeping the numbers synced could be a good idea since it (often) gives you a
> > rather quick way to check if and where we're missing some update in the
spec.
> > 
> > Also, extensions/non-compliances are typically summarized in this comment
> rather
> > than (just) in the implementation. So make it "[87] Add unqualified
'Promise'"
> > or something such?
> 
> Agreed to both.  If you change to a different version of the spec, then please
> update the numbers and change the link above so we can have a consistent
> reference.  Also, a comment above helps us find this:
> # [87] Add unqualified Promise
> 
> Next time someone updates the spec, having these notes helps us verify the
> feature is still an exception or perhaps it has made it into the spec in a
> slightly different form.
> 
> 
Done. I removed numbers from idl_ppapi_parser.py, because they seem useless.

https://codereview.chromium.org/653343002/diff/1/tools/idl_parser/test_parser...
File tools/idl_parser/test_parser/interface_web.idl (right):

https://codereview.chromium.org/653343002/diff/1/tools/idl_parser/test_parser...
tools/idl_parser/test_parser/interface_web.idl:190: *      Promise(Promise)
On 2014/10/15 18:12:32, noelallen1 wrote:
> Shouldn't this be Promise(void) or
>     Promise()
>        PrimitiveType(void)

Done.

[noelallen1, 2014-10-16 17:03:05]

We just need to figure out why the use of Promise in the PPAPI test doesn't
cause an error after DelKeyWords(..., 'Promise', ...) and how it can create a
Typeref for it.

https://codereview.chromium.org/653343002/diff/40001/tools/idl_parser/test_pa...
File tools/idl_parser/test_parser/struct_ppapi.idl (right):

https://codereview.chromium.org/653343002/diff/40001/tools/idl_parser/test_pa...
tools/idl_parser/test_parser/struct_ppapi.idl:55: Promise z;
Didn't you remove Promise from PPAPI parser/lexer?
This should be an error.

[Jens Widell, 2014-10-16 17:08:38]

https://codereview.chromium.org/653343002/diff/40001/tools/idl_parser/test_pa...
File tools/idl_parser/test_parser/struct_ppapi.idl (right):

https://codereview.chromium.org/653343002/diff/40001/tools/idl_parser/test_pa...
tools/idl_parser/test_parser/struct_ppapi.idl:55: Promise z;
On 2014/10/16 17:03:05, noelallen1 wrote:
> Didn't you remove Promise from PPAPI parser/lexer?
> This should be an error.

Since it's not a keyword it's just another identifier. Why would this be an
error?

[noelallen1, 2014-10-16 18:14:02]

https://codereview.chromium.org/653343002/diff/40001/tools/idl_parser/test_pa...
File tools/idl_parser/test_parser/struct_ppapi.idl (right):

https://codereview.chromium.org/653343002/diff/40001/tools/idl_parser/test_pa...
tools/idl_parser/test_parser/struct_ppapi.idl:55: Promise z;
On 2014/10/16 17:08:38, Jens Widell wrote:
> On 2014/10/16 17:03:05, noelallen1 wrote:
> > Didn't you remove Promise from PPAPI parser/lexer?
> > This should be an error.
> 
> Since it's not a keyword it's just another identifier. Why would this be an
> error?

We should expect a type here.

def p_StructMember(self, p):
  """StructMember : ExtendedAttributeList Type identifier ';'"""

Promise an identifier.  It is neither a Type or Extended Attribute.  While
extended attributes list can be empty, this should fail to parse because Type is
required.

[Jens Widell, 2014-10-16 18:21:42]

https://codereview.chromium.org/653343002/diff/40001/tools/idl_parser/test_pa...
File tools/idl_parser/test_parser/struct_ppapi.idl (right):

https://codereview.chromium.org/653343002/diff/40001/tools/idl_parser/test_pa...
tools/idl_parser/test_parser/struct_ppapi.idl:55: Promise z;
On 2014/10/16 18:14:02, noelallen1 wrote:
> On 2014/10/16 17:08:38, Jens Widell wrote:
> > On 2014/10/16 17:03:05, noelallen1 wrote:
> > > Didn't you remove Promise from PPAPI parser/lexer?
> > > This should be an error.
> > 
> > Since it's not a keyword it's just another identifier. Why would this be an
> > error?
> 
> We should expect a type here.
> 
> def p_StructMember(self, p):
>   """StructMember : ExtendedAttributeList Type identifier ';'"""
> 
> Promise an identifier.  It is neither a Type or Extended Attribute.  While
> extended attributes list can be empty, this should fail to parse because Type
is
> required.
> 

But Type references NotAnyType (indirectly) which can be "identifier
TypeSuffix".

[noelallen1, 2014-10-16 18:36:26]

lgtm

https://codereview.chromium.org/653343002/diff/40001/tools/idl_parser/test_pa...
File tools/idl_parser/test_parser/struct_ppapi.idl (right):

https://codereview.chromium.org/653343002/diff/40001/tools/idl_parser/test_pa...
tools/idl_parser/test_parser/struct_ppapi.idl:55: Promise z;
On 2014/10/16 18:21:42, Jens Widell wrote:
> On 2014/10/16 18:14:02, noelallen1 wrote:
> > On 2014/10/16 17:08:38, Jens Widell wrote:
> > > On 2014/10/16 17:03:05, noelallen1 wrote:
> > > > Didn't you remove Promise from PPAPI parser/lexer?
> > > > This should be an error.
> > > 
> > > Since it's not a keyword it's just another identifier. Why would this be
an
> > > error?
> > 
> > We should expect a type here.
> > 
> > def p_StructMember(self, p):
> >   """StructMember : ExtendedAttributeList Type identifier ';'"""
> > 
> > Promise an identifier.  It is neither a Type or Extended Attribute.  While
> > extended attributes list can be empty, this should fail to parse because
Type
> is
> > required.
> > 
> 
> But Type references NotAnyType (indirectly) which can be "identifier
> TypeSuffix".

You are correct.  

Okay, I agree this is legal under the assumption that once the AST is created,
whatever is using it will error out if in fact 'Promise' has not been defined
via a typedef or some other form.

[yhirano, 2014-10-20 11:49:02]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-10-20 11:49:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/653343002/40001

[commit-bot: I haz the power, 2014-10-20 12:49:18]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-10-20 12:50:03]

Patchset 3 (id:??) landed as
https://crrev.com/6ce2b8e162baa9573aaa1819ac610ef854cca9f8
Cr-Commit-Position: refs/heads/master@{#300255}