Fix crash bug with noSuchMethod invocation and argument count mismatch.

Fix crash bug with noSuchMethod invocation and argument count mismatch.

When trying to invoke noSuchMethod on an object and it does not implement
 noSuchMethod(i) -- with one argument, but with a mismatching number of arguments,
throw a NoSuchMethodError.

BUG=dartbug.com/12561
TEST=tests/language/regress_12561_test.dart
R=iposva@google.com

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

[Ivan Posva, 2014-07-14 10:03:19]

LGTMwC -ip

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

https://codereview.chromium.org/390003002/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:1389: no_such_method ^=
Resolver::ResolveDynamicForReceiverClass(
Indentation.

[Florian Schneider, 2014-07-14 10:06:20]

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

https://codereview.chromium.org/390003002/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:1389: no_such_method ^=
Resolver::ResolveDynamicForReceiverClass(
On 2014/07/14 10:03:19, Ivan Posva wrote:
> Indentation.

Done.

[Florian Schneider, 2014-07-14 10:06:48]

Committed patchset #1 manually as r38183 (presubmit successful).

[regis, 2014-07-14 17:40:18]

If I modify the regression test as below, I get a NoSuchMethodError. Is that
expected? I would think the noSuchMethod of the base class should be called.

class B {
  noSuchMethod(Invocation m) => 42;
}

class C extends B {
  noSuchMethod(int x, int y) => x + y;
}

main() {
  Expect.equals(42, new B().foo);  // OK
  Expect.equals(42, new C().foo);  // NoSuchMethodError
}

[Ivan Posva, 2014-07-14 20:04:38]

On 2014/07/14 17:40:18, regis wrote:
> If I modify the regression test as below, I get a NoSuchMethodError. Is that
> expected? I would think the noSuchMethod of the base class should be called.
> 
> class B {
>   noSuchMethod(Invocation m) => 42;
> }
> 
> class C extends B {
>   noSuchMethod(int x, int y) => x + y;
> }
> 
> main() {
>   Expect.equals(42, new B().foo);  // OK
>   Expect.equals(42, new C().foo);  // NoSuchMethodError
> }

We dispatch to the noSuchMethod of the receiver. Since that one is incorrect
(wrong number of parameters) we throw a noSuchMethodError, which is the correct
behaviour. noSuchMethod lookup is just like any regular method lookup. If you
had declared method foo in B and C and the number of parameters did not match
when calling C, you'd not attempt to resolve in B either.

-Ivan

[regis, 2014-07-14 20:30:14]

On 2014/07/14 20:04:38, Ivan Posva wrote:
> On 2014/07/14 17:40:18, regis wrote:
> > If I modify the regression test as below, I get a NoSuchMethodError. Is that
> > expected? I would think the noSuchMethod of the base class should be called.
> > 
> > class B {
> >   noSuchMethod(Invocation m) => 42;
> > }
> > 
> > class C extends B {
> >   noSuchMethod(int x, int y) => x + y;
> > }
> > 
> > main() {
> >   Expect.equals(42, new B().foo);  // OK
> >   Expect.equals(42, new C().foo);  // NoSuchMethodError
> > }
> 
> We dispatch to the noSuchMethod of the receiver. Since that one is incorrect
> (wrong number of parameters) we throw a noSuchMethodError, which is the
correct
> behaviour. noSuchMethod lookup is just like any regular method lookup. If you
> had declared method foo in B and C and the number of parameters did not match
> when calling C, you'd not attempt to resolve in B either.
> 
> -Ivan

I stand corrected. My impression was that noSuchMethod should be called in case
the number of actual and formal parameters do not match. But this is wrong,
noSuchMethod is thrown in this case, which is consistent with the fix.

[regis, 2014-07-14 20:32:08]

On 2014/07/14 20:30:14, regis wrote:
> On 2014/07/14 20:04:38, Ivan Posva wrote:
> > On 2014/07/14 17:40:18, regis wrote:
> > > If I modify the regression test as below, I get a NoSuchMethodError. Is
that
> > > expected? I would think the noSuchMethod of the base class should be
called.
> > > 
> > > class B {
> > >   noSuchMethod(Invocation m) => 42;
> > > }
> > > 
> > > class C extends B {
> > >   noSuchMethod(int x, int y) => x + y;
> > > }
> > > 
> > > main() {
> > >   Expect.equals(42, new B().foo);  // OK
> > >   Expect.equals(42, new C().foo);  // NoSuchMethodError
> > > }
> > 
> > We dispatch to the noSuchMethod of the receiver. Since that one is incorrect
> > (wrong number of parameters) we throw a noSuchMethodError, which is the
> correct
> > behaviour. noSuchMethod lookup is just like any regular method lookup. If
you
> > had declared method foo in B and C and the number of parameters did not
match
> > when calling C, you'd not attempt to resolve in B either.
> > 
> > -Ivan
> 
> I stand corrected. My impression was that noSuchMethod should be called in
case
> the number of actual and formal parameters do not match. But this is wrong,
> noSuchMethod is thrown in this case, which is consistent with the fix.

Argh! I meant: "noSuchMethodError is thrown in this case"

[regis, 2014-07-14 22:00:24]

On 2014/07/14 20:32:08, regis wrote:
> On 2014/07/14 20:30:14, regis wrote:
> > On 2014/07/14 20:04:38, Ivan Posva wrote:
> > > On 2014/07/14 17:40:18, regis wrote:
> > > > If I modify the regression test as below, I get a NoSuchMethodError. Is
> that
> > > > expected? I would think the noSuchMethod of the base class should be
> called.
> > > > 
> > > > class B {
> > > >   noSuchMethod(Invocation m) => 42;
> > > > }
> > > > 
> > > > class C extends B {
> > > >   noSuchMethod(int x, int y) => x + y;
> > > > }
> > > > 
> > > > main() {
> > > >   Expect.equals(42, new B().foo);  // OK
> > > >   Expect.equals(42, new C().foo);  // NoSuchMethodError
> > > > }
> > > 
> > > We dispatch to the noSuchMethod of the receiver. Since that one is
incorrect
> > > (wrong number of parameters) we throw a noSuchMethodError, which is the
> > correct
> > > behaviour. noSuchMethod lookup is just like any regular method lookup. If
> you
> > > had declared method foo in B and C and the number of parameters did not
> match
> > > when calling C, you'd not attempt to resolve in B either.
> > > 
> > > -Ivan
> > 
> > I stand corrected. My impression was that noSuchMethod should be called in
> case
> > the number of actual and formal parameters do not match. But this is wrong,
> > noSuchMethod is thrown in this case, which is consistent with the fix.
> 
> Argh! I meant: "noSuchMethodError is thrown in this case"

Let me clarify. The fact that foo is invoked without parenthesis is important.
It means that we invoke a function and not a method.

A function invocation with the wrong number of arguments results in a
noSuchMethodError being thrown, as Ivan says.
However, a method invocation with the wrong number of arguments results in a
call to noSuchMethod and it was the point I tried to make in my original
comment, but I did not write it correctly (missing parenthesis).

This test succeeds:

class B {
   noSuchMethod(Invocation m) => 42;
}

class C extends B {
   foo(int x, int y) => x + y;
}

main() {
   Expect.equals(42, new C().foo());
}

Therefore, I was expecting this test to succeed as well (note the parenthesis
after foo):

class B {
   noSuchMethod(Invocation m) => 42;
}

class C extends B {
   noSuchMethod(int x, int y) => x + y;
}

main() {
   Expect.equals(42, new C().foo());
}


But it does not, because of Gilad's spec change. Florian's fix matches the
revised spec. However, the implementation is still not quite correct, because
memberName of Invocation should be "noSuchMethod" and not "foo".

Also, this test still crashes with a stack overflow:

class C {
  noSuchMethod(im) => x + y;
}

main() {
  new C().foo;
}