Implement support for static accessors

Implement support for static accessors

- Evaluation of StaticPropertyGet expression by reading a value stored in
the main environment, execution of static getter or creating a method tear off.
- Evaluation of StaticPropertySet expression by modifying the value
stored in main environment or execution of static setter.

BUG=
R=dmitryas@google.com, kmillikin@google.com

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

[zhivkag, 2017-08-09 07:06:08]

Description was changed from

==========
Add support for static accessors

Static accessors are executed by:
 - Reading or assigning the value stored in a location for
fields.
 - Executing the body of static setter or getter.
 - Creating a function value for a static method tear off.

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

to

==========
Implement support for static accessors

- Evaluation of StaticPropertyGet expression by reading a value stored in 
the main environment, execution of static getter or creating a method tear off.
 - Evaluation of StaticPropertySet expression by modifying the value 
stored in main environment or execution of static setter.

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

[zhivkag, 2017-08-09 07:06:24]

Description was changed from

==========
Implement support for static accessors

- Evaluation of StaticPropertyGet expression by reading a value stored in 
the main environment, execution of static getter or creating a method tear off.
 - Evaluation of StaticPropertySet expression by modifying the value 
stored in main environment or execution of static setter.

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

to

==========
Implement support for static accessors

- Evaluation of StaticPropertyGet expression by reading a value stored in 
the main environment, execution of static getter or creating a method tear off.
- Evaluation of StaticPropertySet expression by modifying the value 
stored in main environment or execution of static setter.

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

[zhivkag, 2017-08-09 07:07:58]

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

[Dmitry Stefantsov, 2017-08-16 08:25:39]

Please, find some questions below.

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

https://codereview.chromium.org/2997563002/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:72: void update(Member member, Value
value) {
I would add a comment that [MainEnvironment] instances represent environments
and are immutable, and [update] is used only to initially construct such an
environment and is not used during program execution.

https://codereview.chromium.org/2997563002/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:219: if
(Interpreter.mainEnvironment.contains(member)) {
I would use a special value to indicate that the static field didn't received a
value yet, as written in the spec.  In this case [contains] should be rewritten
accordingly.

https://codereview.chromium.org/2997563002/diff/20001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:229:
Interpreter.mainEnvironment.update(member, Value.nullInstance);
If the special value for uninitialized variables had been used, we would be able
to avoid mutating environments during program execution.

[zhivkag, 2017-08-16 08:57:17]

Thanks for the comments, Dima!

I would like to propose an alternative solution, since I don't like the approach
to populate this environment on startup.

Instead, I think it is preferable to consider that we manipulate the main
environment in the same way as the local environments. It is immutable and when
extended, a new main environment is created. This environment will be threaded
in all transitions, but for simplicity we can assume a global accessor. I will
send you a PR with the exact proposal for the documentation shortly, and update
here accordingly to whichever solution we agree.

[Dmitry Stefantsov, 2017-08-16 09:03:37]

On 2017/08/16 08:57:17, zhivkag wrote:
> Thanks for the comments, Dima!
> 
> I would like to propose an alternative solution, since I don't like the
approach
> to populate this environment on startup.
> 
> Instead, I think it is preferable to consider that we manipulate the main
> environment in the same way as the local environments. It is immutable and
when
> extended, a new main environment is created. This environment will be threaded
> in all transitions, but for simplicity we can assume a global accessor. I will
> send you a PR with the exact proposal for the documentation shortly, and
update
> here accordingly to whichever solution we agree.

I agree, this is another possibility to avoid mutable environments.  Either one
is ok for me.

[zhivkag, 2017-08-16 10:39:01]

PTAL.

[Dmitry Stefantsov, 2017-08-16 12:06:47]

I have a few more questions :)

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

https://codereview.chromium.org/2997563002/diff/40001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:66: class MainEnvironment {
Now that [MainEnvironment]s are immutable and we thread the current
[MainEnvironment] everywhere, why not use existing [Environment] class for it?

https://codereview.chromium.org/2997563002/diff/40001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:137: void update(VariableDeclaration
variable, Value value) {
How about renaming [update] to [updateStore] to better indicate that the
environment is not mutable?

[zhivkag, 2017-08-16 12:27:44]

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

https://codereview.chromium.org/2997563002/diff/40001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:66: class MainEnvironment {
On 2017/08/16 12:06:46, Dmitry Stefantsov wrote:
> Now that [MainEnvironment]s are immutable and we thread the current
> [MainEnvironment] everywhere, why not use existing [Environment] class for it?

One difference is that [MainEnvironment] maps [Member]s to [Location]s, while
[Environment] maps [VariableDeclaration]s to [Location]s. 

Currently Environment has a list of [Binding]s that are pairs of
VariableDeclaration to Location. Instead we could use Binding<T> that maps T to
Location and use only [Environment]. We would also need to change lookup,
updateStore and extend accordingly. Do you think that would be better?

https://codereview.chromium.org/2997563002/diff/40001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:137: void update(VariableDeclaration
variable, Value value) {
On 2017/08/16 12:06:47, Dmitry Stefantsov wrote:
> How about renaming [update] to [updateStore] to better indicate that the
> environment is not mutable?

Done. I also renamed assign -> update -> updateStore for [Environment].
Since we don't have an actual store in the implementation, it might be less
confusing to have updateLocation. WDYT?

[Dmitry Stefantsov, 2017-08-16 12:38:51]

Thank you for the answers! LGTM.

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

https://codereview.chromium.org/2997563002/diff/40001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:66: class MainEnvironment {
On 2017/08/16 12:27:44, zhivkag wrote:
> On 2017/08/16 12:06:46, Dmitry Stefantsov wrote:
> > Now that [MainEnvironment]s are immutable and we thread the current
> > [MainEnvironment] everywhere, why not use existing [Environment] class for
it?
> 
> One difference is that [MainEnvironment] maps [Member]s to [Location]s, while
> [Environment] maps [VariableDeclaration]s to [Location]s. 
> 
> Currently Environment has a list of [Binding]s that are pairs of
> VariableDeclaration to Location. Instead we could use Binding<T> that maps T
to
> Location and use only [Environment]. We would also need to change lookup,
> updateStore and extend accordingly. Do you think that would be better? 

Got it.  Thanks for the explanation!  No, I think having [MainEnvironment] is
better than a generic [Environment].

https://codereview.chromium.org/2997563002/diff/40001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:137: void update(VariableDeclaration
variable, Value value) {
On 2017/08/16 12:27:44, zhivkag wrote:
> On 2017/08/16 12:06:47, Dmitry Stefantsov wrote:
> > How about renaming [update] to [updateStore] to better indicate that the
> > environment is not mutable?
> 
> Done. I also renamed assign -> update -> updateStore for [Environment].
> Since we don't have an actual store in the implementation, it might be less
> confusing to have updateLocation. WDYT?

I think [updateStore] is ok.  As for "updateLocation," we don't update
locations, as I get it, we update values at those locations.

[Kevin Millikin (Google), 2017-08-18 11:06:56]

LGTM.

I do prefer the alternative semantics where the toplevel environment is
initialized before program execution begins.  This makes the toplevel static, in
the sense that a compiler could compile static get/set to a call to a known
getter/setter or a read/write to a fixed location.

In this CL the toplevel is dynamic in the sense that the program needs to be run
to determine which members are in the toplevel and in which order.

====

We will have to handle the case that there is a reference to a static toplevel
(or class) field during its initialization, e.g.:

int circle = circle;

and we should throw at runtime in that case, I think.

(But we can save that for another CL.)

[zhivkag, 2017-08-21 06:08:36]

Thanks for the review!
I will followup with another CL to fix the circular initialization and the
potential change of the top level environment.

[zhivkag, 2017-08-21 06:24:29]

Description was changed from

==========
Implement support for static accessors

- Evaluation of StaticPropertyGet expression by reading a value stored in 
the main environment, execution of static getter or creating a method tear off.
- Evaluation of StaticPropertySet expression by modifying the value 
stored in main environment or execution of static setter.

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

to

==========
Implement support for static accessors

- Evaluation of StaticPropertyGet expression by reading a value stored in
the main environment, execution of static getter or creating a method tear off.
- Evaluation of StaticPropertySet expression by modifying the value
stored in main environment or execution of static setter.

BUG=
R=dmitryas@google.com, kmillikin@google.com

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

[zhivkag, 2017-08-21 06:24:31]

Committed patchset #7 (id:120001) manually as
e89bbf96a5215cc3e622459b787f9d5c5d0d3dd4 (presubmit successful).