Tag closures with their types

This CL implements tagging of functions and methods with function types.

For every class, we now generate a setSignature call which attaches properties to the constructor recording the method signatures, the static function signatures, and the names of all of the static methods. This call also attaches a getter to every static method which returns the type of the function.  Methods are only decorated with runtime types when torn off.  At a tear-off, the type is looked up in the constructor, and then attached to the bound function.

Top level functions and statement level functions get annotated with their type immediately after their declaration.  We could consider moving all of the top level function annotations to the end of the file, but for now I've left it inline.

Closures (function expressions) get wrapped in calls to a dart.fn helper, with type information attached in one of various forms.  This is currently the least attractive part of this CL.  We may want to iterate on the syntax for this.

I've added some support for NSM checking to the dsend/dcall case as well.

We may wish to iterate on the syntax, and on the runtime representation of types, but this should move us forward from a functionality standpoint.

BUG=
R=jmesserly@google.com, vsm@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/636b3401b62022f7ef563c720baaf1e200c6f8f1

[Leaf, 2015-05-15 22:46:20]

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

[vsm, 2015-05-18 17:35:11]

lgtm

General comment - mainly it'd be nice to figure out how to make this more terse
& perhaps readable.  Perhaps better to have a design discussion on it though.

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart_runti...
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart_runti...
lib/runtime/dart_runtime.js:100: if (ftype.checkApply(args)) return f.apply(obj,
args);
A TODO here to throw the right error?  If arity matches, I think this should be
a checked mode type error, not an NSM.

https://codereview.chromium.org/1138793002/diff/120001/lib/src/codegen/js_cod...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1138793002/diff/120001/lib/src/codegen/js_cod...
lib/src/codegen/js_codegen.dart:585: {
Add comment to this block ... e.g, // Emit signatures

https://codereview.chromium.org/1138793002/diff/120001/test/browser/runtime_t...
File test/browser/runtime_tests.js (right):

https://codereview.chromium.org/1138793002/diff/120001/test/browser/runtime_t...
test/browser/runtime_tests.js:128: setRuntimeType(bar1, functionType(A, [C,
String]));
Can we replace all 'setRuntimeType' with your new 'dart.fn' pattern?

https://codereview.chromium.org/1138793002/diff/120001/test/browser/runtime_t...
test/browser/runtime_tests.js:415: // This should throw but currently doesn't.
Nit: indent

https://codereview.chromium.org/1138793002/diff/120001/test/browser/runtime_t...
test/browser/runtime_tests.js:440: checkType(core.identical,
dart.functionType(core.bool, [core.Object, core.Object]));
line len

[Jennifer Messerly, 2015-05-18 17:37:06]

On 2015/05/18 17:35:11, vsm wrote:
> lgtm
> 
> General comment - mainly it'd be nice to figure out how to make this more
terse
> & perhaps readable.  Perhaps better to have a design discussion on it though.

I had the same thought, is it worth discussing here? It might be nice to put the
comments on the generated JS diff

[Jennifer Messerly, 2015-05-18 17:52:53]

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart/core.js
File lib/runtime/dart/core.js (right):

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart/core....
lib/runtime/dart/core.js:36: methods: () => ({
could this level be removed? e.g. 

dart.setSignature(Object, () => ({
  '==': ...
  toString: ...
  noSuchMethod: ...
}));

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart/core....
lib/runtime/dart/core.js:37: '==': dart.functionType(bool, [dart.dynamic]),
could we compute the function type later? the `dart.functionType` feels very
redundant.

'==': [dart.dynamic, bool]
toString: [String]
noSuchMethod: [Invocation, dart.dynamic]

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart/core....
lib/runtime/dart/core.js:110: statics: () => ({compare: dart.functionType(int,
[Comparable$(), Comparable$()])}),
do statics need to be separate from methods? for better or worse, Dart doesn't
allow the same name to be used as instance and static (within the same class)

if they did need to be separate, dart.setStaticSignature(...) would seem
cleaner. Less nesting == win

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart/core....
lib/runtime/dart/core.js:111: names: ['compare']
what does this do?

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart/core....
lib/runtime/dart/core.js:368: '==': dart.functionType(bool, [dart.dynamic]),
I'd be really curious to see this after above suggestions, I bet this would look
a lot simpler

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart/core....
lib/runtime/dart/core.js:955: methods: () => ({toString:
dart.functionType(String, [])})
can we avoid adding types to overrides when it just duplicates info from base
class?

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart/core....
lib/runtime/dart/core.js:1214: namedArguments.forEach(dart.fn((symbol, value) =>
{
this one seems not needed

https://codereview.chromium.org/1138793002/diff/120001/test/codegen/expect/co...
File test/codegen/expect/constructors.js (right):

https://codereview.chromium.org/1138793002/diff/120001/test/codegen/expect/co...
test/codegen/expect/constructors.js:6: dart.setSignature(A, {});
skip empty signatures?
not sure how often it comes up in practice. Maybe only for classes that just
have instance fields.

[Leaf, 2015-05-19 00:02:21]

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart/core.js
File lib/runtime/dart/core.js (right):

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart/core....
lib/runtime/dart/core.js:36: methods: () => ({
On 2015/05/18 17:52:52, John Messerly wrote:
> could this level be removed? e.g. 
> 
> dart.setSignature(Object, () => ({
>   '==': ...
>   toString: ...
>   noSuchMethod: ...
> }));

Certainly in the case that there are no statics, I could do so.  It makes it a
bit less uniform, but probably okay.  When there are statics, I either need to
pass them as a separate argument (which I didn't like) or mix them in, which is
ok but also not my favorite.  In the latter case I also need to pass the names
array as another argument.

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart/core....
lib/runtime/dart/core.js:37: '==': dart.functionType(bool, [dart.dynamic]),
On 2015/05/18 17:52:52, John Messerly wrote:
> could we compute the function type later? the `dart.functionType` feels very
> redundant.
> 
> '==': [dart.dynamic, bool]
> toString: [String]
> noSuchMethod: [Invocation, dart.dynamic]
> 

Something like this sounds like a good idea, might do in a follow up CL.

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart/core....
lib/runtime/dart/core.js:110: statics: () => ({compare: dart.functionType(int,
[Comparable$(), Comparable$()])}),
On 2015/05/18 17:52:52, John Messerly wrote:
> do statics need to be separate from methods? for better or worse, Dart doesn't
> allow the same name to be used as instance and static (within the same class)
> 
> if they did need to be separate, dart.setStaticSignature(...) would seem
> cleaner. Less nesting == win

They don't need to be.  I didn't really like mixing them in - it looked better
to me listed in their own sections.  I'm not wedded to separating them though.

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart/core....
lib/runtime/dart/core.js:111: names: ['compare']
On 2015/05/18 17:52:52, John Messerly wrote:
> what does this do?

The types in general need to be evaluated lazily.  The type data on the static
methods needs to be attached eagerly (at least, before any code that might do a
tear-off runs).  So given 

setSignature(Foo, { statics: () => {bar : dart.functionType(core.int, [])}})

I can't evaluate the closure bound to 'statics' while evaluating setSignature
(because core.int may not be defined yet).  But I need to know what names in Foo
are statics, and that information is under the lambda.  Currently I just pass in
the names separately (and redundantly) in the names array to work around this. 
I'd like a better solution - suggestions welcome.  I can inspect Foo to find the
names, but I was having trouble reliably teasing out which things were actual
static methods and which were other random properties on the class (e.g.
.prototype).  Other options might include moving the laziness under the
properties (generating statics : () => instead of () => {statics : ...), or
postponing the linking up of the metadata with the functions until after we know
everything is defined.

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart/core....
lib/runtime/dart/core.js:955: methods: () => ({toString:
dart.functionType(String, [])})
On 2015/05/18 17:52:52, John Messerly wrote:
> can we avoid adding types to overrides when it just duplicates info from base
> class?

Sure.  I'll do in a follow up CL.

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart/core....
lib/runtime/dart/core.js:1214: namedArguments.forEach(dart.fn((symbol, value) =>
{
On 2015/05/18 17:52:52, John Messerly wrote:
> this one seems not needed

You mean just assuming that any Function not marked with a type is of type
dynamic^n -> dynamic?  I think that's fine, but we need to be careful that we
can always tell whether we're in this case.  Currently I think we would classify
types (i.e. constructor functions) as dart functions if we did this.  If I add
an _runtimeType field to classes (which I'm planning on doing to make core.Type
work out right), then probably we could make this assumption again.  It does
leave us with no way to tell whether we have a javascript function or a dart
function, but that's probably ok.

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart_runti...
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart_runti...
lib/runtime/dart_runtime.js:100: if (ftype.checkApply(args)) return f.apply(obj,
args);
On 2015/05/18 17:35:10, vsm wrote:
> A TODO here to throw the right error?  If arity matches, I think this should
be
> a checked mode type error, not an NSM.

Done.

https://codereview.chromium.org/1138793002/diff/120001/lib/src/codegen/js_cod...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1138793002/diff/120001/lib/src/codegen/js_cod...
lib/src/codegen/js_codegen.dart:585: {
On 2015/05/18 17:35:10, vsm wrote:
> Add comment to this block ... e.g, // Emit signatures

Done.

https://codereview.chromium.org/1138793002/diff/120001/test/browser/runtime_t...
File test/browser/runtime_tests.js (right):

https://codereview.chromium.org/1138793002/diff/120001/test/browser/runtime_t...
test/browser/runtime_tests.js:128: setRuntimeType(bar1, functionType(A, [C,
String]));
On 2015/05/18 17:35:11, vsm wrote:
> Can we replace all 'setRuntimeType' with your new 'dart.fn' pattern?

Done.

https://codereview.chromium.org/1138793002/diff/120001/test/browser/runtime_t...
test/browser/runtime_tests.js:415: // This should throw but currently doesn't.
On 2015/05/18 17:35:10, vsm wrote:
> Nit: indent

Done.

https://codereview.chromium.org/1138793002/diff/120001/test/browser/runtime_t...
test/browser/runtime_tests.js:440: checkType(core.identical,
dart.functionType(core.bool, [core.Object, core.Object]));
On 2015/05/18 17:35:10, vsm wrote:
> line len

Done.

https://codereview.chromium.org/1138793002/diff/120001/test/codegen/expect/co...
File test/codegen/expect/constructors.js (right):

https://codereview.chromium.org/1138793002/diff/120001/test/codegen/expect/co...
test/codegen/expect/constructors.js:6: dart.setSignature(A, {});
On 2015/05/18 17:52:53, John Messerly wrote:
> skip empty signatures?
> not sure how often it comes up in practice. Maybe only for classes that just
> have instance fields.

Could do.  If I skip it, we can't assume that classes have signature attached to
them - we'd presumably assume that anything which doesn't have a signature but
we're treating as a class has an empty signature.  I was also planning to attach
the _runtimeType field during this call, but we could do that separately (or
better, maybe just put it as a static on Object?).

[Jennifer Messerly, 2015-05-19 16:32:50]

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart/core.js
File lib/runtime/dart/core.js (right):

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart/core....
lib/runtime/dart/core.js:111: names: ['compare']
On 2015/05/19 00:02:20, leafp wrote:
> On 2015/05/18 17:52:52, John Messerly wrote:
> > what does this do?
> 
> The types in general need to be evaluated lazily.  The type data on the static
> methods needs to be attached eagerly (at least, before any code that might do
a
> tear-off runs).  So given 
> 
> setSignature(Foo, { statics: () => {bar : http://dart.functionType(core.int,
[])}})
> 
> I can't evaluate the closure bound to 'statics' while evaluating setSignature
> (because http://core.int may not be defined yet).  But I need to know what
names in Foo
> are statics, and that information is under the lambda.  Currently I just pass
in
> the names separately (and redundantly) in the names array to work around this.

> I'd like a better solution - suggestions welcome.  I can inspect Foo to find
the
> names, but I was having trouble reliably teasing out which things were actual
> static methods and which were other random properties on the class (e.g.
> .prototype).  Other options might include moving the laziness under the
> properties (generating statics : () => instead of () => {statics : ...), or
> postponing the linking up of the metadata with the functions until after we
know
> everything is defined.

Yeah, lots of possibilities to fix. Hard to choose :)

A. change the tearoff process for statics, so type info can be associated at
that time. Given the relative infrequency of tearoffs, I think this would be a
fine tradeoff.

B. Like A, but move static method info completely to tear off point (unless
mirrors imported)?

C. change how _setStaticTypes works. Instead have static methods point back to
their class. This allows computing the type later. This pointer could just be
added to everything, as it seems pretty harmless, especially as a symbol.

D. move laziness to each method's signature, means 5 char `() =>` per method

E. just enumerate all static properties. The EcmaScript 6 draft describes which
properties are created on classes/functions. It is just: "prototype" and
"length". "name" will also be added, unless it was defined as a static:
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-runtime-semantics-b....
Non-strict mode functions also get "caller" and "arguments". Searching for
`DefinePropertyOrThrow(F` is an easy way to find them all in the spec. Or just
testing in the console.

I think my stack rank would be: B > E > C > A >> D >>> current :)

The big caveat of B, is we might not be able to use with mirrors. In which case
it won't work until everyone moves to package:reflectable.

[Jennifer Messerly, 2015-05-19 16:34:39]

one more design-y comment. I'll take a look at the impl too now

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart/core.js
File lib/runtime/dart/core.js (right):

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart/core....
lib/runtime/dart/core.js:110: statics: () => ({compare: dart.functionType(int,
[Comparable$(), Comparable$()])}),
On 2015/05/19 00:02:20, leafp wrote:
> On 2015/05/18 17:52:52, John Messerly wrote:
> > do statics need to be separate from methods? for better or worse, Dart
doesn't
> > allow the same name to be used as instance and static (within the same
class)
> > 
> > if they did need to be separate, dart.setStaticSignature(...) would seem
> > cleaner. Less nesting == win
> 
> They don't need to be.  I didn't really like mixing them in - it looked better
> to me listed in their own sections.  I'm not wedded to separating them though.

yeah, not a huge deal to me, just prefer less nesting. I'd be fine with
`dart.setStaticSignature` as a new top level call. What do you think? The
implementation in dart_runtime actually works that way. :-)

[Jennifer Messerly, 2015-05-19 18:19:01]

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart_runti...
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart_runti...
lib/runtime/dart_runtime.js:870: for (let m of mixins) {
this should be reverse order right? e.g.

class Base { Object m(); }
class M1 { Iterable m(); }
class M2 { List m(); }
class Derived extends Base with M1, M2 {}

Return type of `m` should be `List` not `Iterable`

Maybe we could reverse the order up front in the `mixin` function? Then we
wouldn't need to worry about running backwards, either here or in the
initializer method above.

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart_runti...
lib/runtime/dart_runtime.js:875: statics : () => {}, // statics are not
inherited
these could be skipped right? as setSignature ignores unset statics.

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart_runti...
lib/runtime/dart_runtime.js:991: dart._sig = Symbol("_sig");
are all of these needed from generated code? If not, maybe just make them
locals?

    let _sig = Symbol("_sig");

generally I try and avoid exporting symbols, since usually there's a public API
in dart_runtime that exposes the functionality.

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart_runti...
lib/runtime/dart_runtime.js:1024: function _setMethodSignature(f, sigF) {
could this function just be:

defineLazyGetter(f, dart.sig, () => {
  let sigObj = sigF();
  sigObj.__proto__ = f.__proto__[dart.sig];
  return sigObj;
});

of course, we'd need to make `defineLazyGetter` first :). But it would capture
the pattern.
(It's actually similar to defineLazyProperty, but that has a bunch of features
we don't need, like circularity checks and making the property assignable. In
fact, it's overkill even for defineLazyClass use case...)

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart_runti...
lib/runtime/dart_runtime.js:1025: f[dart._sig] = null;
could this just be a closed over local?

let sigObj = null;
function getter() {
  if (sigObj) return sigObj;
  sigObj = sigF();
  sigObj.__proto__ = f.__proto__[dart.sig];
  return sigObj;
}

At that point, not sure if we still need the defineLazyGetter helper (above
comment)

https://codereview.chromium.org/1138793002/diff/120001/lib/src/codegen/js_cod...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1138793002/diff/120001/lib/src/codegen/js_cod...
lib/src/codegen/js_codegen.dart:1001: var lazy = topLevel &&
!_typeIsLoaded(type);
Definitely not something that needs to be changed for this CL, but...

The goal of Loader was to avoid duplication like this, where you need to capture
the same dependencies twice, once in a boolean expression (_typeIsLoaded), and
again when emitting (_emitFunctionTypeParts).

An alternative design would remove this function, and have it work more like:

    if (topLevel) _loader.startTopLevel();
    ... emit stuff ...
    bool lazy = topLevel && _loader.finishTopLevel();

That won't work right now, because startTopLevel requires an argument, which is
what Element we're emitting. there's no notion of "anonymous" top level code.

Interestingly, this suggests another design. Right now, if we want to make
something lazy, it's handled here. But instead, we could just say the whole
top-level element needs to be lazy. That way, if the element ends up lazy for
some other reason, we won't add a nested lazy here too. The alternative design
would then be:

    var e = _loader.currentElement;
    if (topLevel) _loader.startTopLevel(e);
    ... emit stuff ...
    if (topLevel) _loader.finishTopLevel(e);

And then the existing laziness logic for the "currentElement" will handle it.
e.g. a field will become a lazy property.
(note: looking at it now, it seems start/finishTopLevel don't really need the
element. They assert that it's the current element anyway.)

https://codereview.chromium.org/1138793002/diff/120001/lib/src/codegen/js_cod...
lib/src/codegen/js_codegen.dart:1039: if (parent.parent is!
FunctionDeclarationStatement) {
maybe pull out `parent.parent is! FunctionDeclarationStatement` into a local
since we use it twice?


var inFunctionStatement = parent.parent is FunctionDeclarationStatement;
...

if (!inFunctionStatement) { ... }

sort of related
https://github.com/dart-lang/dev_compiler/issues/143#issuecomment-103614170

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

https://codereview.chromium.org/1138793002/diff/120001/lib/src/js/nodes.dart#...
lib/src/js/nodes.dart:1137: final bool vertical;
This could be computed:

bool _vertical;
bool get vertical {
  if (_vertical == null) {
    _vertical = properties.any((p) => p.value is FunctionExpression;
  }
  return _vertical;
}

And then that simplifies Printer.

Not sure I like that better or worse, just throwing it out as an idea to see
what you think :)

Regardless, maybe add a doc comment:

/**
 * If set to true, forces a vertical layout when using the [Printer].
 * Otherwise, layout will be vertical if and only if any [properties]
 * are [FunctionExpression]s.
 */

https://codereview.chromium.org/1138793002/diff/120001/lib/src/js/printer.dart
File lib/src/js/printer.dart (right):

https://codereview.chromium.org/1138793002/diff/120001/lib/src/js/printer.dar...
lib/src/js/printer.dart:864: if (fun.body is ObjectInitializer) out("(");
Lovely fix!

Small suggestion:

// Object initializers require parenthesis to disambiguate
// AssignmentExpression from FunctionBody. See:
//
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-arrow-function-defi...
var needsParen = fun.body is ObjectInitializer;
if (needsParen) ...
...
if (needsParen) ...

[Jennifer Messerly, 2015-05-19 18:22:14]

Forgot high level comment: this looks really good. Generated code style
improvements are up to you, as long as we keep tweaking in follow ups. The only
thing that looks like a bug to me is the mixin signature issue.

I'm happy to help experiment with Loader stuff after this lands. Mainly wanted
to call attention to the possibilities there. The goal was to make a lot of this
stuff easier. Just try and emit stuff, and ask later if it worked :)

[Leaf, 2015-05-19 22:31:13]

Thanks for the comments - very helpful.  I'll revisit the static signatures in a
follow up, maybe try out a couple of different options to see how they turn out.
 PTAL, see what you think.

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart_runti...
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart_runti...
lib/runtime/dart_runtime.js:870: for (let m of mixins) {
On 2015/05/19 18:19:01, John Messerly wrote:
> this should be reverse order right? e.g.
> 
> class Base { Object m(); }
> class M1 { Iterable m(); }
> class M2 { List m(); }
> class Derived extends Base with M1, M2 {}
> 
> Return type of `m` should be `List` not `Iterable`
> 
> Maybe we could reverse the order up front in the `mixin` function? Then we
> wouldn't need to worry about running backwards, either here or in the
> initializer method above.

I don't think so, but could be wrong.  This is mirroring exactly the way that
the methods get copied onto the prototype on line 862, and I think it should be
picking up the right type thereby.  So first we'll copy m from the signature of
M1, then from M2, and thereby end up with List.  Or am I misunderstanding
something?

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart_runti...
lib/runtime/dart_runtime.js:875: statics : () => {}, // statics are not
inherited
On 2015/05/19 18:19:01, John Messerly wrote:
> these could be skipped right? as setSignature ignores unset statics.

Done.

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart_runti...
lib/runtime/dart_runtime.js:991: dart._sig = Symbol("_sig");
On 2015/05/19 18:19:01, John Messerly wrote:
> are all of these needed from generated code? If not, maybe just make them
> locals?
> 
>     let _sig = Symbol("_sig");
> 
> generally I try and avoid exporting symbols, since usually there's a public
API
> in dart_runtime that exposes the functionality.

Done.

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart_runti...
lib/runtime/dart_runtime.js:1024: function _setMethodSignature(f, sigF) {
On 2015/05/19 18:19:01, John Messerly wrote:
> could this function just be:
> 
> defineLazyGetter(f, dart.sig, () => {
>   let sigObj = sigF();
>   sigObj.__proto__ = f.__proto__[dart.sig];
>   return sigObj;
> });
> 
> of course, we'd need to make `defineLazyGetter` first :). But it would capture
> the pattern.
> (It's actually similar to defineLazyProperty, but that has a bunch of features
> we don't need, like circularity checks and making the property assignable. In
> fact, it's overkill even for defineLazyClass use case...)

Done.

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart_runti...
lib/runtime/dart_runtime.js:1025: f[dart._sig] = null;
On 2015/05/19 18:19:01, John Messerly wrote:
> could this just be a closed over local?
> 
> let sigObj = null;
> function getter() {
>   if (sigObj) return sigObj;
>   sigObj = sigF();
>   sigObj.__proto__ = f.__proto__[dart.sig];
>   return sigObj;
> }
> 
> At that point, not sure if we still need the defineLazyGetter helper (above
> comment)

Oh yeah, nice!  Factored out into a helper per previous comment as well.

https://codereview.chromium.org/1138793002/diff/120001/lib/src/codegen/js_cod...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1138793002/diff/120001/lib/src/codegen/js_cod...
lib/src/codegen/js_codegen.dart:1001: var lazy = topLevel &&
!_typeIsLoaded(type);
On 2015/05/19 18:19:01, John Messerly wrote:
> Definitely not something that needs to be changed for this CL, but...
> 
> The goal of Loader was to avoid duplication like this, where you need to
capture
> the same dependencies twice, once in a boolean expression (_typeIsLoaded), and
> again when emitting (_emitFunctionTypeParts).
> 
> An alternative design would remove this function, and have it work more like:
> 
>     if (topLevel) _loader.startTopLevel();
>     ... emit stuff ...
>     bool lazy = topLevel && _loader.finishTopLevel();
> 
> That won't work right now, because startTopLevel requires an argument, which
is
> what Element we're emitting. there's no notion of "anonymous" top level code.
> 
> Interestingly, this suggests another design. Right now, if we want to make
> something lazy, it's handled here. But instead, we could just say the whole
> top-level element needs to be lazy. That way, if the element ends up lazy for
> some other reason, we won't add a nested lazy here too. The alternative design
> would then be:
> 
>     var e = _loader.currentElement;
>     if (topLevel) _loader.startTopLevel(e);
>     ... emit stuff ...
>     if (topLevel) _loader.finishTopLevel(e);
> 
> And then the existing laziness logic for the "currentElement" will handle it.
> e.g. a field will become a lazy property.
> (note: looking at it now, it seems start/finishTopLevel don't really need the
> element. They assert that it's the current element anyway.)

Acknowledged.

https://codereview.chromium.org/1138793002/diff/120001/lib/src/codegen/js_cod...
lib/src/codegen/js_codegen.dart:1039: if (parent.parent is!
FunctionDeclarationStatement) {
On 2015/05/19 18:19:01, John Messerly wrote:
> maybe pull out `parent.parent is! FunctionDeclarationStatement` into a local
> since we use it twice?
> 
> 
> var inFunctionStatement = parent.parent is FunctionDeclarationStatement;
> ...
> 
> if (!inFunctionStatement) { ... }
> 
> sort of related
> https://github.com/dart-lang/dev_compiler/issues/143#issuecomment-103614170

Done.

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

https://codereview.chromium.org/1138793002/diff/120001/lib/src/js/nodes.dart#...
lib/src/js/nodes.dart:1137: final bool vertical;
On 2015/05/19 18:19:01, John Messerly wrote:
> This could be computed:
> 
> bool _vertical;
> bool get vertical {
>   if (_vertical == null) {
>     _vertical = properties.any((p) => p.value is FunctionExpression;
>   }
>   return _vertical;
> }
> 
> And then that simplifies Printer.
> 
> Not sure I like that better or worse, just throwing it out as an idea to see
> what you think :)
> 
> Regardless, maybe add a doc comment:
> 
> /**
>  * If set to true, forces a vertical layout when using the [Printer].
>  * Otherwise, layout will be vertical if and only if any [properties]
>  * are [FunctionExpression]s.
>  */
> 

Done.

https://codereview.chromium.org/1138793002/diff/120001/lib/src/js/printer.dart
File lib/src/js/printer.dart (right):

https://codereview.chromium.org/1138793002/diff/120001/lib/src/js/printer.dar...
lib/src/js/printer.dart:864: if (fun.body is ObjectInitializer) out("(");
On 2015/05/19 18:19:01, John Messerly wrote:
> Lovely fix!
> 
> Small suggestion:
> 
> // Object initializers require parenthesis to disambiguate
> // AssignmentExpression from FunctionBody. See:
> //
>
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-arrow-function-defi...
> var needsParen = fun.body is ObjectInitializer;
> if (needsParen) ...
> ...
> if (needsParen) ...
> 
> 

Done.

[Jennifer Messerly, 2015-05-19 22:49:48]

changes lgtm.

by the way, another thought on less type info: we may be able to skip private
methods in some cases, since we can see the whole library

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart_runti...
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/1138793002/diff/120001/lib/runtime/dart_runti...
lib/runtime/dart_runtime.js:870: for (let m of mixins) {
On 2015/05/19 22:31:12, leafp wrote:
> On 2015/05/19 18:19:01, John Messerly wrote:
> > this should be reverse order right? e.g.
> > 
> > class Base { Object m(); }
> > class M1 { Iterable m(); }
> > class M2 { List m(); }
> > class Derived extends Base with M1, M2 {}
> > 
> > Return type of `m` should be `List` not `Iterable`
> > 
> > Maybe we could reverse the order up front in the `mixin` function? Then we
> > wouldn't need to worry about running backwards, either here or in the
> > initializer method above.
> 
> I don't think so, but could be wrong.  This is mirroring exactly the way that
> the methods get copied onto the prototype on line 862, and I think it should
be
> picking up the right type thereby.  So first we'll copy m from the signature
of
> M1, then from M2, and thereby end up with List.  Or am I misunderstanding
> something?

ah, right, later ones should overwrite. I think that's why I added comments on
848 and 860 ... apparently I'm easily confused by it :). Anyway sounds good,
worth adding a test case? Or is it covered by another one?

https://codereview.chromium.org/1138793002/diff/160001/lib/src/js/printer.dart
File lib/src/js/printer.dart (right):

https://codereview.chromium.org/1138793002/diff/160001/lib/src/js/printer.dar...
lib/src/js/printer.dart:866: //
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-arrow-function-defi...
`sec-arrow-function-definitions` seems to have gotten cut off

https://codereview.chromium.org/1138793002/diff/160001/test/browser/runtime_t...
File test/browser/runtime_tests.js (right):

https://codereview.chromium.org/1138793002/diff/160001/test/browser/runtime_t...
test/browser/runtime_tests.js:171: let cls8 = dart.fn((b, s, o) => { return
null; }, B, [B, String], {p: Object});
long line

[Leaf, 2015-05-19 23:24:08]

Okay, I'm going to land this now, will follow on to address the remaining
comments.  Also just realized that I didn't change the tearoff sites for methods
to call the new tearoff method, will follow up with the change shortly.

https://codereview.chromium.org/1138793002/diff/160001/lib/src/js/printer.dart
File lib/src/js/printer.dart (right):

https://codereview.chromium.org/1138793002/diff/160001/lib/src/js/printer.dar...
lib/src/js/printer.dart:866: //
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-arrow-function-defi...
On 2015/05/19 22:49:48, John Messerly wrote:
> `sec-arrow-function-definitions` seems to have gotten cut off

Done.

https://codereview.chromium.org/1138793002/diff/160001/test/browser/runtime_t...
File test/browser/runtime_tests.js (right):

https://codereview.chromium.org/1138793002/diff/160001/test/browser/runtime_t...
test/browser/runtime_tests.js:171: let cls8 = dart.fn((b, s, o) => { return
null; }, B, [B, String], {p: Object});
On 2015/05/19 22:49:48, John Messerly wrote:
> long line

Done.

[Leaf, 2015-05-19 23:24:40]

Committed patchset #10 (id:180001) manually as
636b3401b62022f7ef563c720baaf1e200c6f8f1 (presubmit successful).