Add support for execution of constructor body

Add support for execution of constructor body

BUG=
R=dmitryas@google.com

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

[zhivkag, 2017-05-15 14:24:13]

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

[zhivkag, 2017-05-15 14:24:13]

Hi, 

This change adds support for execution of the body of constructors. It also
makes the newly allocated object accessible in the intializer list, since this
is allowed in Kernel.

Thanks, 
Zhivka

[Dmitry Stefantsov, 2017-05-16 08:07:50]

I like it! However, there is something I'd like to discuss before we land it.
Please, find the comments below.

https://codereview.chromium.org/2880343002/diff/1/pkg/kernel/lib/interpreter/...
File pkg/kernel/lib/interpreter/interpreter.dart (right):

https://codereview.chromium.org/2880343002/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:46: abstract class Environment {
I'd rather handle the case of 'this' with a couple of new methods of
Environments, than creating a hierarchy for Environments.

We may forbid 'this' as a variable name in existing 'lookup', 'assign', and
'expand' methods and create methods 'lookupThisBinding()' with no arguments and
'expandThis(value)' that takes an object value. It will guarantee the special
treatment of 'this' and that it is unique across the environment branch. It will
also help to preserve the universal treatment of environments.

I just think that keeping different kinds of environments may be hard to
maintain (once we have global variables, InstanceENvironments should have a
StaticEnvironment as a parent, which we can't do now with the introduced
hierarchy) and hard to describe in terms of operational semantics.

https://codereview.chromium.org/2880343002/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:99: get thisInstance =>
_thisInstance ?? parent.thisInstance;
Just in case we decide to go with environment class hierarchy, I guess it may be
"parent?.thisInstance", to avoid null dereferencing in case no
InstanceEnvironment in the list having a "_thisInstance".

https://codereview.chromium.org/2880343002/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:200: newObject, node.target,
nextConfig);
I think it's better to keep 'config.continuation' here. The point is that
constructor invocation produces a value, and  'config.continuation' is an
expression continuation, i.e. something that is expecting a value.
Theoretically, they are supposed to be used together without intermediate
configurations.

https://codereview.chromium.org/2880343002/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:366: class NewInstanceConfiguration
extends StatementConfiguration {
In case we decide to keep the intermediate configuration between constructor
invocation and the expression continuation that expects the created object, I
don't see why we can't use 'ContinuationConfiguration' instead of
'NewInstanceConfiguration'.

https://codereview.chromium.org/2880343002/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:561: final StatementConfiguration
configuration;
I think it's better to keep 'ExpressionContinuation' instead of
'StatementConfiguration' here. Also, please, see the comments above and below on
the same issue.

https://codereview.chromium.org/2880343002/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:596:
.withConfiguration(configuration);
Here we might have used 'withExpressionContinuation' and pass previously
existing field 'expressionContinuation' to it.

https://codereview.chromium.org/2880343002/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:601: new
StatementConfiguration(constructor.function.body, newState);
Here we may assume that the body of constructor has implicit 'return this'
statement that returns the value to the 'returnContinuation'.

[Dmitry Stefantsov, 2017-05-16 09:59:24]

https://codereview.chromium.org/2880343002/diff/1/pkg/kernel/lib/interpreter/...
File pkg/kernel/lib/interpreter/interpreter.dart (right):

https://codereview.chromium.org/2880343002/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:366: class NewInstanceConfiguration
extends StatementConfiguration {
On 2017/05/16 08:07:50, Dmitry Stefantsov wrote:
> In case we decide to keep the intermediate configuration between constructor
> invocation and the expression continuation that expects the created object, I
> don't see why we can't use 'ContinuationConfiguration' instead of
> 'NewInstanceConfiguration'.

Thank you for the explanation offline about the 'NewInstanceConfiguration'! Now
I see that we really need something like this, because it basically acts as the
missing 'return' statement at the end of the constructor body.

One additional consideration is that we may actually lookup 'this' in the
current environment here and pass the value to the continuation, rather than
keeping the object that will be mutated somewhere between the invocations of the
'NewInstanceConfiguration()' constructor and the 'step()' method.

[zhivkag, 2017-05-16 10:51:14]

Thanks for the comments, PTAL!

https://codereview.chromium.org/2880343002/diff/1/pkg/kernel/lib/interpreter/...
File pkg/kernel/lib/interpreter/interpreter.dart (right):

https://codereview.chromium.org/2880343002/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:46: abstract class Environment {
On 2017/05/16 08:07:50, Dmitry Stefantsov wrote:
> I'd rather handle the case of 'this' with a couple of new methods of
> Environments, than creating a hierarchy for Environments.
> 
> We may forbid 'this' as a variable name in existing 'lookup', 'assign', and
> 'expand' methods and create methods 'lookupThisBinding()' with no arguments
and
> 'expandThis(value)' that takes an object value. It will guarantee the special
> treatment of 'this' and that it is unique across the environment branch. It
will
> also help to preserve the universal treatment of environments.
> 
> I just think that keeping different kinds of environments may be hard to
> maintain (once we have global variables, InstanceENvironments should have a
> StaticEnvironment as a parent, which we can't do now with the introduced
> hierarchy) and hard to describe in terms of operational semantics.

I see the limitations the previous hierarchy introduced, I changed to a simpler
approach that should be isomorphic to adding some new methods in the existing
environment, what do you think?

https://codereview.chromium.org/2880343002/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:99: get thisInstance =>
_thisInstance ?? parent.thisInstance;
On 2017/05/16 08:07:50, Dmitry Stefantsov wrote:
> Just in case we decide to go with environment class hierarchy, I guess it may
be
> "parent?.thisInstance", to avoid null dereferencing in case no
> InstanceEnvironment in the list having a "_thisInstance".

Done.

https://codereview.chromium.org/2880343002/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:200: newObject, node.target,
nextConfig);
On 2017/05/16 08:07:50, Dmitry Stefantsov wrote:
> I think it's better to keep 'config.continuation' here. The point is that
> constructor invocation produces a value, and  'config.continuation' is an
> expression continuation, i.e. something that is expecting a value.
> Theoretically, they are supposed to be used together without intermediate
> configurations.

Acknowledged.

https://codereview.chromium.org/2880343002/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:366: class NewInstanceConfiguration
extends StatementConfiguration {
On 2017/05/16 09:59:24, Dmitry Stefantsov wrote:
> On 2017/05/16 08:07:50, Dmitry Stefantsov wrote:
> > In case we decide to keep the intermediate configuration between constructor
> > invocation and the expression continuation that expects the created object,
I
> > don't see why we can't use 'ContinuationConfiguration' instead of
> > 'NewInstanceConfiguration'.
> 
> Thank you for the explanation offline about the 'NewInstanceConfiguration'!
Now
> I see that we really need something like this, because it basically acts as
the
> missing 'return' statement at the end of the constructor body.
> 
> One additional consideration is that we may actually lookup 'this' in the
> current environment here and pass the value to the continuation, rather than
> keeping the object that will be mutated somewhere between the invocations of
the
> 'NewInstanceConfiguration()' constructor and the 'step()' method.

I assume some of the other comments were clarified by our discussion and are
related to this one, so I haven't addressed them (just ACKed), please correct me
if I am wrong.

https://codereview.chromium.org/2880343002/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:561: final StatementConfiguration
configuration;
On 2017/05/16 08:07:50, Dmitry Stefantsov wrote:
> I think it's better to keep 'ExpressionContinuation' instead of
> 'StatementConfiguration' here. Also, please, see the comments above and below
on
> the same issue.

Acknowledged.

https://codereview.chromium.org/2880343002/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:596:
.withConfiguration(configuration);
On 2017/05/16 08:07:50, Dmitry Stefantsov wrote:
> Here we might have used 'withExpressionContinuation' and pass previously
> existing field 'expressionContinuation' to it.

Acknowledged.

https://codereview.chromium.org/2880343002/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:601: new
StatementConfiguration(constructor.function.body, newState);
On 2017/05/16 08:07:50, Dmitry Stefantsov wrote:
> Here we may assume that the body of constructor has implicit 'return this'
> statement that returns the value to the 'returnContinuation'.

Acknowledged.

[Dmitry Stefantsov, 2017-05-16 11:06:52]

I mostly agree with the changes, but have one important comment that I think
should be considered.

https://codereview.chromium.org/2880343002/diff/1/pkg/kernel/lib/interpreter/...
File pkg/kernel/lib/interpreter/interpreter.dart (right):

https://codereview.chromium.org/2880343002/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:46: abstract class Environment {
On 2017/05/16 10:51:13, zhivkag wrote:
> On 2017/05/16 08:07:50, Dmitry Stefantsov wrote:
> > I'd rather handle the case of 'this' with a couple of new methods of
> > Environments, than creating a hierarchy for Environments.
> > 
> > We may forbid 'this' as a variable name in existing 'lookup', 'assign', and
> > 'expand' methods and create methods 'lookupThisBinding()' with no arguments
> and
> > 'expandThis(value)' that takes an object value. It will guarantee the
special
> > treatment of 'this' and that it is unique across the environment branch. It
> will
> > also help to preserve the universal treatment of environments.
> > 
> > I just think that keeping different kinds of environments may be hard to
> > maintain (once we have global variables, InstanceENvironments should have a
> > StaticEnvironment as a parent, which we can't do now with the introduced
> > hierarchy) and hard to describe in terms of operational semantics.
> 
> I see the limitations the previous hierarchy introduced, I changed to a
simpler
> approach that should be isomorphic to adding some new methods in the existing
> environment, what do you think?

To me, additional methods look simpler and easier to translate to
functional-programming style. But this solution is also ok.

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

https://codereview.chromium.org/2880343002/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:672: final Environment
initializerEnvironment;
Having two environments in the same continuation really feels wrong. I think it
should be rewritten.

[Dmitry Stefantsov, 2017-05-18 12:56:51]

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

https://codereview.chromium.org/2880343002/diff/80001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:561: new
NewInstanceConfiguration(continuation, newObject));
I think this statement would be hard to translate to a CEK machine transition.
The reason is that here we assume that 'newObject' is mutable. We pass the
reference to 'NewInstanceConfiguration', and it will return an object that is
referenced by 'newObject', and it will be initialized at that point.

My plan of how we may avoid it is not to create 'NewInstanceConfiguration' here,
but rather delay it until the moment when 'newObject' is fully initialized. Here
we may just pass the necessary part, i.e. 'continuation'.

[Dmitry Stefantsov, 2017-05-18 13:26:07]

After a discussion it turned out that avoiding mutations is a big change and is
better done in a separate CL. And besides the mutations this CL LGTM.

[zhivkag, 2017-05-18 13:30:32]

Description was changed from

==========
Add support for execution of constructor body

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

to

==========
Add support for execution of constructor body

BUG=
R=dmitryas@google.com

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

[zhivkag, 2017-05-18 13:30:33]

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