VM Crash: shadowing noSuchMethod with incompatible signature and reading non-existing getter.

[lrhn]

Example:

  class C {     noSuchMethod(int x, int y) => x + y;   }   main() {     new C().foo;   }

Since we changed a bad shadowing (not a real override) to be a warning instead of an error, it became possible for an object to not implement noSuchMethod(InvocationMirror i). Previously that was impossible.

The VM seems to not be able to handle this gracefully yet.

[iposva-google]

The question here is really what should we do in this case? Skip the bad noSuchMethod?

---

Set owner to @iposva-google. Added Started label.

[lrhn]

The spec doesn't contemplate the possibility. A stack overflow is a valid implementation, it should probably not segfault, though.

We need a spec update to do something else (which was why I cc'ed Gilad). I would suggest throwing a NoSuchMethodError if we can't call noSuchMethod with one argument.

[iposva-google]

I agree that crashing is not the right behaviour. As we are reading the spec right now you get into an endless recursion of calling the noSuchMethod with two arguments. That is what I will be implementing. If the spec changes we should update the VM.

[gbracha]

Looked a bit more into this. Strictly speaking, when we find a method of a given name but with the wrong arguments, we say it is a runtime error but do not actually specify that this is a noSuchMethod situation. That was a spec bug which I have just corrected.

[iposva-google]

Removed Priority-Unassigned label. Added Priority-High label.

[kasperl]

Added this to the 1.6 milestone.

[iposva-google]

In the current TOT we dispatch to the obviously wrong noSuchMethod function:

./xcodebuild/DebugIA32/dart ~/dart/Bug12561.dart Stop message: Wrong number of arguments Trace/BPT trap

There are multiple places where we are looking up the noSuchMethod function without checking for the number of arguments ahead of time such as EffectGraphVisitor::BuildStaticNoSuchMethodCall.

---

cc @crelier. Set owner to @fsc8000. Added Accepted label.

[iposva-google]

Or more likely: Parser::ParseNoSuchMethodDispatcher

[fsc8000]

Parser::ParseNoSuchMethodDispatcher must take the number of arguments into account when looking up noSuchMethod.

dart2js throws a RangeError in this case. Wouldn't throwing a NoSuchMethodError (i.e. invoking Object::noSuchMethod) make more sense?

[fsc8000]

Result with my fix:

Unhandled exception: Class 'C' has no instance getter 'foo'.

NoSuchMethodError: method not found: 'foo' Receiver: Instance of 'C' Arguments: []

­0 Object.noSuchMethod (dart:core-patch/object_patch.dart:45)

­1 main (file:///media/SSD/dart2/dart/nsm.dart:5:11)

­2 _startIsolate.isolateStartHandler (dart:isolate-patch/isolate_patch.dart:214)

­3 _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:122)

Result with dart2js: out.js:0: RangeError: Maximum call stack size exceeded // Generated by dart2js, the Dart to JavaScript compiler version: 1.6.0-edge.3 ^ RangeError: Maximum call stack size exceeded     at C.Object.noSuchMethod$1 (out.js)     at C.Object.noSuchMethod$1 (out.js:1425:19)     at C.Object.noSuchMethod$1 (out.js:1425:19)     at C.Object.noSuchMethod$1 (out.js:1425:19)     at C.Object.noSuchMethod$1 (out.js:1425:19)     at C.Object.noSuchMethod$1 (out.js:1425:19)     at C.Object.noSuchMethod$1 (out.js:1425:19)     at C.Object.noSuchMethod$1 (out.js:1425:19)     at C.Object.noSuchMethod$1 (out.js:1425:19)     at C.Object.noSuchMethod$1 (out.js:1425:19)

[lrhn]

I'm not sure "making sense" is worth considering when you override noSuchMethod incorrectly. A sternly worded warning/hint might be more appropriate. We (dart2js, analyzer) already give a warning when you do a bad override of noSuchMethod. We might want to be even more explicit about that particular case - "IF YOU OVERRIDE noSuchMethod INCORRECLY, YOU'RE GOING TO HAVE A BAD DAY, M'KAY?'.

On one hand dart2js is following the specification. Executing C.foo becomes c.noSuchMethod(invoc) which becomes c.noSuchMethod(invoc2) .....

On the other hand, no user will thank us for following the specification to the letter here, when we can do better. There is no spec for what to do on stack overflow, so calling Object.noSuchMethod isn't necessarily wrong.

[fsc8000]

Fixed in r38183. VM produces a NoSuchMethodError now in this case.

---

Added Fixed label.

[gbracha]

So Lasse is correct in saying that calling the implementation of noSuchMethod in Object when noSuchMethod(inv) is not found is a valid implementation. I think it would be better to explicitly address this in the spec. I will do so and notify TC52 - I don't think they are likely to mind.

[gbracha]

So Lasse is correct in saying that calling the implementation of noSuchMethod in Object when noSuchMethod(inv) is not found is a valid implementation. I think it would be better to explicitly address this in the spec. I will do so and notify TC52 - I don't think they are likely to mind.

[crelier]

Reopening, because this test still crashes with a stack overflow:

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

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

---

Added Accepted label.

[lrhn]

That stack overflow is much harder to fix. It's not unthinkable that some class will have a noSuchMethod with some logic that makes it call method on the same object again a few times before, e.g., finding the correct number of arguments. That means that we can't react on any recursive triggering of noSuchMethod, but must detect that we have an infinite cycle because the Invocation objects are equivalent.

Again, a stack overflow is a valid response. As long as it doesn't crash/seg-fault, I can live with the result.

[crelier]

Fair enough. Several different invocation objects could be involved in a cycle. And the spec would need to be changed again. Never mind. Re-closing the bug :-)

---

Added Fixed label.