Fixes in codegen and test script

Fixes in codegen and test script

R=jmesserly@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/28c7ccb2fc1359df9973fb2565682216c899bb57

[Siggi Cherem (dart-lang), 2015-03-11 19:56:58]

Patchset #1 (id:1) has been deleted

[Siggi Cherem (dart-lang), 2015-03-11 19:57:46]

sigmund@google.com changed reviewers:
+ jmesserly@google.com

[Jennifer Messerly, 2015-03-11 20:28:55]

https://codereview.chromium.org/998043002/diff/20001/lib/src/codegen/js_codeg...
File lib/src/codegen/js_codegen.dart (left):

https://codereview.chromium.org/998043002/diff/20001/lib/src/codegen/js_codeg...
lib/src/codegen/js_codegen.dart:1540: body.add(js.statement('let # = #;',
[_visit(name), _catchParameter]));
hmmm. This is not going to have source location associated with the variable.
Maybe not a big deal. (maybe we should put it on the assignment statement
anyway)

Curious, what did this generate as? I'm worried if our visitSimpleIdentifier
logic isn't catching all of these cases.  If it's wrong here it would be wrong
in other cases (right?)... I think it started too simple -- Analyzer's visitors
tend to do more significant series of checks. Probably we should visit the
Element and make sure all of the cases are handled.

(another thing we should probably be using more is inSetterContext)

https://codereview.chromium.org/998043002/diff/20001/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/998043002/diff/20001/lib/src/utils.dart#newco...
lib/src/utils.dart:47: name = name.replaceAllMapped(new
RegExp(r'[^A-Za-z_$0-9]'), (c) => '_');
are we worried about collisions? would an escaping scheme be better? or fall
back to library's name?

https://codereview.chromium.org/998043002/diff/20001/lib/src/utils.dart#newco...
lib/src/utils.dart:49: return name;
do we need to exclude JS keywords if generating JS? (e.g. `switch.js`)

https://codereview.chromium.org/998043002/diff/20001/test/test.sh
File test/test.sh (right):

https://codereview.chromium.org/998043002/diff/20001/test/test.sh#newcode31
test/test.sh:31: "rm -r $expect/dev_compiler/ $expect/server_mode/dev_compiler/"
aww. Is there a way to exclude these from what we generate? Or have the test
clean up after itself?

(I usually just write the copy command, not copy paste it)

[Siggi Cherem (dart-lang), 2015-03-11 23:27:22]

Thanks for all the great comments. PTAL

https://codereview.chromium.org/998043002/diff/20001/lib/src/codegen/js_codeg...
File lib/src/codegen/js_codegen.dart (left):

https://codereview.chromium.org/998043002/diff/20001/lib/src/codegen/js_codeg...
lib/src/codegen/js_codegen.dart:1540: body.add(js.statement('let # = #;',
[_visit(name), _catchParameter]));
On 2015/03/11 20:28:54, John Messerly wrote:
> hmmm. This is not going to have source location associated with the variable.
> Maybe not a big deal. (maybe we should put it on the assignment statement
> anyway)
> 
Oh, I see.

> Curious, what did this generate as? I'm worried if our visitSimpleIdentifier
> logic isn't catching all of these cases.  If it's wrong here it would be wrong
> in other cases (right?)... I think it started too simple -- Analyzer's
visitors
> tend to do more significant series of checks. Probably we should visit the
> Element and make sure all of the cases are handled.
> 
> (another thing we should probably be using more is inSetterContext)

Here the problem was that visitSimpleIdentifier is assuming that identifiers are
expressions, so it was returning a VariableUse. The let statement expects a
VariableDeclaration or a name, so for example, here is the error you'd get from
js_ast with the regression test (`try_catch.dart`):


Interpolated value #0 is not a VariableDeclaration or String: VariableUse(e)
#0      InstantiatorGeneratorVisitor.error
(package:dev_compiler/src/js/template.dart:154:5)
#1     
InstantiatorGeneratorVisitor.visitInterpolatedVariableDeclaration.<anonymous
closure> (package:dev_compiler/src/js/template.dart:295:19)
#2      InstantiatorGeneratorVisitor.visitVariableInitialization.<anonymous
closure> (package:dev_compiler/src/js/template.dart:584:26)
#3      InstantiatorGeneratorVisitor.visitVariableDeclarationList.<anonymous
closure> (package:dev_compiler/src/js/template.dart:560:34)
#4      InstantiatorGeneratorVisitor.visitExpressionStatement.<anonymous
closure> (package:dev_compiler/src/js/template.dart:361:29)
#5      Template.instantiate (package:dev_compiler/src/js/template.dart:104:26)
#6      JsBuilder.statement (package:dev_compiler/src/js/builder.dart:224:32)
#7      JSCodegenVisitor.visitCatchClause
(package:dev_compiler/src/codegen/js_codegen.dart:1540:30)
#8      CatchClause.accept (package:analyzer/src/generated/ast.dart:4078:57)
#9      JSCodegenVisitor._visit
(package:dev_compiler/src/codegen/js_codegen.dart:1730:29)
#10     JSCodegenVisitor._visitList
(package:dev_compiler/src/codegen/js_codegen.dart:1743:46)
#11     JSCodegenVisitor._visitCatch
(package:dev_compiler/src/codegen/js_codegen.dart:1518:44)
#12     JSCodegenVisitor.visitTryStatement
(package:dev_compiler/src/codegen/js_codegen.dart:1497:53)
...


It seemed strange to change the behavior on visitSimpelIdentifier, so instead I
went ahead and created the node with the sourcespan info here.

https://codereview.chromium.org/998043002/diff/20001/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/998043002/diff/20001/lib/src/utils.dart#newco...
lib/src/utils.dart:47: name = name.replaceAllMapped(new
RegExp(r'[^A-Za-z_$0-9]'), (c) => '_');
On 2015/03/11 20:28:54, John Messerly wrote:
> are we worried about collisions? would an escaping scheme be better? or fall
> back to library's name?

Good point - went with an escaping scheme.

https://codereview.chromium.org/998043002/diff/20001/lib/src/utils.dart#newco...
lib/src/utils.dart:49: return name;
On 2015/03/11 20:28:54, John Messerly wrote:
> do we need to exclude JS keywords if generating JS? (e.g. `switch.js`)

Good point - I made isJsKeyword in js_ast public for this purpose and moved to a
separate library. Not sure if we are trying to minimize changes there though.

https://codereview.chromium.org/998043002/diff/20001/test/test.sh
File test/test.sh (right):

https://codereview.chromium.org/998043002/diff/20001/test/test.sh#newcode31
test/test.sh:31: "rm -r $expect/dev_compiler/ $expect/server_mode/dev_compiler/"
On 2015/03/11 20:28:55, John Messerly wrote:
> aww. Is there a way to exclude these from what we generate? Or have the test
> clean up after itself?
> 
> (I usually just write the copy command, not copy paste it)

yeah, good point. I just realized, we could add these two folders to the
.gitignore, so we can continue using the simple copy command and not worry about
it. WDYT?

[Jennifer Messerly, 2015-03-11 23:48:01]

https://codereview.chromium.org/998043002/diff/20001/lib/src/codegen/js_codeg...
File lib/src/codegen/js_codegen.dart (left):

https://codereview.chromium.org/998043002/diff/20001/lib/src/codegen/js_codeg...
lib/src/codegen/js_codegen.dart:1540: body.add(js.statement('let # = #;',
[_visit(name), _catchParameter]));
On 2015/03/11 23:27:22, Siggi Cherem (dart-lang) wrote:
> On 2015/03/11 20:28:54, John Messerly wrote:
> > hmmm. This is not going to have source location associated with the
variable.
> > Maybe not a big deal. (maybe we should put it on the assignment statement
> > anyway)
> > 
> Oh, I see.
> 
> > Curious, what did this generate as? I'm worried if our visitSimpleIdentifier
> > logic isn't catching all of these cases.  If it's wrong here it would be
wrong
> > in other cases (right?)... I think it started too simple -- Analyzer's
> visitors
> > tend to do more significant series of checks. Probably we should visit the
> > Element and make sure all of the cases are handled.
> > 
> > (another thing we should probably be using more is inSetterContext)
> 
> Here the problem was that visitSimpleIdentifier is assuming that identifiers
are
> expressions, so it was returning a VariableUse. The let statement expects a
> VariableDeclaration or a name, so for example, here is the error you'd get
from
> js_ast with the regression test (`try_catch.dart`):
> 
> 
> Interpolated value #0 is not a VariableDeclaration or String: VariableUse(e)
> #0      InstantiatorGeneratorVisitor.error
> (package:dev_compiler/src/js/template.dart:154:5)
> #1     
> InstantiatorGeneratorVisitor.visitInterpolatedVariableDeclaration.<anonymous
> closure> (package:dev_compiler/src/js/template.dart:295:19)
> #2      InstantiatorGeneratorVisitor.visitVariableInitialization.<anonymous
> closure> (package:dev_compiler/src/js/template.dart:584:26)
> #3      InstantiatorGeneratorVisitor.visitVariableDeclarationList.<anonymous
> closure> (package:dev_compiler/src/js/template.dart:560:34)
> #4      InstantiatorGeneratorVisitor.visitExpressionStatement.<anonymous
> closure> (package:dev_compiler/src/js/template.dart:361:29)
> #5      Template.instantiate
(package:dev_compiler/src/js/template.dart:104:26)
> #6      JsBuilder.statement (package:dev_compiler/src/js/builder.dart:224:32)
> #7      JSCodegenVisitor.visitCatchClause
> (package:dev_compiler/src/codegen/js_codegen.dart:1540:30)
> #8      CatchClause.accept (package:analyzer/src/generated/ast.dart:4078:57)
> #9      JSCodegenVisitor._visit
> (package:dev_compiler/src/codegen/js_codegen.dart:1730:29)
> #10     JSCodegenVisitor._visitList
> (package:dev_compiler/src/codegen/js_codegen.dart:1743:46)
> #11     JSCodegenVisitor._visitCatch
> (package:dev_compiler/src/codegen/js_codegen.dart:1518:44)
> #12     JSCodegenVisitor.visitTryStatement
> (package:dev_compiler/src/codegen/js_codegen.dart:1497:53)
> ...
> 
> 
> It seemed strange to change the behavior on visitSimpelIdentifier, so instead
I
> went ahead and created the node with the sourcespan info here.

ouch, yeah, it seems like my Template.visitInterpolatedVariableDeclaration is
not general enough to handle that case :|

I'd probably put the blame here on the JS AST structure not quite being what we
want. But yeah, your new code is fine to do, and it's the same workaround we do
elsewhere.

https://codereview.chromium.org/998043002/diff/20001/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/998043002/diff/20001/lib/src/utils.dart#newco...
lib/src/utils.dart:49: return name;
On 2015/03/11 23:27:22, Siggi Cherem (dart-lang) wrote:
> On 2015/03/11 20:28:54, John Messerly wrote:
> > do we need to exclude JS keywords if generating JS? (e.g. `switch.js`)
> 
> Good point - I made isJsKeyword in js_ast public for this purpose and moved to
a
> separate library. Not sure if we are trying to minimize changes there though.

Generally I've been sticking to adds. But that was a method I added, so no
worries about moving it :)

https://codereview.chromium.org/998043002/diff/20001/test/test.sh
File test/test.sh (right):

https://codereview.chromium.org/998043002/diff/20001/test/test.sh#newcode31
test/test.sh:31: "rm -r $expect/dev_compiler/ $expect/server_mode/dev_compiler/"
On 2015/03/11 23:27:22, Siggi Cherem (dart-lang) wrote:
> On 2015/03/11 20:28:55, John Messerly wrote:
> > aww. Is there a way to exclude these from what we generate? Or have the test
> > clean up after itself?
> > 
> > (I usually just write the copy command, not copy paste it)
> 
> yeah, good point. I just realized, we could add these two folders to the
> .gitignore, so we can continue using the simple copy command and not worry
about
> it. WDYT?

As long as they're excluded from the `diff` command, it sounds good to me.
(I presume we want to check in everything expected, so running tests on clean
checkout shows PASS)

https://codereview.chromium.org/998043002/diff/40001/test/test.sh
File test/test.sh (right):

https://codereview.chromium.org/998043002/diff/40001/test/test.sh#newcode29
test/test.sh:29: echo "$ cp -a test/codegen/actual/* test/codegen/expect"
This doesn't work if you're standing in a folder besides root. This script is
meant to work from anywhere, hence changed on purpose in a past revision to use
`pwd`. I'm fine with interpolating the `pwd` here if you want to avoid
pushd/popd.

[Jennifer Messerly, 2015-03-11 23:48:25]

also, besides test.sh LGTM :)

[Siggi Cherem (dart-lang), 2015-03-12 00:16:11]

Committed patchset #2 (id:40001) manually as
28c7ccb2fc1359df9973fb2565682216c899bb57 (presubmit successful).

[Siggi Cherem (dart-lang), 2015-03-12 00:16:26]

thanks!

https://codereview.chromium.org/998043002/diff/40001/test/test.sh
File test/test.sh (right):

https://codereview.chromium.org/998043002/diff/40001/test/test.sh#newcode29
test/test.sh:29: echo "$ cp -a test/codegen/actual/* test/codegen/expect"
On 2015/03/11 23:48:00, John Messerly wrote:
> This doesn't work if you're standing in a folder besides root. This script is
> meant to work from anywhere, hence changed on purpose in a past revision to
use
> `pwd`. I'm fine with interpolating the `pwd` here if you want to avoid
> pushd/popd.

Ah, I see, never mind, I somehow missed that. reverted