Add support for implicit getters and setters

Add support for implicit getters and setters

BUG=
R=kmillikin@google.com

Committed: https://github.com/dart-lang/sdk/commit/02251a05eb7ff45be9a809779b0356aecc7f4f3f

[zhivkag, 2017-03-21 16:42:20]

Description was changed from

==========
Implement support for implicit getters and setters for Objects


BUG=
==========

to

==========
Add support for implicit getters and setters

BUG=
==========

[zhivkag, 2017-03-21 16:42:43]

zhivkag@google.com changed reviewers:
+ kmillikin@google.com

[Kevin Millikin (Google), 2017-03-23 10:42:51]

I can see some obvious ways that we will want to change it soon, suggested
below.

What do you think?

https://codereview.chromium.org/2764073002/diff/20001/pkg/kernel/lib/interpre...
File pkg/kernel/lib/interpreter/interpreter.dart (right):

https://codereview.chromium.org/2764073002/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:123: ObjectValue receiver =
eval(node.receiver, env);
We have to support property get on things like int and String (.hashCode is an
obvious example).

I think that suggests that those classes have to be able to have getters and
setters too.

Perhaps it makes most sense to have a base Value class that has a superclass,
methods, getters, and setters, and then int, String, etc. are subclasses?

https://codereview.chromium.org/2764073002/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:124: return
receiver.propertyGet(node.name);
I suggest we factor this slightly differently:

1. Look up the getter by name on the receiver's class.
2. Invoke it, whatever it is.

That looks like:

return receiver.class.lookupGetter(node.name)(receiver);

https://codereview.chromium.org/2764073002/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:248: Class superClass;
We might not agree, but superclass is one word in both the Dart language spec
and in the Kernel AST implementation.  superClass ==> superclass.

https://codereview.chromium.org/2764073002/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:252: Map<Name, int> getters = <Name,
int>{};
Thinking ahead, this needs to map names to getters (not just the implicit
getters).

I'd change that here to have:

typedef Value Getter(Value);

and:

Map<Name, Getter> getters ...

https://codereview.chromium.org/2764073002/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:279: getters[f.name] = i;
So the getter function should be:

getters[f.name] = (Value receiver) => receiver.fields[i];

https://codereview.chromium.org/2764073002/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:287: Value propertyGet(ObjectValue
object, Name name) {
The name should be the verb form: getProperty.

https://codereview.chromium.org/2764073002/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:288: if (getters.containsKey(name))
{
We should look in the superclass for a getter.  If we factor this into first
looking up the getter and then invoking it, the lookup would be something like:

Getter lookupGetter(Name name) {
  Getter getter = getters[name];
  if (getter != null) return getter;
  if (superclass != null) return superclass.lookupGetter(name);
  return (Value receiver) => notImplemented(obj: name);
}

https://codereview.chromium.org/2764073002/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:295: Value propertySet(ObjectValue
object, Name name, Value value) {
Likewise, I's split this into looking up the setter:

typedef void Setter(Value receiver, Value value);

Map<Name, Setter> setters = ...;

Setter lookupSetter(Name name) {
  Setter setter = setters[name];
  if (setter != null) return setter;
  if (superclass != null) return superclass.lookupSetter(name);
  return (Value receiver, Value value) => ...
}

And invoking the setter in the interpreter:

Value visitPropertySet(PropertySet node, Environment env) {
  Value receiver = eval(node.receiver, env);
  Value value = eval(node.value, env);
  receiver.class.lookupSetter(node.name)(receiver, value);
  return value;
}

[zhivkag, 2017-03-23 13:50:39]

I applied the suggestions, thanks!  

There is one question regarding the return value of PropertySet evaluation, can
you take a look?

https://codereview.chromium.org/2764073002/diff/20001/pkg/kernel/lib/interpre...
File pkg/kernel/lib/interpreter/interpreter.dart (right):

https://codereview.chromium.org/2764073002/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:123: ObjectValue receiver =
eval(node.receiver, env);
On 2017/03/23 10:42:51, Kevin Millikin (Google) wrote:
> We have to support property get on things like int and String (.hashCode is an
> obvious example).
> 
> I think that suggests that those classes have to be able to have getters and
> setters too.
> 
> Perhaps it makes most sense to have a base Value class that has a superclass,
> methods, getters, and setters, and then int, String, etc. are subclasses?

Indeed, thanks! Done.

https://codereview.chromium.org/2764073002/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:124: return
receiver.propertyGet(node.name);
On 2017/03/23 10:42:50, Kevin Millikin (Google) wrote:
> I suggest we factor this slightly differently:
> 
> 1. Look up the getter by name on the receiver's class.
> 2. Invoke it, whatever it is.
> 
> That looks like:
> 
> return receiver.class.lookupGetter(node.name)(receiver);

Done.

https://codereview.chromium.org/2764073002/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:248: Class superClass;
On 2017/03/23 10:42:51, Kevin Millikin (Google) wrote:
> We might not agree, but superclass is one word in both the Dart language spec
> and in the Kernel AST implementation.  superClass ==> superclass.

Done.

https://codereview.chromium.org/2764073002/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:252: Map<Name, int> getters = <Name,
int>{};
On 2017/03/23 10:42:51, Kevin Millikin (Google) wrote:
> Thinking ahead, this needs to map names to getters (not just the implicit
> getters).
> 
> I'd change that here to have:
> 
> typedef Value Getter(Value);
> 
> and:
> 
> Map<Name, Getter> getters ...

Done.

https://codereview.chromium.org/2764073002/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:279: getters[f.name] = i;
On 2017/03/23 10:42:51, Kevin Millikin (Google) wrote:
> So the getter function should be:
> 
> getters[f.name] = (Value receiver) => receiver.fields[i];

Done.

https://codereview.chromium.org/2764073002/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:287: Value propertyGet(ObjectValue
object, Name name) {
On 2017/03/23 10:42:51, Kevin Millikin (Google) wrote:
> The name should be the verb form: getProperty.

Done.

https://codereview.chromium.org/2764073002/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:288: if (getters.containsKey(name))
{
On 2017/03/23 10:42:51, Kevin Millikin (Google) wrote:
> We should look in the superclass for a getter.  If we factor this into first
> looking up the getter and then invoking it, the lookup would be something
like:
> 
> Getter lookupGetter(Name name) {
>   Getter getter = getters[name];
>   if (getter != null) return getter;
>   if (superclass != null) return superclass.lookupGetter(name);
>   return (Value receiver) => notImplemented(obj: name);
> }

Makes sense, thanks! Done.

https://codereview.chromium.org/2764073002/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:295: Value propertySet(ObjectValue
object, Name name, Value value) {
On 2017/03/23 10:42:51, Kevin Millikin (Google) wrote:
> Likewise, I's split this into looking up the setter:
> 
> typedef void Setter(Value receiver, Value value);
> 
> Map<Name, Setter> setters = ...;
> 
> Setter lookupSetter(Name name) {
>   Setter setter = setters[name];
>   if (setter != null) return setter;
>   if (superclass != null) return superclass.lookupSetter(name);
>   return (Value receiver, Value value) => ...
> }
> 
> And invoking the setter in the interpreter:
> 
> Value visitPropertySet(PropertySet node, Environment env) {
>   Value receiver = eval(node.receiver, env);
>   Value value = eval(node.value, env);
>   receiver.class.lookupSetter(node.name)(receiver, value);
>   return value;
> }

Applied the suggestion but I have a doubt whether the setter should return
value. The type of setters being void, if we don't return value, does it make
sense to return Value.nullInstance instead?

[Kevin Millikin (Google), 2017-03-24 14:50:15]

LGTM.

[zhivkag, 2017-03-24 15:04:19]

Description was changed from

==========
Add support for implicit getters and setters

BUG=
==========

to

==========
Add support for implicit getters and setters

BUG=
R=kmillikin@google.com

Committed:
https://github.com/dart-lang/sdk/commit/02251a05eb7ff45be9a809779b0356aecc7f4f3f
==========

[zhivkag, 2017-03-24 15:04:20]

Committed patchset #4 (id:60001) manually as
02251a05eb7ff45be9a809779b0356aecc7f4f3f (presubmit successful).