fixes #131, use before define from variables to classes

fixes #131, use before define from variables to classes

Factored the load order logic into its own file, as it was easier to work with that way. We could definitely tweak the structure.

Also tried hoisting constants out to top level, but it's not quite ready yet, so splitting that work out into a separate CL.

(also fixes #152 about cleaning up lazyClass, and fixes #181 about static fields)

R=vsm@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/3dcea8bf156b3947c4dbf4b36da55a64cd9dc492

[Jennifer Messerly, 2015-05-12 16:01:47]

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

[Jennifer Messerly, 2015-05-12 16:23:46]

https://codereview.chromium.org/1133593004/diff/20001/lib/runtime/dart/async.js
File lib/runtime/dart/async.js (left):

https://codereview.chromium.org/1133593004/diff/20001/lib/runtime/dart/async....
lib/runtime/dart/async.js:4654: Zone.ROOT = _ROOT_ZONE;
this is an example of something fixed.  _ROOT_ZONE is now defined before use.

https://codereview.chromium.org/1133593004/diff/20001/lib/runtime/dart/async.js
File lib/runtime/dart/async.js (right):

https://codereview.chromium.org/1133593004/diff/20001/lib/runtime/dart/async....
lib/runtime/dart/async.js:4845: let _ROOT_ZONE = dart.const(new _RootZone());
and here's the _ROOT_ZONE fixed version. The order is _RootZone class, 
_ROOT_ZONE field, then other fields that depend on it, including static ones.

https://codereview.chromium.org/1133593004/diff/20001/lib/runtime/dart/collec...
File lib/runtime/dart/collection.js (right):

https://codereview.chromium.org/1133593004/diff/20001/lib/runtime/dart/collec...
lib/runtime/dart/collection.js:1036: this[_state] =
HasNextIterator$()._NOT_MOVED_YET;
this is needed to use the dynamic instantiation for statics consistently.
we were previously broken on static fields: we had different slots for
HasNextIterator<T>._NOT_MOVED_YET, for each T. That's fine if it's final
(although you could observe lazy-init happening more than once), but mutable
static fields in a generic were broken.

https://codereview.chromium.org/1133593004/diff/20001/lib/runtime/dart_runtim...
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/1133593004/diff/20001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:400: this._stringValue = opt_typedefName;
I merged the two subtypes, since the only difference was now whether
_stringValue was set.
We could store a `bool isTypedef` field too if we wanted to.

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

https://codereview.chromium.org/1133593004/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:52: class ConstFieldVisitor {
I forgot to move this to side_effect_analysis.dart file (which already exists).
What do you think?

Since the main visitor is now >2k loc, I've been trying to split things out into
other files when possible

https://codereview.chromium.org/1133593004/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:123: _loader = new
ModuleItemLoadOrder(_emitModuleItem);
alternate design: this could be a mixin which declares an abstract method for
emitting the module item, which JSCodegenVisitor implements.

I went with composition though.

https://codereview.chromium.org/1133593004/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:145: // TODO(jmesserly): visit scriptTag,
directives?
this TODO just moved from somewhere else

https://codereview.chromium.org/1133593004/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1114: var fnType = type as FunctionType;
if we get here, type has to be FunctionType.

https://codereview.chromium.org/1133593004/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1169: JS.Expression _maybeQualifiedName(Element
e, JS.Expression name,
we can now make name qualification decisions after the main pass through the
code.

for example, lazy classes: a class is lazy if we try and generate its `extends`
list, and fail, because of library cycles. If a class is lazy, we need to
reference it as `exports.ClassName` instead of `ClassName`, because the latter
will not exist. But we might not know that yet:


class Foo { method() { print(new Bar()); } }
class Bar extends Baz/*from library cycle*/ {}

When we see Foo, we don't know Bar is lazy.


How did we used to make lazy classes work? Through code duplication: we could
compute if a class was lazy, but the logic had to be kept in sync with how we
emitted the extends clause. That approach gets harder as we have more things
generating at top level, especially because with fields, because we cannot
understand their dependencies without generating the initializer Expression. (by
contrast, classes capture their dependencies in the element model as well as the
AST)

https://codereview.chromium.org/1133593004/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1708: return js.call('dart.const(#)', expr());
the idea here is: _emitConst can tell _loader it's starting a top level field,
then it calls the `expr` function, then it stops emitting top-level field. But I
removed that from this CL

https://codereview.chromium.org/1133593004/diff/20001/lib/src/codegen/js_name...
File lib/src/codegen/js_names.dart (right):

https://codereview.chromium.org/1133593004/diff/20001/lib/src/codegen/js_name...
lib/src/codegen/js_names.dart:17: static int __id = 0;
this helped for debugging, I could remove it though now that the bug is fixed?

https://codereview.chromium.org/1133593004/diff/20001/lib/src/dependency_grap...
File lib/src/dependency_graph.dart (right):

https://codereview.chromium.org/1133593004/diff/20001/lib/src/dependency_grap...
lib/src/dependency_graph.dart:478: 'dart.async',
stole this from your CL. Gets sunflower working

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

https://codereview.chromium.org/1133593004/diff/20001/lib/src/utils.dart#newc...
lib/src/utils.dart:390: ParameterizedType fillDynamicTypeArgs(ParameterizedType
t, TypeProvider types) {
also works for function types now

https://codereview.chromium.org/1133593004/diff/20001/test/codegen/expect/mis...
File test/codegen/expect/misc.js (right):

https://codereview.chromium.org/1133593004/diff/20001/test/codegen/expect/mis...
test/codegen/expect/misc.js:5: class _Uninitialized extends core.Object {
this was the repro in the bug

https://codereview.chromium.org/1133593004/diff/20001/test/codegen/expect/sun...
File test/codegen/expect/sunflower/dom.js (right):

https://codereview.chromium.org/1133593004/diff/20001/test/codegen/expect/sun...
test/codegen/expect/sunflower/dom.js:17: let EventListener =
dart.typedef('EventListener', dart.functionType(dart.void, [Event]));
typedefs can now skip the laziness (usually)

[vsm, 2015-05-12 18:00:11]

lgtm

nice!

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

https://codereview.chromium.org/1133593004/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:52: class ConstFieldVisitor {
On 2015/05/12 16:23:45, John Messerly wrote:
> I forgot to move this to side_effect_analysis.dart file (which already
exists).
> What do you think?
> 
> Since the main visitor is now >2k loc, I've been trying to split things out
into
> other files when possible

+1 to moving out.  Not really js_cg specific.

https://codereview.chromium.org/1133593004/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:75: // by ConstantEvaluator.
Is that equivalent to only doing this if !field.isConst?

https://codereview.chromium.org/1133593004/diff/20001/lib/src/codegen/js_modu...
File lib/src/codegen/js_module_item_order.dart (right):

https://codereview.chromium.org/1133593004/diff/20001/lib/src/codegen/js_modu...
lib/src/codegen/js_module_item_order.dart:25: final _loaded = new Map<Element,
bool>.identity();
Can you add a comment on why identity?  Should all our Map<Element, *> be
identity?

https://codereview.chromium.org/1133593004/diff/20001/lib/src/codegen/js_modu...
lib/src/codegen/js_module_item_order.dart:98: // Normally this is called
implicitly by [loadDeclaration] and/or [loadElement].
line len?

https://codereview.chromium.org/1133593004/diff/20001/lib/src/codegen/js_modu...
lib/src/codegen/js_module_item_order.dart:182: if (library == null) return true;
When would this be null?

[Jennifer Messerly, 2015-05-12 18:47:04]

thanks!!!

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

https://codereview.chromium.org/1133593004/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:52: class ConstFieldVisitor {
On 2015/05/12 18:00:11, vsm wrote:
> On 2015/05/12 16:23:45, John Messerly wrote:
> > I forgot to move this to side_effect_analysis.dart file (which already
> exists).
> > What do you think?
> > 
> > Since the main visitor is now >2k loc, I've been trying to split things out
> into
> > other files when possible
> 
> +1 to moving out.  Not really js_cg specific.

Done.

https://codereview.chromium.org/1133593004/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:75: // by ConstantEvaluator.
On 2015/05/12 18:00:11, vsm wrote:
> Is that equivalent to only doing this if !field.isConst?

Ah, yes. I think so. I'll add an assert in place of TODO :)

https://codereview.chromium.org/1133593004/diff/20001/lib/src/codegen/js_modu...
File lib/src/codegen/js_module_item_order.dart (right):

https://codereview.chromium.org/1133593004/diff/20001/lib/src/codegen/js_modu...
lib/src/codegen/js_module_item_order.dart:25: final _loaded = new Map<Element,
bool>.identity();
On 2015/05/12 18:00:11, vsm wrote:
> Can you add a comment on why identity?  Should all our Map<Element, *> be
> identity?

ah, I can remove. This was to workaround the temp-identity problem.

https://codereview.chromium.org/1133593004/diff/20001/lib/src/codegen/js_modu...
lib/src/codegen/js_module_item_order.dart:98: // Normally this is called
implicitly by [loadDeclaration] and/or [loadElement].
On 2015/05/12 18:00:11, vsm wrote:
> line len?

Done.

https://codereview.chromium.org/1133593004/diff/20001/lib/src/codegen/js_modu...
lib/src/codegen/js_module_item_order.dart:182: if (library == null) return true;
On 2015/05/12 18:00:11, vsm wrote:
> When would this be null?

good catch. This showed up in the code at one point ... I think maybe our
synthetic elements for temps didn't have enclosingElement filled in, hence no
library. Anyways, fixed now, so removed this check :)

[Jennifer Messerly, 2015-05-12 18:47:42]

Committed patchset #3 (id:40001) manually as
3dcea8bf156b3947c4dbf4b36da55a64cd9dc492 (presubmit successful).