Remove virtualField helper, just emit getters/setters in codegen.

Remove virtualField helper, just emit getters/setters in codegen.

Also fixes problems with final field overrides, which only override
the getter, not the setter.

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

Committed: https://github.com/dart-lang/dev_compiler/commit/7e8d226163429e7cce08dcf9de2581b985bbb76a

[Harry Terkelsen, 2016-04-25 22:04:05]

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

[Jennifer Messerly, 2016-04-25 22:47:05]

Wow, this is headed in an awesome direction! <3 the simplifications around
removing virtual field!

A few comments, mostly style. Only ones that seemed worth addressing are the
initializer issue & if we can simplify away the hashmaps.

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

https://codereview.chromium.org/1918033002/diff/40001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:84: final _virtualFields = new
HashMap<VariableDeclaration, JS.TemporaryId>();
Generally we don't use AST nodes as hash keys. Shouldn't we be able to not visit
this AST node twice?

(if it's unavoidable, the Element is more typically used to uniquely identify
something.)

https://codereview.chromium.org/1918033002/diff/40001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:85: final _virtualFieldSymbols = new
HashMap<ClassElement, List<JS.Statement>>();
do we need this map? could we simply keep a List<JS.Statement> scoped to the
class? I can't imagine this would be needed outside of visitClassDeclaration's
call tree

https://codereview.chromium.org/1918033002/diff/40001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:538: className = new
JS.Identifier(classElem.name);
there was a comment below:
"// Generic classes will be defined inside ..."

can you move it up here? it goes with this line.

https://codereview.chromium.org/1918033002/diff/40001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:770: ClassElement classElem) {
This parameter isn't needed.

See the very next line `var element = node.element` :)

Feel free to rename "element" to "classElem" or vice versa in the other method,
if it makes the code more clear.

https://codereview.chromium.org/1918033002/diff/40001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:809: if (propertyOverrideResult.foundGetter
||
consider: a shorter name here.

"overrideInfo" or "overrideResult" might read nicely:

if (overrideInfo.foundGetter || overrideInfo.foundSetter) {
 ...
}

https://codereview.chromium.org/1918033002/diff/40001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:812: _emitVirtualFieldAccessor(field, type,
className, classElem));
fyi -- you probably don't need to pass classElem or type here.

Fields can find their class pretty easily via `.enclosingElement` and from the
class you can get the type via `.type`, for example:

var fieldElem = fieldNode.element;
var classElem = fieldElem.enclosingElement;
var classType = classElem.type;

in general, it's nice to avoid extra parameters when possible, as it makes the
API easier to use and prevents bugs where the wrong thing was passed down.

https://codereview.chromium.org/1918033002/diff/40001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:850: 'function(value) {'
style comment: triple quote style is nicer for these long ones, IMO

var setter = js.call('''
  function(value) {
    ...
  }
''');

that way we can edit without worrying about all of the quote symbols

https://codereview.chromium.org/1918033002/diff/40001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:852: '  if (f === undefined) {'
Hmmmm, I'm a little confused about what's going on here.

Is this for the field initializer (that we generate in the JS `constructor()`?

If so, the best way to handle that is to have it use your virtualField symbol
storage slot directly:

constructor() {
  this[... virtual field symbol ] = ...;
}


Should be a pretty straightforward fix in _initializeFields.


Otherwise I don't think it will work -- we treat "null" and "undefined" as
synonyms, so you could hit a bug:


class B { int get x => _x; set x(int v) => print(v); }

void returnsNull() {} // but really returns JS undefined.
class D extends B { final int x = returnsNull(); }

main() {
  dynamic d = new D();
  d.x = 42; // oops! this sets d.x instead of printing "42" 
}

[Harry Terkelsen, 2016-04-26 18:29:27]

thanks John, PTAL!

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

https://codereview.chromium.org/1918033002/diff/40001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:84: final _virtualFields = new
HashMap<VariableDeclaration, JS.TemporaryId>();
On 2016/04/25 22:47:04, John Messerly wrote:
> Generally we don't use AST nodes as hash keys. Shouldn't we be able to not
visit
> this AST node twice?
> 
> (if it's unavoidable, the Element is more typically used to uniquely identify
> something.)

Done.

https://codereview.chromium.org/1918033002/diff/40001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:85: final _virtualFieldSymbols = new
HashMap<ClassElement, List<JS.Statement>>();
On 2016/04/25 22:47:04, John Messerly wrote:
> do we need this map? could we simply keep a List<JS.Statement> scoped to the
> class? I can't imagine this would be needed outside of visitClassDeclaration's
> call tree

Done.

https://codereview.chromium.org/1918033002/diff/40001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:538: className = new
JS.Identifier(classElem.name);
On 2016/04/25 22:47:04, John Messerly wrote:
> there was a comment below:
> "// Generic classes will be defined inside ..."
> 
> can you move it up here? it goes with this line.

Done.

https://codereview.chromium.org/1918033002/diff/40001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:770: ClassElement classElem) {
On 2016/04/25 22:47:04, John Messerly wrote:
> This parameter isn't needed.
> 
> See the very next line `var element = node.element` :)
> 
> Feel free to rename "element" to "classElem" or vice versa in the other
method,
> if it makes the code more clear.

Done.

https://codereview.chromium.org/1918033002/diff/40001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:809: if (propertyOverrideResult.foundGetter
||
On 2016/04/25 22:47:04, John Messerly wrote:
> consider: a shorter name here.
> 
> "overrideInfo" or "overrideResult" might read nicely:
> 
> if (overrideInfo.foundGetter || overrideInfo.foundSetter) {
>  ...
> }

Done.

https://codereview.chromium.org/1918033002/diff/40001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:812: _emitVirtualFieldAccessor(field, type,
className, classElem));
On 2016/04/25 22:47:04, John Messerly wrote:
> fyi -- you probably don't need to pass classElem or type here.
> 
> Fields can find their class pretty easily via `.enclosingElement` and from the
> class you can get the type via `.type`, for example:
> 
> var fieldElem = fieldNode.element;
> var classElem = fieldElem.enclosingElement;
> var classType = classElem.type;
> 
> in general, it's nice to avoid extra parameters when possible, as it makes the
> API easier to use and prevents bugs where the wrong thing was passed down.

Done.

https://codereview.chromium.org/1918033002/diff/40001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:850: 'function(value) {'
On 2016/04/25 22:47:04, John Messerly wrote:
> style comment: triple quote style is nicer for these long ones, IMO
> 
> var setter = js.call('''
>   function(value) {
>     ...
>   }
> ''');
> 
> that way we can edit without worrying about all of the quote symbols

Done.

https://codereview.chromium.org/1918033002/diff/40001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:852: '  if (f === undefined) {'
On 2016/04/25 22:47:04, John Messerly wrote:
> Hmmmm, I'm a little confused about what's going on here.
> 
> Is this for the field initializer (that we generate in the JS `constructor()`?
> 
> If so, the best way to handle that is to have it use your virtualField symbol
> storage slot directly:
> 
> constructor() {
>   this[... virtual field symbol ] = ...;
> }
> 
> 
> Should be a pretty straightforward fix in _initializeFields.
> 
> 
> Otherwise I don't think it will work -- we treat "null" and "undefined" as
> synonyms, so you could hit a bug:
> 
> 
> class B { int get x => _x; set x(int v) => print(v); }
> 
> void returnsNull() {} // but really returns JS undefined.
> class D extends B { final int x = returnsNull(); }
> 
> main() {
>   dynamic d = new D();
>   d.x = 42; // oops! this sets d.x instead of printing "42" 
> }
> 

Done.

[Jennifer Messerly, 2016-04-26 19:39:11]

LGTM!

[Harry Terkelsen, 2016-04-26 19:39:51]

Description was changed from

==========
Remove virtualField helper, just emit getters/setters in codegen.

Also fixes problems with final field overrides, which only override
the getter, not the setter.

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

to

==========
Remove virtualField helper, just emit getters/setters in codegen.

Also fixes problems with final field overrides, which only override
the getter, not the setter.

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

Committed:
https://github.com/dart-lang/dev_compiler/commit/7e8d226163429e7cce08dcf9de25...
==========

[Harry Terkelsen, 2016-04-26 19:39:53]

Committed patchset #5 (id:80001) manually as
7e8d226163429e7cce08dcf9de2581b985bbb76a (presubmit successful).