Issue 839: Bad code leading to top level methods being something other than identifier

Issue 839: Bad code leading to top level methods being something other than identifier

Note; borrowed code from http://codereview.chromium.org/8914015/ as the
editor had the same issue.

Added support for the AST writer to handle Function Expressions and updated
the parser to give the AST writer a chance to dump the unit (if requested
via the current command line parameters) before bailing on the whole
program.


BUG=
TEST=

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

[Brian Wilkerson, 2011-12-14 23:41:01]

LGTM

[mmendez, 2011-12-15 15:02:19]

We need to go another round on this.  We can reproduce the problem if you simply
type the following code as a top-level declaration:

Foo.bar() {}

Assuming that the potential NPE caused by the AstWriter is addressed, the next
problem that you will find is that the
com.google.dart.compiler.resolver.MethodElementImplementation.fromMethodNode(MethodElementImplementation.java:136)
will CCE or assert if you have assertions turned on.

http://codereview.chromium.org/8913016/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/DartCompiler.java (right):

http://codereview.chromium.org/8913016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/DartCompiler.java:842:
astWriter.process(unit);
This will NPE if you don't have the writer stuff configured.

http://codereview.chromium.org/8913016/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/ast/LibraryUnit.java (right):

http://codereview.chromium.org/8913016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/ast/LibraryUnit.java:328:
topLevelNodes.put(extractName(node.getName()), node);
You could use node.getName().toSource() it is not exactly intended for this, but
it will produce a string and avoid the cast class.  Alternatively, you could
create a specific visitor that computes a name ONLY from a given node.

http://codereview.chromium.org/8913016/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java (right):

http://codereview.chromium.org/8913016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:345:
DartIdentifier name = node.getName();
Double check that you can't get a cast class here.

[codefu, 2011-12-15 16:23:59]

http://codereview.chromium.org/8913016/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/DartCompiler.java (right):

http://codereview.chromium.org/8913016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/DartCompiler.java:842:
astWriter.process(unit);
On 2011/12/15 15:02:20, mmendez wrote:
> This will NPE if you don't have the writer stuff configured.

Added a NullWriter class so this looks pretty straight forward.

http://codereview.chromium.org/8913016/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java (right):

http://codereview.chromium.org/8913016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:345:
DartIdentifier name = node.getName();
On 2011/12/15 15:02:20, mmendez wrote:
> Double check that you can't get a cast class here.

Done. DartFunctionExpression.getName() returns a stored identifier.

[mmendez, 2011-12-15 18:26:17]

LGTM with nits.  I added shauvik so he could weigh in on this as well.

http://codereview.chromium.org/8913016/diff/6001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/ast/LibraryUnit.java (right):

http://codereview.chromium.org/8913016/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/LibraryUnit.java:311:
topLevelNodes.put(node.getName().toSource(), node);
Nit: Please double check that this does make compilation speed any worse.

http://codereview.chromium.org/8913016/diff/6001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/parser/DartParser.java (right):

http://codereview.chromium.org/8913016/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/parser/DartParser.java:1118:
if(currentlyParsingTopleve()) {
Do you have a test for this new behavior?

http://codereview.chromium.org/8913016/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/parser/DartParser.java:3754: private
boolean currentlyParsingTopleve() {
Nit: currentlyParsingTopleve -> currentlyParsingToplevel

http://codereview.chromium.org/8913016/diff/6001/compiler/java/com/google/dar...
File
compiler/java/com/google/dart/compiler/resolver/MethodElementImplementation.java
(right):

http://codereview.chromium.org/8913016/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/MethodElementImplementation.java:140:
targetName = node.toSource();
Nit: add a comment here about why you are using node.toSource().  This maybe a
better pattern to use in the LibraryUnit.

[codefu, 2011-12-15 18:49:55]

http://codereview.chromium.org/8913016/diff/6001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/ast/LibraryUnit.java (right):

http://codereview.chromium.org/8913016/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/LibraryUnit.java:311:
topLevelNodes.put(node.getName().toSource(), node);
On 2011/12/15 18:26:18, mmendez wrote:
> Nit: Please double check that this does make compilation speed any worse.


Will change to if(is identifier)-else

http://codereview.chromium.org/8913016/diff/6001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/parser/DartParser.java (right):

http://codereview.chromium.org/8913016/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/parser/DartParser.java:1118:
if(currentlyParsingTopleve()) {
On 2011/12/15 18:26:18, mmendez wrote:
> Do you have a test for this new behavior?

Done.

http://codereview.chromium.org/8913016/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/parser/DartParser.java:3754: private
boolean currentlyParsingTopleve() {
On 2011/12/15 18:26:18, mmendez wrote:
> Nit: currentlyParsingTopleve -> currentlyParsingToplevel

Done.

http://codereview.chromium.org/8913016/diff/6001/compiler/java/com/google/dar...
File
compiler/java/com/google/dart/compiler/resolver/MethodElementImplementation.java
(right):

http://codereview.chromium.org/8913016/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/MethodElementImplementation.java:140:
targetName = node.toSource();
On 2011/12/15 18:26:18, mmendez wrote:
> Nit: add a comment here about why you are using node.toSource().  This maybe a
> better pattern to use in the LibraryUnit. 

Done.