emit 'this.x' when 'super.x' accesses a field

emit 'this.x' when 'super.x' accesses a field

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

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

[Harry Terkelsen, 2016-04-18 18:15:47]

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

[vsm, 2016-04-18 20:27:43]

https://codereview.chromium.org/1895903002/diff/1/lib/src/compiler/code_gener...
File lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/1895903002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:3059: member.getter.isSynthetic) {
Do we need this for setters as well?

[Harry Terkelsen, 2016-04-18 20:59:50]

thanks Vijay. PTAL

https://codereview.chromium.org/1895903002/diff/1/lib/src/compiler/code_gener...
File lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/1895903002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:3059: member.getter.isSynthetic) {
On 2016/04/18 20:27:43, vsm wrote:
> Do we need this for setters as well?

Doesn't synthetic getter imply synthetic setter? Actually, now that I think
about it, when we fix https://github.com/dart-lang/dev_compiler/issues/497 it
might be possible to have synthetic getters and implemented setters (the
synthetic getter is just a call to the super getter).

[Jennifer Messerly, 2016-04-18 21:10:12]

almost looks good to me :)

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

https://codereview.chromium.org/1895903002/diff/20001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:3045: JS.Expression _emitGet(Expression
target, SimpleIdentifier memberId) {
Suggestion: if you want to rename this "_emitProperty" or "_emitAccess" I think
that would be good too.

it is called as part of generating gets or sets, so the name is misleading :)

https://codereview.chromium.org/1895903002/diff/20001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:3060: member.setter.isSynthetic) {
Oops I didn't reply fast enough to the original thread :)
Here's what I was going to say in reply to Vijay:

I think the idea is that a field will always has an implied getter. We have
read-only fields (final int x = ...) but not write-only fields :). So checking
the getter is safe.

So the old code LGTM but not the new code :). I don't think a setter is
guaranteed to be there if you have a final field? Either way would be good to
add a test for it.

if it helps I can poke around in Analyzer and see if they have a better way of
seeing what is an "actual field". I'm a little worried if the `isSynthetic` bit
is reliable or if the check is necessary. But maybe it is.

[Jennifer Messerly, 2016-04-18 21:20:13]

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

https://codereview.chromium.org/1895903002/diff/20001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:3060: member.setter.isSynthetic) {
On 2016/04/18 21:10:12, John Messerly wrote:
> Oops I didn't reply fast enough to the original thread :)
> Here's what I was going to say in reply to Vijay:
> 
> I think the idea is that a field will always has an implied getter. We have
> read-only fields (final int x = ...) but not write-only fields :). So checking
> the getter is safe.
> 
> So the old code LGTM but not the new code :). I don't think a setter is
> guaranteed to be there if you have a final field? Either way would be good to
> add a test for it.
> 
> if it helps I can poke around in Analyzer and see if they have a better way of
> seeing what is an "actual field". I'm a little worried if the `isSynthetic`
bit
> is reliable or if the check is necessary. But maybe it is.

Ah, so you should be able to just check `!member.isSynthetic` -- in other words,
check that the field itself is not synthetic. My grepping through Analyzer
(specifically pkg/analyzer/lib/src/dart/element/builder.dart) leads me to
believe that should work :)

[Harry Terkelsen, 2016-04-18 21:28:58]

Thanks!

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

https://codereview.chromium.org/1895903002/diff/20001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:3045: JS.Expression _emitGet(Expression
target, SimpleIdentifier memberId) {
On 2016/04/18 21:10:12, John Messerly wrote:
> Suggestion: if you want to rename this "_emitProperty" or "_emitAccess" I
think
> that would be good too.
> 
> it is called as part of generating gets or sets, so the name is misleading :)

Done.

https://codereview.chromium.org/1895903002/diff/20001/lib/src/compiler/code_g...
lib/src/compiler/code_generator.dart:3060: member.setter.isSynthetic) {
On 2016/04/18 21:10:12, John Messerly wrote:
> Oops I didn't reply fast enough to the original thread :)
> Here's what I was going to say in reply to Vijay:
> 
> I think the idea is that a field will always has an implied getter. We have
> read-only fields (final int x = ...) but not write-only fields :). So checking
> the getter is safe.
> 
> So the old code LGTM but not the new code :). I don't think a setter is
> guaranteed to be there if you have a final field? Either way would be good to
> add a test for it.
> 
> if it helps I can poke around in Analyzer and see if they have a better way of
> seeing what is an "actual field". I'm a little worried if the `isSynthetic`
bit
> is reliable or if the check is necessary. But maybe it is.

On final fields, setter is null. I'll just check if member is synthetic per your
suggestion. I also added a test.

[Harry Terkelsen, 2016-04-18 21:44:27]

Description was changed from

==========
emit 'this.x' when 'super.x' accesses a field

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

to

==========
emit 'this.x' when 'super.x' accesses a field

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

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

[Harry Terkelsen, 2016-04-18 21:44:29]

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

[Jennifer Messerly, 2016-04-18 21:51:31]

LGTM. By the way, did you understand the diff to:
https://codereview.chromium.org/download/issue1895903002_40001_50002.diff
?

a lot of code seems to have changed, I don't understand why.

[Harry Terkelsen, 2016-04-18 21:55:54]

On 2016/04/18 21:51:31, John Messerly wrote:
> LGTM. By the way, did you understand the diff to:
> https://codereview.chromium.org/download/issue1895903002_40001_50002.diff
> ?
> 
> a lot of code seems to have changed, I don't understand why.

Yeah, it looks like it was just reordered, and my two tests were added in. I'm
not sure why there was so much churn, I just figured it was normal when new
tests were added that this file would have a large change.

[Harry Terkelsen, 2016-04-18 22:02:36]

On 2016/04/18 21:55:54, Harry Terkelsen wrote:
> On 2016/04/18 21:51:31, John Messerly wrote:
> > LGTM. By the way, did you understand the diff to:
> > https://codereview.chromium.org/download/issue1895903002_40001_50002.diff
> > ?
> > 
> > a lot of code seems to have changed, I don't understand why.
> 
> Yeah, it looks like it was just reordered, and my two tests were added in. I'm
> not sure why there was so much churn, I just figured it was normal when new
> tests were added that this file would have a large change.

It looks like the reorderings are mostly rearranging things with underscores in
the name. The code that generates language-all.js is:

cat test/codegen/expect/language/*.js > test/codegen/expect/language-all.js

So maybe it has to do with some setting that sorts names with underscores
differently from system to system?