Use smoke in polymer and polymer_expressions.

Use smoke in polymer and polymer_expressions.

R=jmesserly@google.com

Committed: https://code.google.com/p/dart/source/detail?r=32991

[Siggi Cherem (dart-lang), 2014-02-20 05:05:16]

there is still more work to be done in polymer, but all tests are passing with
these changes. This may also clarify why I included some things in the smoke
package in the first place.

(note: this CL cannot be submitted/patched without first getting the smoke
package)

https://codereview.chromium.org/173003006/diff/1/pkg/polymer/lib/polymer.dart
File pkg/polymer/lib/polymer.dart (right):

https://codereview.chromium.org/173003006/diff/1/pkg/polymer/lib/polymer.dart...
pkg/polymer/lib/polymer.dart:73: _unused() => MirrorSystem; // just to prevent
analyzer warnings.
technically I still don't need this. We still use mirrors for the loader.dart
code. I plan to separate that out so the only use of mirrors is for the
MirrorsUsed annotation, but we are not there yet.

https://codereview.chromium.org/173003006/diff/1/pkg/polymer_expressions/lib/...
File pkg/polymer_expressions/lib/eval.dart (right):

https://codereview.chromium.org/173003006/diff/1/pkg/polymer_expressions/lib/...
pkg/polymer_expressions/lib/eval.dart:44: return f(a);
another option here would be to change _MethodClosure in smoke/mirrors.dart.
That code is similar to the old [Method] class below, except that I made it more
general. The default 'call' is a call of 0 args, but I could change it to a call
of 1 arg. Feels weird to make that the default, but it would allow us to get rid
of this weirdness here.

Thoughts?

[Siggi Cherem (dart-lang), 2014-02-20 16:50:16]

https://codereview.chromium.org/173003006/diff/1/pkg/polymer/lib/polymer.dart
File pkg/polymer/lib/polymer.dart (right):

https://codereview.chromium.org/173003006/diff/1/pkg/polymer/lib/polymer.dart...
pkg/polymer/lib/polymer.dart:73: _unused() => MirrorSystem; // just to prevent
analyzer warnings.
On 2014/02/20 05:05:16, Siggi Cherem (dart-lang) wrote:
> technically I still don't need this. We still use mirrors for the loader.dart
> code. I plan to separate that out so the only use of mirrors is for the
> MirrorsUsed annotation, but we are not there yet.

turns out I didn't need this altogether (the analyzer will not give you a
warning if you have the MirrorsUsed annotation). removed.

[blois, 2014-02-20 17:29:24]

https://codereview.chromium.org/173003006/diff/60001/pkg/polymer/lib/polymer....
File pkg/polymer/lib/polymer.dart (right):

https://codereview.chromium.org/173003006/diff/60001/pkg/polymer/lib/polymer....
pkg/polymer/lib/polymer.dart:50: import 'dart:mirrors';
if mirrors are not being used otherwise, add a show MirrorsUsed to make it
clear.

https://codereview.chromium.org/173003006/diff/60001/pkg/polymer_expressions/...
File pkg/polymer_expressions/lib/eval.dart (right):

https://codereview.chromium.org/173003006/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/eval.dart:13: import 'dart:mirrors' as unused;
just show MirrorsUsed?

https://codereview.chromium.org/173003006/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/eval.dart:23: _unused() => unused.MirrorSystem; //
to prevent analyzer warnings
Should this be removed as well?

[Jennifer Messerly, 2014-02-20 21:15:40]

lgtm!

https://codereview.chromium.org/173003006/diff/60001/pkg/polymer/lib/polymer....
File pkg/polymer/lib/polymer.dart (right):

https://codereview.chromium.org/173003006/diff/60001/pkg/polymer/lib/polymer....
pkg/polymer/lib/polymer.dart:50: import 'dart:mirrors';
On 2014/02/20 17:29:25, blois wrote:
> if mirrors are not being used otherwise, add a show MirrorsUsed to make it
> clear.

+1

https://codereview.chromium.org/173003006/diff/60001/pkg/polymer/lib/src/inst...
File pkg/polymer/lib/src/instance.dart (right):

https://codereview.chromium.org/173003006/diff/60001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:380: type = currentValue.runtimeType;
Hmm, did the VM fix .runtimeType? It used to return things like _Smi for int and
String exposed a private internal class too. That's why we added _getCoreType.
OTOH, I do see that 42.runtimeType now prints "int" so maybe it is fixed. :)

https://codereview.chromium.org/173003006/diff/60001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:403: Object deserializeValue(String value,
Object currentValue, Type type) =>
lovely!

https://codereview.chromium.org/173003006/diff/60001/pkg/polymer_expressions/...
File pkg/polymer_expressions/lib/eval.dart (right):

https://codereview.chromium.org/173003006/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/eval.dart:13: import 'dart:mirrors' as unused;
On 2014/02/20 17:29:25, blois wrote:
> just show MirrorsUsed?

yeah, unused is sort of odd since it is actually used

[Siggi Cherem (dart-lang), 2014-02-21 03:55:12]

Thanks guys!

https://codereview.chromium.org/173003006/diff/60001/pkg/polymer/lib/polymer....
File pkg/polymer/lib/polymer.dart (right):

https://codereview.chromium.org/173003006/diff/60001/pkg/polymer/lib/polymer....
pkg/polymer/lib/polymer.dart:50: import 'dart:mirrors';
On 2014/02/20 21:15:41, John Messerly wrote:
> On 2014/02/20 17:29:25, blois wrote:
> > if mirrors are not being used otherwise, add a show MirrorsUsed to make it
> > clear.
> 
> +1

soon - I still use it in development time for the loading and initializing apps,
but I plan to refactor that out soon.

https://codereview.chromium.org/173003006/diff/60001/pkg/polymer/lib/src/inst...
File pkg/polymer/lib/src/instance.dart (right):

https://codereview.chromium.org/173003006/diff/60001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:380: type = currentValue.runtimeType;
On 2014/02/20 21:15:41, John Messerly wrote:
> Hmm, did the VM fix .runtimeType? It used to return things like _Smi for int
and
> String exposed a private internal class too. That's why we added _getCoreType.
> OTOH, I do see that 42.runtimeType now prints "int" so maybe it is fixed. :)

Oh, funny, I was wondering why you had that :). Yeah, just tested it once more
and it seems to be working great. woo hoo!

https://codereview.chromium.org/173003006/diff/60001/pkg/polymer/test/prop_at...
File pkg/polymer/test/prop_attr_reflection_test.dart (right):

https://codereview.chromium.org/173003006/diff/60001/pkg/polymer/test/prop_at...
pkg/polymer/test/prop_attr_reflection_test.dart:24: // TODO(sigmund): change
this type to Object (see dartbug.com/16962)
FYI - I managed to revert this workaround (I made a more general workaround in
_toType in smoke.mirrors)

https://codereview.chromium.org/173003006/diff/60001/pkg/polymer_expressions/...
File pkg/polymer_expressions/lib/eval.dart (right):

https://codereview.chromium.org/173003006/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/eval.dart:13: import 'dart:mirrors' as unused;
On 2014/02/20 21:15:41, John Messerly wrote:
> On 2014/02/20 17:29:25, blois wrote:
> > just show MirrorsUsed?
> 
> yeah, unused is sort of odd since it is actually used

good idea. Done.

https://codereview.chromium.org/173003006/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/eval.dart:23: _unused() => unused.MirrorSystem; //
to prevent analyzer warnings
On 2014/02/20 17:29:25, blois wrote:
> Should this be removed as well?

yeah, I fixed it elsewhere, but forgot to do so here. Thanks!

https://codereview.chromium.org/173003006/diff/60001/pkg/polymer_expressions/...
pkg/polymer_expressions/lib/eval.dart:44: return f(a);
BTW - I removed this workaround and instead hid this inside smoke.mirrors (see
_MethodClosure.call in this CL)

https://codereview.chromium.org/173003006/diff/270001/pkg/smoke/lib/mirrors.dart
File pkg/smoke/lib/mirrors.dart (right):

https://codereview.chromium.org/173003006/diff/270001/pkg/smoke/lib/mirrors.d...
pkg/smoke/lib/mirrors.dart:309: call([a, b, c]) => invoke(receiver, methodName,
[a, b, c], adjust: true);
technically all we need is call(a) for eval.dart. I'm happy to change it to that
if you prefer.

[Siggi Cherem (dart-lang), 2014-02-21 23:14:09]

PTAL

I just added two small changes needed before I can switch to use smoke:
 - in loader.dart I added a call to ensure that smoke is initialized. This is
needed because pub-build will remove the default implementation of smoke. For
now, we just initialize smoke with the mirror-based implementation.

 - in builder.dart I added the transformation for the smoke package that will
remove the default implementation. This is so that both build.dart and pub-build
are consistent. 

Unfortunately, I don't have this running in our test infrastructure yet. I think
that's ok for now. Even when we switch to use codegen, the only difference will
be that pub-build will make the mirror-based implementation unreachable, while
the tests will still have the code reachable but unused.

Alternatively I could remove the transformation in package-smoke until we are
ready to test it out.

[Jennifer Messerly, 2014-02-21 23:31:29]

lgtm

[Siggi Cherem (dart-lang), 2014-02-24 23:05:54]

Committed patchset #7 manually as r32991 (presubmit successful).

[Siggi Cherem (dart-lang), 2014-02-24 23:12:43]

https://codereview.chromium.org/173003006/diff/400001/pkg/smoke/lib/mirrors.dart
File pkg/smoke/lib/mirrors.dart (right):

https://codereview.chromium.org/173003006/diff/400001/pkg/smoke/lib/mirrors.d...
pkg/smoke/lib/mirrors.dart:300: // Technically we could just use noSuchMethod to
implement [call], but we
I did one last change here to remove the use of nSM. It's technically not
correct to remove it, but (a) we don't use this in practice, and (b) it saves
space in the dart2js output (~25% on medium tests). I'm ok with doing so because
this is anyways a workaround for dartbug.com/13002.

I updated the comment here to clarify.