Parser work

Parser work
* added new class to support redirecting factory constructors
* used new class to clean up instance creation expressions
* added cascade expressions in initializers
* added error detection
* re-worked parser to improve recovery
Committed: https://code.google.com/p/dart/source/detail?r=15417

[Brian Wilkerson, 2012-11-27 17:54:17]

Sorry about the size. Steve and Konstantin: I am most interested in your review
of the AST changes, though other comments are of course welcome.

[messick, 2012-11-27 18:51:25]

LGTM

This looks very similar to one of the (many) options we discussed yesterday, so
I don't have too many comments.

https://chromiumcodereview.appspot.com/11415153/diff/1/editor/tools/plugins/c...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/ConstructorDeclaration.java
(right):

https://chromiumcodereview.appspot.com/11415153/diff/1/editor/tools/plugins/c...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/ConstructorDeclaration.java:367:
if (externalKeyword != null) {
Using setters to modify the keywords could invalidate any assumptions made
because the parser won't construct instances with keywords out-of-order.

https://chromiumcodereview.appspot.com/11415153/diff/1/editor/tools/plugins/c...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/parser/ParserErrorCode.java
(right):

https://chromiumcodereview.appspot.com/11415153/diff/1/editor/tools/plugins/c...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/parser/ParserErrorCode.java:31:
ABSTRACT_TOP_LEVEL_VARIABLE("Top-level functions cannot be declared to be
'abstract'"),
functions->variables

https://chromiumcodereview.appspot.com/11415153/diff/1/editor/tools/plugins/c...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/parser/ParserErrorCode.java:71:
FINAL_CLASS("Classes cannot be declared to be 'final'"),
Thank goodness!

[scheglov, 2012-11-27 19:00:48]

LGTM

https://codereview.chromium.org/11415153/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/ConstructorDeclaration.java
(right):

https://codereview.chromium.org/11415153/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/ConstructorDeclaration.java:84:
private Token colon;
IIRC we though about more generic name.
It may be "=" in case of redirecting factory constructor.

https://codereview.chromium.org/11415153/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/ConstructorDeclaration.java:95:
private ConstructorName constructorName;
Probably 3 words is too much, but I've thought initially that this is actually
name of _this_ constructor.

[Brian Wilkerson, 2012-11-27 19:08:37]

https://chromiumcodereview.appspot.com/11415153/diff/1/editor/tools/plugins/c...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/ConstructorDeclaration.java
(right):

https://chromiumcodereview.appspot.com/11415153/diff/1/editor/tools/plugins/c...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/ConstructorDeclaration.java:367:
if (externalKeyword != null) {
> Using setters to modify the keywords could invalidate any assumptions made
> because the parser won't construct instances with keywords out-of-order.

I'm not sure I know what you mean, but I'll take a stab at responding. Yes,
having all these setters everywhere is dangerous, but I think we decided that we
want them anyway. No, the parser will construct instances with keywords out of
order, so I need to fix this method.

https://chromiumcodereview.appspot.com/11415153/diff/1/editor/tools/plugins/c...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/parser/ParserErrorCode.java
(right):

https://chromiumcodereview.appspot.com/11415153/diff/1/editor/tools/plugins/c...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/parser/ParserErrorCode.java:31:
ABSTRACT_TOP_LEVEL_VARIABLE("Top-level functions cannot be declared to be
'abstract'"),
Done

[Brian Wilkerson, 2012-11-27 19:16:13]

https://codereview.chromium.org/11415153/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/ConstructorDeclaration.java
(right):

https://codereview.chromium.org/11415153/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/ConstructorDeclaration.java:84:
private Token colon;
> IIRC we though about more generic name.

You're right. I missed it the first time around. Done.

https://codereview.chromium.org/11415153/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/ConstructorDeclaration.java:95:
private ConstructorName constructorName;
> Probably 3 words is too much, but I've thought initially that this is actually
> name of _this_ constructor.

How about "redirectedConstructor"?

[scheglov, 2012-11-27 19:26:55]

LGTM

https://codereview.chromium.org/11415153/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/ConstructorDeclaration.java
(right):

https://codereview.chromium.org/11415153/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/ConstructorDeclaration.java:95:
private ConstructorName constructorName;
On 2012/11/27 19:16:14, Brian Wilkerson wrote:
> > Probably 3 words is too much, but I've thought initially that this is
actually
> > name of _this_ constructor.
> 
> How about "redirectedConstructor"?

LGTM

[Brian Wilkerson, 2012-11-27 19:44:06]

https://codereview.chromium.org/11415153/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/ConstructorDeclaration.java
(right):

https://codereview.chromium.org/11415153/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/ConstructorDeclaration.java:95:
private ConstructorName constructorName;
> > How about "redirectedConstructor"?
> 
> LGTM

Done

[danrubel, 2012-11-27 21:25:13]

lgtm

https://codereview.chromium.org/11415153/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/ConstructorName.java
(right):

https://codereview.chromium.org/11415153/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/ConstructorName.java:71:
}
Will we ever be representing invalid code with this structure? In other words,
if name is null, will the "." token always be null? The current implementation
of Parser#parseConstructor appears to allow this case... in which case "."
should be returned as the end token.

[Brian Wilkerson, 2012-11-27 21:41:23]

https://codereview.chromium.org/11415153/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/ConstructorName.java
(right):

https://codereview.chromium.org/11415153/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/ConstructorName.java:71:
}
> Will we ever be representing invalid code with this structure?

Users can create invalid AST structures, but I haven't worried about that. The
parser, on the other hand will create a synthetic name if one doesn't appear in
the source, so no, the parser won't create an invalid structure.