Name and hoist types

Name and hoist types

Adds infrastructure for naming and hoisting types in various ways. Makes an initial cut at how to slice the naming, but we will want to adjust.

BUG=
R=jmesserly@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/5d101bb7dc8cb775458af123df7525e509fe6a38

[Leaf, 2016-05-24 21:08:49]

Description was changed from

==========
Name and hoist types

BUG=
==========

to

==========
Name and hoist types

Adds infrastructure for naming and hoisting types in various ways. Makes an
initial cut at how to slice the naming, but we will want to adjust. 

BUG=
==========

[Leaf, 2016-05-24 21:08:50]

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

[Leaf, 2016-05-24 21:09:34]

Take a look and see what you think.  Rietveld won't upload the dart_sdk diff, so
you may want to download the patch and generate it yourself to see what the
output looks like.

[Jennifer Messerly, 2016-05-24 21:18:53]

On 2016/05/24 21:09:34, Leaf wrote:
> Take a look and see what you think.  Rietveld won't upload the dart_sdk diff,
so
> you may want to download the patch and generate it yourself to see what the
> output looks like.

Leaf uploaded it as a branch, so
https://github.com/dart-lang/dev_compiler/compare/nametypes is another way to
see it

[sra1, 2016-05-24 21:26:23]

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

https://codereview.chromium.org/1988023008/diff/140001/test/browser/runtime_t...
test/browser/runtime_tests.js:156: dart.fn(bar1, dart.definiteFunctionType(A,
[C, String]));
It would be nice to shorten this (another CL).

maybe 'dart.arrow'.

[Jennifer Messerly, 2016-05-24 21:56:39]

Few thoughts on organization/style, but LGTM overall.

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
File lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
lib/src/compiler/code_generator.dart:2292: bool hoistType: true,
do we ever set this to false? I wasn't able to find it, seems like we always let
the default value kick in or just pass it along.

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
lib/src/compiler/code_generator.dart:4637: /// are emitted in place with a
hoisted variable for a cache.
Consider: including an example here of what it looks like in the code. Might
make it easier to visualize.

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
lib/src/compiler/code_generator.dart:4638: class _CacheTable {
consider: putting these new classes into their own file(s)? They look pretty
self contained (which is good!)

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
lib/src/compiler/code_generator.dart:4646: return (js.statement('let #;',
[name]));
nit: parens not needed around return value

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
lib/src/compiler/code_generator.dart:4651: /// Emit a list of statements
declaring the cache variables for
doc comment style nit -- first sentence should be its own paragraph:
https://www.dartlang.org/effective-dart/documentation/#do-make-the-first-sent...
(applies to all of them)

That said content is more important and these are private APIs so I wouldn't
worry about it, but as a general FYI.

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
lib/src/compiler/code_generator.dart:4670: var temp = _names[type];
you could also do

    var temp = _names.putIfAbsent(type, () => chooseTypeName(type));

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
lib/src/compiler/code_generator.dart:4672: ;
unintended empty statement?

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
lib/src/compiler/code_generator.dart:4722: _GeneratorTable();
note: this is not needed

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
lib/src/compiler/code_generator.dart:4738: JS.Expression nameType(DartType type,
JS.Expression typeRep) {
this looks mostly identical to the super method, might be worth sharing

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
lib/src/compiler/code_generator.dart:4803: /// Add the type and its
representation to the table, returning an
nit: lowercase "add"

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
lib/src/compiler/code_generator.dart:4805: /// If [hoist] is true, then the JS
representation will be hoisted up
consider: splitting this into a few paragraphs?

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
lib/src/compiler/code_generator.dart:4812: /// representations as definite and
indefinite function types).
I seem to recall at one point we had considered putting the arrow type (fuzzy or
definite) on FunctionTypeImpl in Analyzer. Makes me wonder about that again.
Maybe that's something we should look into at some point?

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
lib/src/compiler/code_generator.dart:4813: JS.Expression nameType(DartType type,
JS.Expression typeRep, bool hoist,
any reason to make "hoist" required and "definite" a named arg?

usual rule is to avoid required booleans:
https://www.dartlang.org/effective-dart/design/#avoid-positional-boolean-para...

[Leaf, 2016-05-25 00:56:13]

PTAL.  Also pushed an update to branch nametypes.

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
File lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
lib/src/compiler/code_generator.dart:2292: bool hoistType: true,
On 2016/05/24 21:56:39, John Messerly wrote:
> do we ever set this to false? I wasn't able to find it, seems like we always
let
> the default value kick in or just pass it along.

It was here so that we can play with different strategies.  Added flags so its
controllable from the command line, plus test.

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
lib/src/compiler/code_generator.dart:4637: /// are emitted in place with a
hoisted variable for a cache.
On 2016/05/24 21:56:39, John Messerly wrote:
> Consider: including an example here of what it looks like in the code. Might
> make it easier to visualize.

Done below.

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
lib/src/compiler/code_generator.dart:4638: class _CacheTable {
On 2016/05/24 21:56:39, John Messerly wrote:
> consider: putting these new classes into their own file(s)? They look pretty
> self contained (which is good!)

Done.

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
lib/src/compiler/code_generator.dart:4646: return (js.statement('let #;',
[name]));
On 2016/05/24 21:56:39, John Messerly wrote:
> nit: parens not needed around return value

Done.

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
lib/src/compiler/code_generator.dart:4646: return (js.statement('let #;',
[name]));
On 2016/05/24 21:56:39, John Messerly wrote:
> nit: parens not needed around return value

Done.

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
lib/src/compiler/code_generator.dart:4651: /// Emit a list of statements
declaring the cache variables for
On 2016/05/24 21:56:39, John Messerly wrote:
> doc comment style nit -- first sentence should be its own paragraph:
>
https://www.dartlang.org/effective-dart/documentation/#do-make-the-first-sent...
> (applies to all of them)
> 
> That said content is more important and these are private APIs so I wouldn't
> worry about it, but as a general FYI.

Acknowledged.

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
lib/src/compiler/code_generator.dart:4670: var temp = _names[type];
On 2016/05/24 21:56:39, John Messerly wrote:
> you could also do
> 
>     var temp = _names.putIfAbsent(type, () => chooseTypeName(type));

Acknowledged.

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
lib/src/compiler/code_generator.dart:4672: ;
On 2016/05/24 21:56:39, John Messerly wrote:
> unintended empty statement?

Done.

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
lib/src/compiler/code_generator.dart:4722: _GeneratorTable();
On 2016/05/24 21:56:39, John Messerly wrote:
> note: this is not needed

Done.

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
lib/src/compiler/code_generator.dart:4738: JS.Expression nameType(DartType type,
JS.Expression typeRep) {
On 2016/05/24 21:56:39, John Messerly wrote:
> this looks mostly identical to the super method, might be worth sharing

It's 3 of 5 lines that are duplicated.  I don't see an easy way to share that
doesn't just add ceremony.

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
lib/src/compiler/code_generator.dart:4803: /// Add the type and its
representation to the table, returning an
On 2016/05/24 21:56:39, John Messerly wrote:
> nit: lowercase "add"

Done.

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
lib/src/compiler/code_generator.dart:4805: /// If [hoist] is true, then the JS
representation will be hoisted up
On 2016/05/24 21:56:39, John Messerly wrote:
> consider: splitting this into a few paragraphs?

Done.

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
lib/src/compiler/code_generator.dart:4812: /// representations as definite and
indefinite function types).
On 2016/05/24 21:56:39, John Messerly wrote:
> I seem to recall at one point we had considered putting the arrow type (fuzzy
or
> definite) on FunctionTypeImpl in Analyzer. Makes me wonder about that again.
> Maybe that's something we should look into at some point?

Acknowledged.

https://codereview.chromium.org/1988023008/diff/150001/lib/src/compiler/code_...
lib/src/compiler/code_generator.dart:4813: JS.Expression nameType(DartType type,
JS.Expression typeRep, bool hoist,
On 2016/05/24 21:56:39, John Messerly wrote:
> any reason to make "hoist" required and "definite" a named arg?
> 
> usual rule is to avoid required booleans:
>
https://www.dartlang.org/effective-dart/design/#avoid-positional-boolean-para...

Made them both named, asserted hoist is set.  Definite has an obvious default
(almost no types are definite), but hoist is something that is specific to every
call site and shouldn't be omitted.

[Jennifer Messerly, 2016-05-25 01:24:18]

lgtm

https://codereview.chromium.org/1988023008/diff/160001/lib/src/compiler/compi...
File lib/src/compiler/compiler.dart (right):

https://codereview.chromium.org/1988023008/diff/160001/lib/src/compiler/compi...
lib/src/compiler/compiler.dart:250: ..addFlag('hoist-instance-creation',
Should we hide these options from the "--help"? I think there's a boolean that
does that. (hide: true?)

[Leaf, 2016-05-25 18:07:42]

Description was changed from

==========
Name and hoist types

Adds infrastructure for naming and hoisting types in various ways. Makes an
initial cut at how to slice the naming, but we will want to adjust. 

BUG=
==========

to

==========
Name and hoist types

Adds infrastructure for naming and hoisting types in various ways. Makes an
initial cut at how to slice the naming, but we will want to adjust.

BUG=
R=jmesserly@google.com

Committed:
https://github.com/dart-lang/dev_compiler/commit/5d101bb7dc8cb775458af123df75...
==========

[Leaf, 2016-05-25 18:07:43]

Committed patchset #12 (id:160001) manually as
5d101bb7dc8cb775458af123df7525e509fe6a38 (presubmit successful).