implement opassign, fix bugs in pre/postfix, introduce a let* helper

implement opassign, fix bugs in pre/postfix, introduce a let* helper

Simplifies js_codegen to remove the special cases for things like cascades and statement parent, instead these "fall out" of JSMetaLet node and the various to* methods in js_ast. As a result of handling things more uniformly, code gets cleaner in a lot of cases. One slight "regression" is a cascade at the end of a variable init list with multiple variables. But multiple variables aren't very common so doesn't seem worth optimizing readability there.

R=leafp@google.com, vsm@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/0c480a7d4b518d5f2ce5d3afbe706937299a0ff2

[Jennifer Messerly, 2015-04-07 21:50:16]

Patchset #1 (id:1) has been deleted

[Jennifer Messerly, 2015-04-07 22:00:28]

jmesserly@google.com changed reviewers:
+ jacobr@google.com, leafp@google.com

[Jennifer Messerly, 2015-04-07 22:00:29]

Thanks to Leaf & Vijay for the meta-let* brainstorm :)

https://codereview.chromium.org/1069493002/diff/20001/lib/runtime/dart/_inter...
File lib/runtime/dart/_internal.js (right):

https://codereview.chromium.org/1069493002/diff/20001/lib/runtime/dart/_inter...
lib/runtime/dart/_internal.js:301: let result = growable ? ((_) => (_ = new
(core.List$(E))(), _[core.$length] = length, _))() : new
(core.List$(E))(length);
hmm, we could just emit this into multiple lines always.

https://codereview.chromium.org/1069493002/diff/20001/lib/src/codegen/js_meta...
File lib/src/codegen/js_metalet.dart (right):

https://codereview.chromium.org/1069493002/diff/20001/lib/src/codegen/js_meta...
lib/src/codegen/js_metalet.dart:207: // If the identifier was used elsewhere,
bail, because we're going to change
oops, long line.

https://codereview.chromium.org/1069493002/diff/20001/lib/src/js/nodes.dart
File lib/src/js/nodes.dart (right):

https://codereview.chromium.org/1069493002/diff/20001/lib/src/js/nodes.dart#n...
lib/src/js/nodes.dart:229: // For debugging
it was hard to read js_ast nodes in debugger, compared to analyzer nodes which
were really easy

https://codereview.chromium.org/1069493002/diff/20001/test/codegen/expect/cas...
File test/codegen/expect/cascade.js (right):

https://codereview.chromium.org/1069493002/diff/20001/test/codegen/expect/cas...
test/codegen/expect/cascade.js:58: let a = 42, b = ((_) => {
this is the one case that's now worse (multiple variables, last one used to be
treated specially)

[Jennifer Messerly, 2015-04-07 22:09:53]

on a related note, I forgot to fix printing of a single argument lambda. will do
that too, either here or in a follow up.

Here's the pull request version if you're having problems with Rietveld:
https://github.com/dart-lang/dev_compiler/pull/126

https://codereview.chromium.org/1069493002/diff/20001/lib/src/codegen/js_meta...
File lib/src/codegen/js_metalet.dart (right):

https://codereview.chromium.org/1069493002/diff/20001/lib/src/codegen/js_meta...
lib/src/codegen/js_metalet.dart:94: // Assign parameters inside the body, to get
sequential let* behavior
for a single variable we could emit:

     (x => x*x)(123)

instead of:

    (x => (x = 123, x*x))()

thoughts?

anyway, it's kind of neat how easy it is to tweak these heuristics now and have
it apply everywhere :D

[Leaf, 2015-04-08 04:08:35]

Nice!  Some questions and comments, some of which may just be my unfamiliarity
with the code generator, but basically lgtm.

https://codereview.chromium.org/1069493002/diff/20001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1069493002/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1171: var vars = {};
Why does order not matter for these bindings?  Or are you able to recover order
of insertion?

https://codereview.chromium.org/1069493002/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1687: ///
Probably worth mentioning that the first parameter gets mutated with the
additional bindings.

https://codereview.chromium.org/1069493002/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1695: Expression _captureLeftHandSide(
The "captureXXX" naming convention surprised me (but maybe I'm just missing the
context for it).  Maybe "_nameLeftHandSide"?  Or "_bindLeftHandSide"?

https://codereview.chromium.org/1069493002/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1715: return expr;
When can this happen and why is this right?  Is this always a simpleidentifier?

https://codereview.chromium.org/1069493002/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1726: Expression _captureValue(
As before, consider other names?

https://codereview.chromium.org/1069493002/diff/20001/lib/src/codegen/js_meta...
File lib/src/codegen/js_metalet.dart (right):

https://codereview.chromium.org/1069493002/diff/20001/lib/src/codegen/js_meta...
lib/src/codegen/js_metalet.dart:18: ///     (let* (x1=expr1, x2=expr2,
t=expr1[expr2]) { x1[x2] = t + 1; t })
t=x1[x2] ?

https://codereview.chromium.org/1069493002/diff/20001/lib/src/codegen/js_meta...
lib/src/codegen/js_metalet.dart:94: // Assign parameters inside the body, to get
sequential let* behavior
On 2015/04/07 22:09:52, John Messerly wrote:
> for a single variable we could emit:
> 
>      (x => x*x)(123)
> 
> instead of:
> 
>     (x => (x = 123, x*x))()
> 
> thoughts?
> 
> anyway, it's kind of neat how easy it is to tweak these heuristics now and
have
> it apply everywhere :D

I like the former, but I suspect it's because I'm used to the pattern.  The
latter is probably more accessible.

https://codereview.chromium.org/1069493002/diff/20001/lib/src/codegen/js_meta...
lib/src/codegen/js_metalet.dart:110: if (_statement != null) return _statement;
I didn't follow the need for caching.  Do these really get emitted multiple
times?

https://codereview.chromium.org/1069493002/diff/20001/lib/src/js/nodes.dart
File lib/src/js/nodes.dart (right):

https://codereview.chromium.org/1069493002/diff/20001/lib/src/js/nodes.dart#n...
lib/src/js/nodes.dart:772: if (!isCommaOperator) return
super.toVoidExpression();
It almost feels like it would be worth having a CommaOperator subclass of
Binary.  WDYT?

https://codereview.chromium.org/1069493002/diff/20001/test/codegen/expect/cas...
File test/codegen/expect/cascade.js (right):

https://codereview.chromium.org/1069493002/diff/20001/test/codegen/expect/cas...
test/codegen/expect/cascade.js:16: let _ = a;
That's really nice.

[Jennifer Messerly, 2015-04-08 16:35:45]

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

[Jennifer Messerly, 2015-04-08 16:35:46]

Thanks Leaf! Based on discussions yesterday, I uploaded two variants: one
addressing your comments, and then a follow up to that that converts it to use
statement lambda form. That results in less long lines, and maybe is a bit more
clear. It doesn't address being able to lift the bindings fully out of the
expression, though. I can write up that variant too if interested.

To be honest, looking at the examples, it looks like nullability and postfix is
the cause of pretty much all of the ugliness, it might be better to add special
cases, e.g. postfix null check could be just:    `dart.notNull(obj.prop++)`.

prefix is harder... maybe could build on postfix:
`dart.notNull(obj.prop++) + 1`

Opassign is harder too. We could use wrappers:

    dart.prop(obj, 'prop').notNull += 42
    dart.index(obj, 123).notNull += 42

(Cost might be prohibitive though)

https://codereview.chromium.org/1069493002/diff/20001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1069493002/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1171: var vars = {};
On 2015/04/08 04:08:34, leafp wrote:
> Why does order not matter for these bindings?  Or are you able to recover
order
> of insertion?

{} (and `new Map()`) makes a LinkedHashMap which preserves order. IIRC it was
chosen as a safer default. I try to create a new HashMap explicitly when order
does not matter (it's faster).

https://codereview.chromium.org/1069493002/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1687: ///
On 2015/04/08 04:08:34, leafp wrote:
> Probably worth mentioning that the first parameter gets mutated with the
> additional bindings. 

sounds good. To be honest, I hate mutating arguments but it can be hard to
avoid. WDYT about creating a builder object to encapsulate? I originally had
one, but wasn't sure if it's overkill or not.

https://codereview.chromium.org/1069493002/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1695: Expression _captureLeftHandSide(
On 2015/04/08 04:08:34, leafp wrote:
> The "captureXXX" naming convention surprised me (but maybe I'm just missing
the
> context for it).  Maybe "_nameLeftHandSide"?  Or "_bindLeftHandSide"?

yeah, I struggled to find a good name. "bind" works for me

https://codereview.chromium.org/1069493002/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1715: return expr;
On 2015/04/08 04:08:34, leafp wrote:
> When can this happen and why is this right?  Is this always a
simpleidentifier?

Yeah, it's always a SimpleIdentifier, I'll add a cast to make that more obvious.
Section 16.31 Assignable Expressions in the spec.
In Analyzer's AST this is isAssignable:
https://code.google.com/p/dart/source/browse/trunk/dart/pkg/analyzer/lib/src/...

which is true only for Identifier, IndexExpression, and PropertyAccess (and
their subtypes) which is LibraryIdentifier, PrefixIdentifier and
SimpleIdentifier.

https://codereview.chromium.org/1069493002/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1726: Expression _captureValue(
On 2015/04/08 04:08:34, leafp wrote:
> As before, consider other names?

Done.

https://codereview.chromium.org/1069493002/diff/20001/lib/src/codegen/js_meta...
File lib/src/codegen/js_metalet.dart (right):

https://codereview.chromium.org/1069493002/diff/20001/lib/src/codegen/js_meta...
lib/src/codegen/js_metalet.dart:18: ///     (let* (x1=expr1, x2=expr2,
t=expr1[expr2]) { x1[x2] = t + 1; t })
On 2015/04/08 04:08:34, leafp wrote:
> t=x1[x2] ?

Good catch! Yes :)

https://codereview.chromium.org/1069493002/diff/20001/lib/src/codegen/js_meta...
lib/src/codegen/js_metalet.dart:110: if (_statement != null) return _statement;
On 2015/04/08 04:08:34, leafp wrote:
> I didn't follow the need for caching.  Do these really get emitted multiple
> times?

That's fair. It's only really needed for toExpression(), because we do that
implicitly whenever the tree is walked. I'll add a comment.

https://codereview.chromium.org/1069493002/diff/20001/lib/src/js/nodes.dart
File lib/src/js/nodes.dart (right):

https://codereview.chromium.org/1069493002/diff/20001/lib/src/js/nodes.dart#n...
lib/src/js/nodes.dart:772: if (!isCommaOperator) return
super.toVoidExpression();
On 2015/04/08 04:08:34, leafp wrote:
> It almost feels like it would be worth having a CommaOperator subclass of
> Binary.  WDYT?

Yeah, I agree. So far I was trying to maintain compatibility-ish with js_ast in
dart2js, but maybe not worth it. It definitely constrains the design. Hmmm,
maybe I could make the Binary constructor be a factory, that creates Comma
subclass when op == ','

https://codereview.chromium.org/1069493002/diff/20001/test/codegen/expect/cas...
File test/codegen/expect/cascade.js (right):

https://codereview.chromium.org/1069493002/diff/20001/test/codegen/expect/cas...
test/codegen/expect/cascade.js:16: let _ = a;
On 2015/04/08 04:08:34, leafp wrote:
> That's really nice.

yeah, there were a few "emergent" cases like this where I felt like the compiler
was outsmarting me =). Actually thought it was a bug at first before noticing
the assignment to a :)

[Jennifer Messerly, 2015-04-08 22:10:18]

BTW, I fixed some of the postfix issues with this change
https://codereview.chromium.org/1071773002 ... does that pattern seem worth
using?

[vsm, 2015-04-10 00:10:14]

lgtm

I think statement form variant is the easiest to grok.  The prop variant in the
other CL is cool, but magical / not so easy to grok on first glance.

[Jennifer Messerly, 2015-04-10 01:01:12]

On 2015/04/10 00:10:14, vsm wrote:
> lgtm
> 
> I think statement form variant is the easiest to grok.  The prop variant in
the
> other CL is cool, but magical / not so easy to grok on first glance.

Agreed. maybe we make this notNull business go away, then it won't be an issue
:)

[Jennifer Messerly, 2015-04-10 16:53:39]

Committed patchset #4 (id:80001) manually as
0c480a7d4b518d5f2ce5d3afbe706937299a0ff2 (presubmit successful).