Implement support for variables and evaluation of logic expressions

Implements support for variables and evaluation of logic expressions

BUG=
R=kmillikin@google.com

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

[zhivkag, 2017-03-08 09:22:42]

Description was changed from

==========
Implements support for variables and evaluation of logic expressions

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

to

==========
Implements support for variables and evaluation of logic expressions

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

[zhivkag, 2017-03-08 09:26:03]

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

[Kevin Millikin (Google), 2017-03-15 10:01:24]

LGTM.

I have tons of comments, but you should interpret most of them as suggestions
and wait to address them after committing this change and the ones that depend
on it.

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

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:21: void evalProgram() {
Program's don't produce values, and maybe we should reserve the term 'evaluate'
for finding something's value.  evalProgram ==> runProgram?

Perhaps program is implied (it's not an argument), so I'd actually just call
this 'run()'.

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:25: // Evaluates only
ExpressionStatements and VariableDeclarations in the top
Evaluates ==> Executes.

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:28: var exprEval = new
ExpressionEval();
So far ExpressionEval doesn't have any state, so I think it would be more
obvious to make it a member of class Interpreter.

I think expression is implied by evaluator and I prefer to avoid abbreviations. 
exprEval -> evaluator.

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:33: s.expression.accept1(exprEval,
env);
It reads more nicely to have a method

Value eval(Expression expr, Environment env) => expr.accept1(this, env);

in class ExpressionEval, and not call accept1 directly.

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:57: class Binding {
Ultimately we should have really detailed comments for classes like this.  I
think it's fine to leave them until later because things will change, but we
should plan to spend some time just writing comments as documentation.

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:66: Environment parent;
final Environment parent;

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:71: Binding
lookupBinding(VariableDeclaration variable) {
I'd make this the recursive function (and never return null):

assert(contains(variable))
for (var b in bindings.reversed) {
  if (identical(b.variable, variable)) return b;
}
return parent.lookup(variable);

I'd also use identical here and in other places where we mean 'the same object'
to dodge any questions about Dart's overridable operator==.

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:78: Value lookup(VariableDeclaration
variable) {
Then this is:

return lookupBinding(variable).value;

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:84: void assign(VariableDeclaration
variable, Value value) {
And this is:

lookupBinding(variable).value = value;

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:96: if (lookupBinding(variable) !=
null) {
Variable declarations should be unique.  We could just
assert(!contains(variable)), if we implemented contains.

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:103: class ExpressionEval extends
ExpressionVisitor1<Value> {
class Evaluator?

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:113: Value
visitVariableGet(VariableGet node, arg) {
arg ==> env.

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:126: return null;
Not null, but new NullValue().

We should make canonical instances for null, true, and false.  One way to do
that is to have them as static fields in class Value so here it is:

return Value.nullInstance;

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:134: return new
BoolValue(!node.operand.accept1(this, arg).asBool);
I think I'd do the boolean conversion a little differently.  It's hard for the
reader to look at all the asBool overrides to see exactly what function is being
implemented.  I would also make it return a Value, not a Dart bool.  I wouldn't
use the word 'as' in the name, because Dart's as throws if the operand doesn't
have the required type, and this is a total function.

Suggest, in class Value:

BoolValue toBoolean() {
  return identical(this, Value.trueInstance)
      ? Value.trueInstance
      : Value.falseInstance;
}

Here, maybe:

var operand = eval(node.operand, env).toBoolean();
return identical(operand, Value.trueInstance)
    ? Value.falseInstance
    : Value.trueInstance;

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:140: if ('||' == node.operator) {
We might want a more literal interpretation of the spec for expression
evaluation, which describes dispatching on the operator before evaluating the
left-hand side:

if (node.operator == '||') {
  var left = eval(node.left, env).toBoolean();
  if (identical(left, Value.trueInstance)) return Value.trueInstance;
  return eval(node.right, env).toBoolean();
}

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:178: var letEnv = new
Environment(arg);
It might be more clear to name the value instead of the new environment:

var value = eval(node.variable.initializer, env);
return eval(node.body, new Environment(env).expand(node.variable, value));

[zhivkag, 2017-03-15 11:48:53]

I applied some of the suggestions in comments and will address the remaining in
another CL.

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

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:21: void evalProgram() {
On 2017/03/15 10:01:23, Kevin Millikin (Google) wrote:
> Program's don't produce values, and maybe we should reserve the term
'evaluate'
> for finding something's value.  evalProgram ==> runProgram?
> 
> Perhaps program is implied (it's not an argument), so I'd actually just call
> this 'run()'.

Done.

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:25: // Evaluates only
ExpressionStatements and VariableDeclarations in the top
On 2017/03/15 10:01:23, Kevin Millikin (Google) wrote:
> Evaluates ==> Executes.

Done.

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:28: var exprEval = new
ExpressionEval();
On 2017/03/15 10:01:24, Kevin Millikin (Google) wrote:
> So far ExpressionEval doesn't have any state, so I think it would be more
> obvious to make it a member of class Interpreter.
> 
> I think expression is implied by evaluator and I prefer to avoid
abbreviations. 
> exprEval -> evaluator.

Done.

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:33: s.expression.accept1(exprEval,
env);
On 2017/03/15 10:01:23, Kevin Millikin (Google) wrote:
> It reads more nicely to have a method
> 
> Value eval(Expression expr, Environment env) => expr.accept1(this, env);
> 
> in class ExpressionEval, and not call accept1 directly.

Done.

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:57: class Binding {
On 2017/03/15 10:01:24, Kevin Millikin (Google) wrote:
> Ultimately we should have really detailed comments for classes like this.  I
> think it's fine to leave them until later because things will change, but we
> should plan to spend some time just writing comments as documentation.

Acknowledged.

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:66: Environment parent;
On 2017/03/15 10:01:24, Kevin Millikin (Google) wrote:
> final Environment parent;

Done.

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:71: Binding
lookupBinding(VariableDeclaration variable) {
On 2017/03/15 10:01:23, Kevin Millikin (Google) wrote:
> I'd make this the recursive function (and never return null):
> 
> assert(contains(variable))
> for (var b in bindings.reversed) {
>   if (identical(b.variable, variable)) return b;
> }
> return parent.lookup(variable);
> 
> I'd also use identical here and in other places where we mean 'the same
object'
> to dodge any questions about Dart's overridable operator==.

Done.

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:78: Value lookup(VariableDeclaration
variable) {
On 2017/03/15 10:01:23, Kevin Millikin (Google) wrote:
> Then this is:
> 
> return lookupBinding(variable).value;

Done.

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:84: void assign(VariableDeclaration
variable, Value value) {
On 2017/03/15 10:01:23, Kevin Millikin (Google) wrote:
> And this is:
> 
> lookupBinding(variable).value = value;

Done.

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:96: if (lookupBinding(variable) !=
null) {
On 2017/03/15 10:01:23, Kevin Millikin (Google) wrote:
> Variable declarations should be unique.  We could just
> assert(!contains(variable)), if we implemented contains.

Done.

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:103: class ExpressionEval extends
ExpressionVisitor1<Value> {
On 2017/03/15 10:01:24, Kevin Millikin (Google) wrote:
> class Evaluator?

Done.

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:113: Value
visitVariableGet(VariableGet node, arg) {
On 2017/03/15 10:01:24, Kevin Millikin (Google) wrote:
> arg ==> env.

Done.

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:126: return null;
On 2017/03/15 10:01:23, Kevin Millikin (Google) wrote:
> Not null, but new NullValue().
> 
> We should make canonical instances for null, true, and false.  One way to do
> that is to have them as static fields in class Value so here it is:
> 
> return Value.nullInstance;

Acknowledged.

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:134: return new
BoolValue(!node.operand.accept1(this, arg).asBool);
On 2017/03/15 10:01:23, Kevin Millikin (Google) wrote:
> I think I'd do the boolean conversion a little differently.  It's hard for the
> reader to look at all the asBool overrides to see exactly what function is
being
> implemented.  I would also make it return a Value, not a Dart bool.  I
wouldn't
> use the word 'as' in the name, because Dart's as throws if the operand doesn't
> have the required type, and this is a total function.
> 
> Suggest, in class Value:
> 
> BoolValue toBoolean() {
>   return identical(this, Value.trueInstance)
>       ? Value.trueInstance
>       : Value.falseInstance;
> }
> 
> Here, maybe:
> 
> var operand = eval(node.operand, env).toBoolean();
> return identical(operand, Value.trueInstance)
>     ? Value.falseInstance
>     : Value.trueInstance;

Acknowledged.

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:140: if ('||' == node.operator) {
On 2017/03/15 10:01:24, Kevin Millikin (Google) wrote:
> We might want a more literal interpretation of the spec for expression
> evaluation, which describes dispatching on the operator before evaluating the
> left-hand side:
> 
> if (node.operator == '||') {
>   var left = eval(node.left, env).toBoolean();
>   if (identical(left, Value.trueInstance)) return Value.trueInstance;
>   return eval(node.right, env).toBoolean();
> }

Acknowledged.

https://codereview.chromium.org/2740433006/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:178: var letEnv = new
Environment(arg);
On 2017/03/15 10:01:23, Kevin Millikin (Google) wrote:
> It might be more clear to name the value instead of the new environment:
> 
> var value = eval(node.variable.initializer, env);
> return eval(node.body, new Environment(env).expand(node.variable, value));

Acknowledged.

[zhivkag, 2017-03-15 11:51:19]

Description was changed from

==========
Implements support for variables and evaluation of logic expressions

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

to

==========
Implements support for variables and evaluation of logic expressions

BUG=
R=kmillikin@google.com

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

[zhivkag, 2017-03-15 11:51:22]

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