VM: Fix bug with evaluation order of static setter invocation.

VM: Fix bug with evaluation order of static setter invocation.

This fixes the case for an assignment of the form
v=e
where v is unknown. The right-hand side of the assignment
has to be evaluated before throwing a NoSuchMethodError.

I moved the generation of the NoSuchMethodError out of
the parser to the flow-graph builder, where the right-hand side
argument is evaluated before throwing.

BUG=https://code.google.com/p/dart/issues/detail?id=13494
TEST=co19/Language/12_Expressions/18_Assignment_A01_t07
R=hausner@google.com

Committed: https://code.google.com/p/dart/source/detail?r=28723

[Florian Schneider, 2013-10-03 15:33:21]

https://codereview.chromium.org/25842003/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/25842003/diff/1/runtime/vm/parser.cc#newcode8139
runtime/vm/parser.cc:8139: field_name);
field_name is already a ZoneHandle.

[Ivan Posva, 2013-10-03 16:44:04]

https://codereview.chromium.org/25842003/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (left):

https://codereview.chromium.org/25842003/diff/1/runtime/vm/parser.cc#oldcode8213
runtime/vm/parser.cc:8213: InvocationMirror::kStatic,
You are losing information here about the kind of NoSuchMethod. Why don't we
just pass the arguments here?

[hausner, 2013-10-03 17:36:28]

A solution that keeps the generation of NSME in the parser is preferable. The
NSME generation in the backend is something we wanted to get rid of over the
long term.

[Florian Schneider, 2013-10-04 09:10:28]

I agree that the current generation of NoSuchMethodErrors is a mess: There are
12 places in the parser and 2 in the flow-graph builder where we generate a
throw NoSuchMethodError.

To simplify this, I think we would have to change the way we parse assignments.

On the other hand my change does not really add any more complexity, other than
deferring one case to the flow-graph builder which already handles
StaticGetter/StaticSetters which seems consistent with the current approach.
With that there are 11 ThrowNoSuchMethodError left in the parser left.

https://codereview.chromium.org/25842003/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (left):

https://codereview.chromium.org/25842003/diff/1/runtime/vm/parser.cc#oldcode8213
runtime/vm/parser.cc:8213: InvocationMirror::kStatic,
On 2013/10/03 16:44:05, Ivan Posva wrote:
> You are losing information here about the kind of NoSuchMethod. Why don't we
> just pass the arguments here?

We would need the RHS of the assignment as arguments here. I think this would be
a bigger change to the parser.

Actually, the contrary is the case: Here we don't know if it will be a getter or
setter. Later, the StaticGetterNode is transformed into StaticSetterNode if it
occurs in an assignment. As a result, the error message is more precise with my
change: It include the arguments and distinguishes between getter and setter.

Before:
No top-level getter or setter 'unknown' declared.

NoSuchMethodError : method not found: 'unknown'
Receiver: top-level
Arguments: [...]


After:
No top-level setter 'set:unknown' declared.

NoSuchMethodError : method not found: 'set:unknown'
Receiver: top-level
Arguments: [null]

[Florian Schneider, 2013-10-10 08:55:13]

On 2013/10/04 09:10:28, Florian Schneider wrote:
> I agree that the current generation of NoSuchMethodErrors is a mess: There are
> 12 places in the parser and 2 in the flow-graph builder where we generate a
> throw NoSuchMethodError.
> 
> To simplify this, I think we would have to change the way we parse
assignments.
> 
> On the other hand my change does not really add any more complexity, other
than
> deferring one case to the flow-graph builder which already handles
> StaticGetter/StaticSetters which seems consistent with the current approach.
> With that there are 11 ThrowNoSuchMethodError left in the parser left.
> 
> https://codereview.chromium.org/25842003/diff/1/runtime/vm/parser.cc
> File runtime/vm/parser.cc (left):
> 
>
https://codereview.chromium.org/25842003/diff/1/runtime/vm/parser.cc#oldcode8213
> runtime/vm/parser.cc:8213: InvocationMirror::kStatic,
> On 2013/10/03 16:44:05, Ivan Posva wrote:
> > You are losing information here about the kind of NoSuchMethod. Why don't we
> > just pass the arguments here?
> 
> We would need the RHS of the assignment as arguments here. I think this would
be
> a bigger change to the parser.
> 
> Actually, the contrary is the case: Here we don't know if it will be a getter
or
> setter. Later, the StaticGetterNode is transformed into StaticSetterNode if it
> occurs in an assignment. As a result, the error message is more precise with
my
> change: It include the arguments and distinguishes between getter and setter.
> 
> Before:
> No top-level getter or setter 'unknown' declared.
> 
> NoSuchMethodError : method not found: 'unknown'
> Receiver: top-level
> Arguments: [...]
> 
> 
> After:
> No top-level setter 'set:unknown' declared.
> 
> NoSuchMethodError : method not found: 'set:unknown'
> Receiver: top-level
> Arguments: [null]

Ping?

[hausner, 2013-10-15 21:00:04]

LGTM. Sorry for the long delay!

Matthias

[Florian Schneider, 2013-10-16 13:08:06]

Committed patchset #3 manually as r28723 (presubmit successful).