Refactor argument evaluation for static invocation

Refactor argument evaluation for static invocation

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

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

[zhivkag, 2017-05-10 07:10:49]

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

[zhivkag, 2017-05-10 07:10:50]

Hi, 

This change refactors the evaluation of arguments for static invocation as we
discussed offline. I made this a separate change, but it will be reused for
constructor invocation in a another CL. 

It introduces a new evaluation function for a list of arguments and an
ArgumentsConfiguration which is a executer step for invoking the function.

It also introduces a new kind of continuations, ApplicationContinuation which
are applied to a list of argument values.

Thanks, 
Zhivka

[Dmitry Stefantsov, 2017-05-10 08:08:49]

Nice! LGTM.

I have only one comment which is more a question than a suggestion.

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

https://codereview.chromium.org/2871953002/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:390: String get name => throw
'PositionalArgumentExpression has no name';
It's not clear why we define "name" getter in ArgumentExpression and throw an
exception here, but not follow the same pattern with ArgumentValue and
PositionalArgumentValue below (the "name" field is not defined in ArgumentValue
class).

[zhivkag, 2017-05-10 08:27:07]

Thanks!

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

https://codereview.chromium.org/2871953002/diff/1/pkg/kernel/lib/interpreter/...
pkg/kernel/lib/interpreter/interpreter.dart:390: String get name => throw
'PositionalArgumentExpression has no name';
On 2017/05/10 08:08:49, Dmitry Stefantsov wrote:
> It's not clear why we define "name" getter in ArgumentExpression and throw an
> exception here, but not follow the same pattern with ArgumentValue and
> PositionalArgumentValue below (the "name" field is not defined in
ArgumentValue
> class).

I initially intended to have name for both, but finally decided to only add the
name attribute for the named expressions and values. I must have forgotten to
remove it here. Done.

I removed the name attribute because later we might have more general
interpreter expressions (for example for field and local initializers), which
will have other attributes and the only common one is an expression getter.

[Dmitry Stefantsov, 2017-05-10 08:41:45]

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

https://codereview.chromium.org/2871953002/diff/40001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:574: (currentArgument as
NamedArgumentExpression).name, v);
Btw, we don't need the "as" cast here, because of the type promotion. If we
already checked that the type of "currentArgument" is "NamedArgumentExpression"
using the "is" check, we may avoid an explicit cast. The IDE doesn't detect it
always, but it actually works :)

[Kevin Millikin (Google), 2017-05-10 08:59:14]

LGTM!

[zhivkag, 2017-05-10 09:11:07]

Thanks!

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

https://codereview.chromium.org/2871953002/diff/40001/pkg/kernel/lib/interpre...
pkg/kernel/lib/interpreter/interpreter.dart:574: (currentArgument as
NamedArgumentExpression).name, v);
On 2017/05/10 08:41:45, Dmitry Stefantsov wrote:
> Btw, we don't need the "as" cast here, because of the type promotion. If we
> already checked that the type of "currentArgument" is
"NamedArgumentExpression"
> using the "is" check, we may avoid an explicit cast. The IDE doesn't detect it
> always, but it actually works :)

Type promotion doesn't seem to be working in this case, I am not sure why.
Dartanalyzer outputs an error if we omit the cast :( Keeping it for now.

[zhivkag, 2017-05-10 09:11:45]

Description was changed from

==========
Refactor argument evaluation for static invocation

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

to

==========
Refactor argument evaluation for static invocation

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

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

[zhivkag, 2017-05-10 09:11:46]

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