Emit forwarding getter/setter when overriding just a getter or setter.

Emit forwarding getter/setter when overriding just a getter or setter.

Based on Vijay's change: https://github.com/dart-lang/dev_compiler/commit/74dc4a6a808b271312aabe1bf463f1e7f4889a43

BUG=https://github.com/dart-lang/dev_compiler/issues/497
R=jmesserly@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/1b461199920149e856272182187f506260f62cf5

[Harry Terkelsen, 2016-04-20 00:56:25]

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

[Harry Terkelsen, 2016-04-20 00:57:36]

https://codereview.chromium.org/1899373002/diff/20001/lib/runtime/dart_sdk.js
File lib/runtime/dart_sdk.js (right):

https://codereview.chromium.org/1899373002/diff/20001/lib/runtime/dart_sdk.js...
lib/runtime/dart_sdk.js:4062: +      super.length = value;
it looks like ddc is smart enough to rewrite a['b'] into a.b. Neat!

[Jennifer Messerly, 2016-04-20 21:16:40]

Wow nice! A few thought here, in how we can make the field code a lot cleaner.
Sorry about that -- the mess definitely existed already -- if you prefer to
stage it by landing this and do a clean up after that works for me.

But it'd be so awesome to enlist more help on simplifying the code base, so
here's some thoughts on how we can do that. I've been trying to encourage Vijay
too (so far unsuccessfully ;-) ) ... but in the long run it will make the code
easier for all of us to work with. =)

https://codereview.chromium.org/1899373002/diff/20001/lib/runtime/dart_sdk.js
File lib/runtime/dart_sdk.js (right):

https://codereview.chromium.org/1899373002/diff/20001/lib/runtime/dart_sdk.js...
lib/runtime/dart_sdk.js:4062: +      super.length = value;
On 2016/04/20 00:57:36, Harry Terkelsen wrote:
> it looks like ddc is smart enough to rewrite a['b'] into a.b. Neat!

yes we get this "for free" from js_ast code taken from dart2js

https://codereview.chromium.org/1899373002/diff/20001/lib/src/compiler/code_g...
File lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/1899373002/diff/20001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:79: HashSet<FieldElement>
_propertyOverrides;
if following my suggestion in the next file, this and _fieldsNeedingStorage
won't be needed :)

https://codereview.chromium.org/1899373002/diff/20001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:794: if (methodElement is
PropertyAccessorElement) {
it would be good to refactor this into a method

_emitSuperAccessorWrapper?

https://codereview.chromium.org/1899373002/diff/20001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:796: if
(_propertyOverrides.contains(fieldElement)) {
here we can just call make a call to ask if our property needed an override.

https://codereview.chromium.org/1899373002/diff/20001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:803: var fn = new JS.Fun(
this could be done a bit simpler:

var fn = js.call('function(#) { super.# = #; }', [value, name, value]);

In this case, we know there's no user variable named "value" in scope, so you
could even skip the temp:

var fn = js.call('function(value) { super.# = value; }', [name]);

https://codereview.chromium.org/1899373002/diff/20001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:808: var fn =
same here. if using js_ast, I find it's easier to just use it the maximum it
can.

BTW, the reason we need the `new JS.Method` explicitly is I didn't make a
`js.method` helper yet. But we can parse methods so it likely wouldn't be hard,
if we wanted to do that. That would simplify even further:

    jsMethods.add(js.method('#() { return super.#; }', name, name));

I think the "hard" part would be JS_AST parser allowing #-interpolation for the
method name.

https://codereview.chromium.org/1899373002/diff/20001/lib/src/compiler/js_fie...
File lib/src/compiler/js_field_storage.dart (right):

https://codereview.chromium.org/1899373002/diff/20001/lib/src/compiler/js_fie...
lib/src/compiler/js_field_storage.dart:10: /// We use a storage slot for fields
that override or can be overridden by
Update this comment?  And method name?

https://codereview.chromium.org/1899373002/diff/20001/lib/src/compiler/js_fie...
lib/src/compiler/js_field_storage.dart:12: void findFieldsNeedingStorage(
Oh wow!

So it looks like this old code was doing way more than it needed to! :)

The whole idea of visiting compilation units in a pre-pass was to do fancier
analysis. I thought it was ripped out but apparently not.

So I think you can radically simplify this...

Basically: just wait until we're going to emit a field (or a getter or a
setter). At that point, use checkPropertyOverride.

Then we don't need hash sets and stuff. It also keeps the logic more localized.

https://codereview.chromium.org/1899373002/diff/20001/lib/src/compiler/js_fie...
lib/src/compiler/js_field_storage.dart:63: // TODO(vsm): Get rid of redundant
check here.
+1 we should clean this up. Here's a refactor:


// just typed this in code review tool, please check for syntax and such :)
bool foundGetter = false;
bool foundSetter = false;

for (var superclass in superclasses) {
  // Stop if we reach a native type.
  if (extensionTypes.contains(superclass)) break;

  var superprop = getProperty(superclass, library, field.name);
  if (superprop == null) continue;

  var getter = superprop.getter;
  bool hasGetter = getter != null && !getter.isAbstract;
  if (hasGetter) foundGetter = true;

  var setter = superprop.setter;
  bool hasSetter =  setter != null && !setter.isAbstract;
  if (hasSetter) foundSetter = true;

  // Stop if this is an abstract getter/setter
  // TODO: why were we doing this?
  if (!hasGetter && !hasSetter) break;
}

// TODO: or wrap this in an class that stores the getter/setter elements. that
might be clean.
return new Tuple2<bool, bool>(foundGetter, foundSetter);


The result of the function is used by the caller. Therefore no "checkGetter"
"checkSetter" needs to be passed down. Nor "overrides"

https://codereview.chromium.org/1899373002/diff/20001/lib/src/compiler/js_fie...
lib/src/compiler/js_field_storage.dart:67: // TODO(vsm): Why do we need this?
this is not needed anymore. Please dead code remove :)

[Harry Terkelsen, 2016-04-20 23:33:28]

Thanks John, PTAL

https://codereview.chromium.org/1899373002/diff/20001/lib/runtime/dart_sdk.js
File lib/runtime/dart_sdk.js (right):

https://codereview.chromium.org/1899373002/diff/20001/lib/runtime/dart_sdk.js...
lib/runtime/dart_sdk.js:4062: +      super.length = value;
On 2016/04/20 21:16:39, John Messerly wrote:
> On 2016/04/20 00:57:36, Harry Terkelsen wrote:
> > it looks like ddc is smart enough to rewrite a['b'] into a.b. Neat!
> 
> yes we get this "for free" from js_ast code taken from dart2js

Acknowledged.

https://codereview.chromium.org/1899373002/diff/20001/lib/src/compiler/code_g...
File lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/1899373002/diff/20001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:79: HashSet<FieldElement>
_propertyOverrides;
On 2016/04/20 21:16:39, John Messerly wrote:
> if following my suggestion in the next file, this and _fieldsNeedingStorage
> won't be needed :)

Acknowledged.

https://codereview.chromium.org/1899373002/diff/20001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:794: if (methodElement is
PropertyAccessorElement) {
On 2016/04/20 21:16:39, John Messerly wrote:
> it would be good to refactor this into a method
> 
> _emitSuperAccessorWrapper?

Done.

https://codereview.chromium.org/1899373002/diff/20001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:803: var fn = new JS.Fun(
On 2016/04/20 21:16:39, John Messerly wrote:
> this could be done a bit simpler:
> 
> var fn = js.call('function(#) { super.# = #; }', [value, name, value]);
> 
> In this case, we know there's no user variable named "value" in scope, so you
> could even skip the temp:
> 
> var fn = js.call('function(value) { super.# = value; }', [name]);

Done.

https://codereview.chromium.org/1899373002/diff/20001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:808: var fn =
On 2016/04/20 21:16:39, John Messerly wrote:
> same here. if using js_ast, I find it's easier to just use it the maximum it
> can.
> 
> BTW, the reason we need the `new JS.Method` explicitly is I didn't make a
> `js.method` helper yet. But we can parse methods so it likely wouldn't be
hard,
> if we wanted to do that. That would simplify even further:
> 
>     jsMethods.add(js.method('#() { return super.#; }', name, name));
> 
> I think the "hard" part would be JS_AST parser allowing #-interpolation for
the
> method name.

Done.

https://codereview.chromium.org/1899373002/diff/20001/lib/src/compiler/js_fie...
File lib/src/compiler/js_field_storage.dart (right):

https://codereview.chromium.org/1899373002/diff/20001/lib/src/compiler/js_fie...
lib/src/compiler/js_field_storage.dart:10: /// We use a storage slot for fields
that override or can be overridden by
On 2016/04/20 21:16:40, John Messerly wrote:
> Update this comment?  And method name?

Done.

https://codereview.chromium.org/1899373002/diff/20001/lib/src/compiler/js_fie...
lib/src/compiler/js_field_storage.dart:12: void findFieldsNeedingStorage(
On 2016/04/20 21:16:40, John Messerly wrote:
> Oh wow!
> 
> So it looks like this old code was doing way more than it needed to! :)
> 
> The whole idea of visiting compilation units in a pre-pass was to do fancier
> analysis. I thought it was ripped out but apparently not.
> 
> So I think you can radically simplify this...
> 
> Basically: just wait until we're going to emit a field (or a getter or a
> setter). At that point, use checkPropertyOverride.
> 
> Then we don't need hash sets and stuff. It also keeps the logic more
localized.

Done.

https://codereview.chromium.org/1899373002/diff/20001/lib/src/compiler/js_fie...
lib/src/compiler/js_field_storage.dart:63: // TODO(vsm): Get rid of redundant
check here.
On 2016/04/20 21:16:40, John Messerly wrote:
> +1 we should clean this up. Here's a refactor:
> 
> 
> // just typed this in code review tool, please check for syntax and such :)
> bool foundGetter = false;
> bool foundSetter = false;
> 
> for (var superclass in superclasses) {
>   // Stop if we reach a native type.
>   if (extensionTypes.contains(superclass)) break;
> 
>   var superprop = getProperty(superclass, library, field.name);
>   if (superprop == null) continue;
> 
>   var getter = superprop.getter;
>   bool hasGetter = getter != null && !getter.isAbstract;
>   if (hasGetter) foundGetter = true;
> 
>   var setter = superprop.setter;
>   bool hasSetter =  setter != null && !setter.isAbstract;
>   if (hasSetter) foundSetter = true;
> 
>   // Stop if this is an abstract getter/setter
>   // TODO: why were we doing this?
>   if (!hasGetter && !hasSetter) break;
> }
> 
> // TODO: or wrap this in an class that stores the getter/setter elements. that
> might be clean.
> return new Tuple2<bool, bool>(foundGetter, foundSetter);
> 
> 
> The result of the function is used by the caller. Therefore no "checkGetter"
> "checkSetter" needs to be passed down. Nor "overrides"

Done.

https://codereview.chromium.org/1899373002/diff/20001/lib/src/compiler/js_fie...
lib/src/compiler/js_field_storage.dart:67: // TODO(vsm): Why do we need this?
On 2016/04/20 21:16:40, John Messerly wrote:
> this is not needed anymore. Please dead code remove :)

Done.

[Jennifer Messerly, 2016-04-21 00:53:08]

LGTM!

https://codereview.chromium.org/1899373002/diff/40001/lib/src/compiler/code_g...
File lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/1899373002/diff/40001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:45: //TODO(leafp): Remove deprecated
dependency
it looks like this comment got moved around in a sort?

https://codereview.chromium.org/1899373002/diff/40001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:926: if (!field.element.isSynthetic &&
I think these can't be synthetic, because we got them from the AST fields.
Perhaps make that an assert?

https://codereview.chromium.org/1899373002/diff/40001/lib/src/compiler/js_fie...
File lib/src/compiler/js_field_storage.dart (right):

https://codereview.chromium.org/1899373002/diff/40001/lib/src/compiler/js_fie...
lib/src/compiler/js_field_storage.dart:12: class PropertyOverrideResult {
nice!

https://codereview.chromium.org/1899373002/diff/40001/lib/src/compiler/js_fie...
lib/src/compiler/js_field_storage.dart:40: // TODO: why were we doing this?
feel free to make it `TODO(jmesserly)` :)

(I could help explain the issue to someone in the future)

[Harry Terkelsen, 2016-04-21 01:00:11]

thanks John!

https://codereview.chromium.org/1899373002/diff/40001/lib/src/compiler/code_g...
File lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/1899373002/diff/40001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:45: //TODO(leafp): Remove deprecated
dependency
On 2016/04/21 00:53:08, John Messerly wrote:
> it looks like this comment got moved around in a sort?

Done.

https://codereview.chromium.org/1899373002/diff/40001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:926: if (!field.element.isSynthetic &&
On 2016/04/21 00:53:08, John Messerly wrote:
> I think these can't be synthetic, because we got them from the AST fields.
> Perhaps make that an assert?

Done.

[Harry Terkelsen, 2016-04-21 01:08:13]

Description was changed from

==========
Emit forwarding getter/setter when overriding just a getter or setter.

Based on Vijay's change:
https://github.com/dart-lang/dev_compiler/commit/74dc4a6a808b271312aabe1bf463...

BUG=https://github.com/dart-lang/dev_compiler/issues/497
==========

to

==========
Emit forwarding getter/setter when overriding just a getter or setter.

Based on Vijay's change:
https://github.com/dart-lang/dev_compiler/commit/74dc4a6a808b271312aabe1bf463...

BUG=https://github.com/dart-lang/dev_compiler/issues/497
R=jmesserly@google.com

Committed:
https://github.com/dart-lang/dev_compiler/commit/1b461199920149e856272182187f...
==========

[Harry Terkelsen, 2016-04-21 01:08:14]

Committed patchset #5 (id:80001) manually as
1b461199920149e856272182187f506260f62cf5 (presubmit successful).