Using annotations to help with parser recovery

Using annotations to help with parser recovery

There are cases in the parser where a method is consuming tokens aren't
expected at that level, but could have been used to sucessfully complete
a non-terminal further up in the stack.

This change adds an annotation @Terminals that is consulted when 
an unexpected token is encountered to determine whether or not the
token should be consumed.  Adding a token to @Terminals tells the
parser it is OK not to consume that token, that the parser will eventually
make progress.

http://code.google.com/p/dart/issues/detail?id=2417

Committed: https://code.google.com/p/dart/source/detail?r=6299

[zundel, 2012-04-06 17:10:20]

No unit tests yet, but this does fix the problem mentioned in the issue:

foo(Selector.);

What do you guys think?  On the plus side, there is no penalty until you hit an
unexpected token.  On the minus side,  the call to
'Thread.currentThread().getStackTrace()' is expensive, and is called multiple
times when recovering from a single parse error as it makes its way up the
stack.

[messick, 2012-04-06 17:11:47]

Nice. DBC

https://chromiumcodereview.appspot.com/10005040/diff/1010/compiler/java/com/g...
File compiler/java/com/google/dart/compiler/parser/DartParser.java (right):

https://chromiumcodereview.appspot.com/10005040/diff/1010/compiler/java/com/g...
compiler/java/com/google/dart/compiler/parser/DartParser.java:256:
@Terminals(tokens={Token.EOS, Token.CLASS})
INTERFACE?

https://chromiumcodereview.appspot.com/10005040/diff/1010/lib/compiler/implem...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10005040/diff/1010/lib/compiler/implem...
lib/compiler/implementation/compiler.dart:113: Send fakeSend = new Send(null,
Selector.);
Is this intentional?

[scheglov, 2012-04-06 18:00:11]

LGTM

https://chromiumcodereview.appspot.com/10005040/diff/1010/compiler/java/com/g...
File compiler/java/com/google/dart/compiler/parser/AbstractParser.java (right):

https://chromiumcodereview.appspot.com/10005040/diff/1010/compiler/java/com/g...
compiler/java/com/google/dart/compiler/parser/AbstractParser.java:71:
classes.put(frame.getClassName(), null);
Hm... It was null and we put null again?

[Brian Wilkerson, 2012-04-06 18:09:23]

I like the approach, but I'm not thrilled about the implementation. (I generally
have that reaction when using reflection.) Unfortunately (for me) I can't think
of any other good implementation that doesn't incur a cost in the case of valid
input.

Unless you get an objection from someone else, I'd say LGTM.

[zundel, 2012-04-06 18:30:07]

https://chromiumcodereview.appspot.com/10005040/diff/1010/compiler/java/com/g...
File compiler/java/com/google/dart/compiler/parser/DartParser.java (right):

https://chromiumcodereview.appspot.com/10005040/diff/1010/compiler/java/com/g...
compiler/java/com/google/dart/compiler/parser/DartParser.java:256:
@Terminals(tokens={Token.EOS, Token.CLASS})
On 2012/04/06 17:11:47, messick wrote:
> INTERFACE?

This is an interesting problem.  INTERFACE isn't a token, its a "Builtin
Identifer", aka, pseudokeyword.  I have a special check for pseudokeywords
INTERFACE and TYPEDEF in some places.  I could try to extend this mechanism to
cover that case.

[Brian Wilkerson, 2012-04-06 18:57:08]

> This is an interesting problem.  INTERFACE isn't a token, its a "Builtin
> Identifer", aka, pseudokeyword.  I have a special check for pseudokeywords
> INTERFACE and TYPEDEF in some places.  I could try to extend this mechanism to
> cover that case.

In the new scanner it will be a keyword token. The parser will determine whether
it's being used where an identifier could be and treat it like an identifier (I
assume), but it would be available as a keyword for these purposes.

[danrubel, 2012-04-06 21:34:07]

Very clever approach, but I'm not comfortable with this implementation long
term. LGTM as an stop gap measure as we work on better implementation to solve
the same problem.

[messick, 2012-04-06 21:46:40]

On 2012/04/06 21:34:07, danrubel wrote:
> Very clever approach, but I'm not comfortable with this implementation long
> term. LGTM as an stop gap measure as we work on better implementation to solve
> the same problem.

Actually, could all those beginXXX() methods be used here? You'd have to record
the state in the parser, and therefore pay a cost even for successful parses.
This type of situation is what they are designed for, although I only expected
them to be used in code completion.