Add support for static invocation with arguments

Add support for static invocation with arguments

BUG=
R=dmitryas@google.com

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

[zhivkag, 2017-04-21 12:24:14]

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

[zhivkag, 2017-04-21 12:24:15]

Hi, 

This CL adds support for static invocation with arguments.

When a static invocation expression is evaluated, first the actual/provided
positional and named arguments are evaluated, then the missing arguments (which
are optional positional and/or named arguments) are initialized by evaluating
their respective initializer expression (with assumption the later is not null).
At last, the body of the function is executed. 

Any comments or suggestions are welcome! 

Thanks,
Zhivka

[Dmitry Stefantsov, 2017-04-21 13:49:36]

The code looks almost ok to me, I only have a couple of comments. One of them is
about possible issues with order of elements in Map.keys.

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

https://codereview.chromium.org/2832873004/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:490: assert(_currentPositional >=
functionNode.requiredParameterCount);
I think we can make this assertion once, in the constructor.

https://codereview.chromium.org/2832873004/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:495: String name =
_missingFormalNamed.keys.first;
I think we need to find another way of determining the name of the named
argument here. The order in ".keys" may be implementation-specific and may
change when the map is modified or copied. Using ".keys.first" here adds some
implicit constraints to the modification of the code in future:

(1) The "_missingFormalNamed" should not be modified between the previous call
to "createCurrentConfiguration" (where ".keys.first" is also used, and we need
to sync with that) and the subsequent call to "call" on the same object.

(2) The "_missingFormalNamed" should not be copied and replaced with its copy.
The same goes for the enclosing OptionalArgumentsContinuation. If in future we
decide to not use indices to track the progress on arguments evaluation, but
rather create a new OptionalArgumentsContinuation that is more defined than the
previous one, that'll be an issue.

I propose getting here (and in "createCurrentConfiguration" below) the fist name
in lexicographical order. Maybe, there's a better approach for passing the name
of the argument from "createCurrentConfiguration" here.

[Dmitry Stefantsov, 2017-04-21 14:06:11]

On 2017/04/21 13:49:36, Dmitry Stefantsov wrote:
> The code looks almost ok to me, I only have a couple of comments. One of them
is
> about possible issues with order of elements in Map.keys.
> 
>
https://codereview.chromium.org/2832873004/diff/1/pkg/kernel/lib/interpreter/...
> File pkg/kernel/lib/interpreter/interpreter.dart (right):
> 
>
https://codereview.chromium.org/2832873004/diff/1/pkg/kernel/lib/interpreter/...
> pkg/kernel/lib/interpreter/interpreter.dart:490: assert(_currentPositional >=
> functionNode.requiredParameterCount);
> I think we can make this assertion once, in the constructor.
> 
>
https://codereview.chromium.org/2832873004/diff/1/pkg/kernel/lib/interpreter/...
> pkg/kernel/lib/interpreter/interpreter.dart:495: String name =
> _missingFormalNamed.keys.first;
> I think we need to find another way of determining the name of the named
> argument here. The order in ".keys" may be implementation-specific and may
> change when the map is modified or copied. Using ".keys.first" here adds some
> implicit constraints to the modification of the code in future:
> 
> (1) The "_missingFormalNamed" should not be modified between the previous call
> to "createCurrentConfiguration" (where ".keys.first" is also used, and we need
> to sync with that) and the subsequent call to "call" on the same object.
> 
> (2) The "_missingFormalNamed" should not be copied and replaced with its copy.
> The same goes for the enclosing OptionalArgumentsContinuation. If in future we
> decide to not use indices to track the progress on arguments evaluation, but
> rather create a new OptionalArgumentsContinuation that is more defined than
the
> previous one, that'll be an issue.
> 
> I propose getting here (and in "createCurrentConfiguration" below) the fist
name
> in lexicographical order. Maybe, there's a better approach for passing the
name
> of the argument from "createCurrentConfiguration" here.

Regarding my previous comment: we may use a custom implementation of Map that is
essentially a list of pairs (name, expression), and leave invocations of
".keys.first". Whenever option you'll go with, the code will LGTM :)

[zhivkag, 2017-04-21 16:44:01]

Thanks for the review and the comments!

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

https://codereview.chromium.org/2832873004/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:490: assert(_currentPositional >=
functionNode.requiredParameterCount);
On 2017/04/21 13:49:36, Dmitry Stefantsov wrote:
> I think we can make this assertion once, in the constructor.

Done.

https://codereview.chromium.org/2832873004/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:495: String name =
_missingFormalNamed.keys.first;
On 2017/04/21 13:49:36, Dmitry Stefantsov wrote:
> I think we need to find another way of determining the name of the named
> argument here. The order in ".keys" may be implementation-specific and may
> change when the map is modified or copied. Using ".keys.first" here adds some
> implicit constraints to the modification of the code in future:
> 
> (1) The "_missingFormalNamed" should not be modified between the previous call
> to "createCurrentConfiguration" (where ".keys.first" is also used, and we need
> to sync with that) and the subsequent call to "call" on the same object.
> 
> (2) The "_missingFormalNamed" should not be copied and replaced with its copy.
> The same goes for the enclosing OptionalArgumentsContinuation. If in future we
> decide to not use indices to track the progress on arguments evaluation, but
> rather create a new OptionalArgumentsContinuation that is more defined than
the
> previous one, that'll be an issue.
> 
> I propose getting here (and in "createCurrentConfiguration" below) the fist
name
> in lexicographical order. Maybe, there's a better approach for passing the
name
> of the argument from "createCurrentConfiguration" here.

Done. Indeed, another approach for this would be better. I have opted for saving
the Name of the parameter for which we are evaluating the initializer, which
stays similar to the way we deal with positional arguments.

[zhivkag, 2017-04-24 08:14:34]

Description was changed from

==========
Add support for static invocation with arguments

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

to

==========
Add support for static invocation with arguments

BUG=
R=dmitryas@google.com

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

[zhivkag, 2017-04-24 08:14:35]

Committed patchset #2 (id:20001) manually as
2860c9cb1b83556b96ec82871be2ec727b4925d8 (presubmit successful).