Fix #14144 confusing error message misusing a callable object

Fix #14144 confusing error message misusing a callable object

New tests to make sure that:
* Error message calling a closure wrong is unchanged
* Error message calling a static function wrong is unchanged
* Error message calling a callable object wrong is unchanged
* Error message calling nonexist method is unchanged
* Error message calling nonexist method for a callable object is clearer

The new tests involving calling a closure wrong exposed a bug in the
inliner, it assumed (due to lack of ic data, it seems) that all closures
have the right number of arguments. Left that assertion/behavior, but
put a guard around closures specifically (since static and method calls
don't have that bug, due to I think better ic data).

Welcoming myself to the world of VMs.

BUG=
R=fschneider@google.com

Committed: https://github.com/dart-lang/sdk/commit/761d21d41466eae28074ff7a42a75f60ad4fde6c

[mfairhurst, 2017-03-30 01:08:21]

mfairhurst@google.com changed reviewers:
+ asiva@google.com, fschneider@google.com, rmacnak@google.com

[mfairhurst, 2017-03-30 01:08:21]

Started at the oldest and worked back. This one was pretty easy, with an
unexpected hitch with crashing the inliner. But fixed that too. Hoping that fix
is in the best place, and my tests hit up all relevant edge cases.

[Florian Schneider, 2017-03-30 20:18:06]

https://codereview.chromium.org/2785623004/diff/1/runtime/lib/errors_patch.dart
File runtime/lib/errors_patch.dart (right):

https://codereview.chromium.org/2785623004/diff/1/runtime/lib/errors_patch.da...
runtime/lib/errors_patch.dart:317: (_receiver is Function &&
memberName.startsWith("call"))) {
I think this should be

(_receiver is Function && memberName == "call")

so that something like "callbar" is handled as expected.

https://codereview.chromium.org/2785623004/diff/1/runtime/vm/flow_graph_inlin...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/2785623004/diff/1/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:1265: if (target.NumParameters() !=
call->ArgumentCount()) {
Good catch!

Your fix is safe, but it could be improved by taking the fixed parameter count
into account:

if (call->ArgumentCount() > target.NumParameters() ||
    call->ArgumentCount() < target.num_fixed_parameters()) {
  // Bailout
  continue;
}

This way the inliner can still inline the closure if it has optional parameters.
A function f(a, b, [c]) would be only valid to inline when called with 2 or 3
arguments.

Optional parameters are dealt with later in AdjustForOptionalParameters which
fills in the default values for missing optional arguments.

[mfairhurst, 2017-03-31 02:17:11]

Good catch on optional arguments. Had seen that code, but was overly focused on
the failed assertion itself, so replicated that test instead of the better one
you provided. Fixed!

regarding using == call vs startsWith call, looked over it again, and I think
the whole check is more confusing. Clearer to say "YourClass has no method
call() with matching arguments" than "closure with wrong number of objects;
tried calling call(foo) on YourClass". I think this is the cleanest way.

[rmacnak, 2017-03-31 16:36:39]

https://codereview.chromium.org/2785623004/diff/40001/runtime/lib/errors_patc...
File runtime/lib/errors_patch.dart (right):

https://codereview.chromium.org/2785623004/diff/40001/runtime/lib/errors_patc...
runtime/lib/errors_patch.dart:315: if (_receiver.runtimeType == Function ||
`_receiver.runtimeType == Function` doesn't match anything because Function is
abstract.


class C {
  call() => 42;
}

main() {
  var c = new C();
  print(c.runtimeType == Function);  // false
  print(c is Function);  // true
}

https://codereview.chromium.org/2785623004/diff/40001/tests/language/vm/no_su...
File tests/language/vm/no_such_args_error_message_vm_test.dart (right):

https://codereview.chromium.org/2785623004/diff/40001/tests/language/vm/no_su...
tests/language/vm/no_such_args_error_message_vm_test.dart:17:
Expect.isTrue(e.toString().indexOf(
string.indexOf(substring) != -1 
=>
string.contains(substring)

https://codereview.chromium.org/2785623004/diff/40001/tests/language/vm/no_su...
tests/language/vm/no_such_args_error_message_vm_test.dart:29:
Expect.isTrue(e.toString().indexOf("Tried calling: noargs(\"bar\")") != -1);
"

https://codereview.chromium.org/2785623004/diff/40001/tests/language/vm/no_su...
File tests/language/vm/no_such_method_error_message_callable_vm_test.dart
(right):

https://codereview.chromium.org/2785623004/diff/40001/tests/language/vm/no_su...
tests/language/vm/no_such_method_error_message_callable_vm_test.dart:22:
Expect.isTrue(e.toString().indexOf("has no instance method 'bar'") != -1);
"

https://codereview.chromium.org/2785623004/diff/40001/tests/language/vm/no_su...
tests/language/vm/no_such_method_error_message_callable_vm_test.dart:32:
Expect.isTrue(e.toString().indexOf(noMatchingArgs) != -1);
"

[mfairhurst, 2017-03-31 18:27:14]

+Ryan's feedback.

Dropped the entire '.runtimeType ==', can now simply do 'is Closure' instead of
the old 'is Function' to not hit objects which implement call().

Also replaced both my tests and the one I had modeled them from to use
.contains(). Had forgotten there was a really ugly formatted expectation in one
of them, way cleaner with .contains(), big win there.

[Florian Schneider, 2017-03-31 21:09:51]

Lgtm!

[mfairhurst, 2017-03-31 22:09:09]

Description was changed from

==========
Fix #14144 confusing error message misusing a callable object

New tests to make sure that:
* Error message calling a closure wrong is unchanged
* Error message calling a static function wrong is unchanged
* Error message calling a callable object wrong is unchanged
* Error message calling nonexist method is unchanged
* Error message calling nonexist method for a callable object is clearer

The new tests involving calling a closure wrong exposed a bug in the
inliner, it assumed (due to lack of ic data, it seems) that all closures
have the right number of arguments. Left that assertion/behavior, but
put a guard around closures specifically (since static and method calls
don't have that bug, due to I think better ic data).

Welcoming myself to the world of VMs.

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

to

==========
Fix #14144 confusing error message misusing a callable object

New tests to make sure that:
* Error message calling a closure wrong is unchanged
* Error message calling a static function wrong is unchanged
* Error message calling a callable object wrong is unchanged
* Error message calling nonexist method is unchanged
* Error message calling nonexist method for a callable object is clearer

The new tests involving calling a closure wrong exposed a bug in the
inliner, it assumed (due to lack of ic data, it seems) that all closures
have the right number of arguments. Left that assertion/behavior, but
put a guard around closures specifically (since static and method calls
don't have that bug, due to I think better ic data).

Welcoming myself to the world of VMs.

BUG=
R=fschneider@google.com

Committed:
https://github.com/dart-lang/sdk/commit/761d21d41466eae28074ff7a42a75f60ad4fde6c
==========

[mfairhurst, 2017-03-31 22:09:11]

Committed patchset #5 (id:80001) manually as
761d21d41466eae28074ff7a42a75f60ad4fde6c (presubmit successful).