Make unmatched static call a runtime error.

Make unmatched static call a runtime error.

The runtime error message will be constructed using NoSuchMethodError.toString(), which prints "incorrect number of arguments passed to method", which may be a bit confusing for calls where the named optional arguments do not match, but we can emit a more precise message in the resolver.

BUG=http://code.google.com/p/dart/issues/detail?id=6217

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

[ngeoffray, 2012-10-25 11:24:09]

http://codereview.chromium.org/11273034/diff/1/lib/compiler/implementation/re...
File lib/compiler/implementation/resolution/members.dart (right):

http://codereview.chromium.org/11273034/diff/1/lib/compiler/implementation/re...
lib/compiler/implementation/resolution/members.dart:1704: || target.kind ==
ElementKind.GENERATIVE_CONSTRUCTOR)
Why do you need these kind checks?

http://codereview.chromium.org/11273034/diff/1/lib/compiler/implementation/ss...
File lib/compiler/implementation/ssa/builder.dart (right):

http://codereview.chromium.org/11273034/diff/1/lib/compiler/implementation/ss...
lib/compiler/implementation/ssa/builder.dart:2741: //
[generateWrongArgumentCoundError].
generateWrongArgumentCoundError doesn't use function, so I think you don't have
to change that comment.

[ahe, 2012-10-25 13:34:04]

LGTM, except for the use of optional arguments.

http://codereview.chromium.org/11273034/diff/1/lib/compiler/implementation/li...
File lib/compiler/implementation/lib/js_helper.dart (right):

http://codereview.chromium.org/11273034/diff/1/lib/compiler/implementation/li...
lib/compiler/implementation/lib/js_helper.dart:1378: void throwNoSuchMethod(obj,
name, arguments, [expectedArgumentNames]) {
Could you avoid making the argument optional?

http://codereview.chromium.org/11273034/diff/1/lib/compiler/implementation/ss...
File lib/compiler/implementation/ssa/builder.dart (right):

http://codereview.chromium.org/11273034/diff/1/lib/compiler/implementation/ss...
lib/compiler/implementation/ssa/builder.dart:3114: {Link<Node> argumentNodes,
Why do you need optional arguments here?

[karlklose, 2012-10-25 13:55:44]

Thanks for the reviews. I have uploaded a new version which addresses some
offline comments Peter had and adds the status file update.

http://codereview.chromium.org/11273034/diff/1/lib/compiler/implementation/re...
File lib/compiler/implementation/resolution/members.dart (right):

http://codereview.chromium.org/11273034/diff/1/lib/compiler/implementation/re...
lib/compiler/implementation/resolution/members.dart:1704: || target.kind ==
ElementKind.GENERATIVE_CONSTRUCTOR)
I need to be able to compute the signature for the applies check.

http://codereview.chromium.org/11273034/diff/1/lib/compiler/implementation/ss...
File lib/compiler/implementation/ssa/builder.dart (right):

http://codereview.chromium.org/11273034/diff/1/lib/compiler/implementation/ss...
lib/compiler/implementation/ssa/builder.dart:2741: //
[generateWrongArgumentCoundError].
On 2012/10/25 11:24:09, ngeoffray wrote:
> generateWrongArgumentCoundError doesn't use function, so I think you don't
have
> to change that comment.

Done.

[ahe, 2012-10-25 14:07:27]

LGTM, except for the optional arguments.

http://codereview.chromium.org/11273034/diff/3002/lib/compiler/implementation...
File lib/compiler/implementation/elements/elements.dart (right):

http://codereview.chromium.org/11273034/diff/3002/lib/compiler/implementation...
lib/compiler/implementation/elements/elements.dart:151: bool hasSignature() =>
isFunction() || isConstructor();
Let's try to add documentation comments to these methods. Starting with this one
:-)

http://codereview.chromium.org/11273034/diff/3002/lib/compiler/implementation...
lib/compiler/implementation/elements/elements.dart:1656: static bool
isValid(Element e) => !isUnresolved(e);
A documentation comment would also be great here.

http://codereview.chromium.org/11273034/diff/3002/lib/compiler/implementation...
File lib/compiler/implementation/ssa/builder.dart (right):

http://codereview.chromium.org/11273034/diff/3002/lib/compiler/implementation...
lib/compiler/implementation/ssa/builder.dart:3114: {Link<Node> argumentNodes,
I still don't understand why these arguments are optional.

[ngeoffray, 2012-10-25 14:21:58]

http://codereview.chromium.org/11273034/diff/3002/lib/compiler/implementation...
File lib/compiler/implementation/resolution/members.dart (right):

http://codereview.chromium.org/11273034/diff/3002/lib/compiler/implementation...
lib/compiler/implementation/resolution/members.dart:1712: if
(Elements.isValid(target) && target.hasSignature()
Actually, I'd prefer having the applies method taking care of it. If the target
does not have a signature, of course it does not apply.

[ahe, 2012-10-25 15:37:36]

http://codereview.chromium.org/11273034/diff/3002/lib/compiler/implementation...
File lib/compiler/implementation/resolution/members.dart (right):

http://codereview.chromium.org/11273034/diff/3002/lib/compiler/implementation...
lib/compiler/implementation/resolution/members.dart:1712: if
(Elements.isValid(target) && target.hasSignature()
On 2012/10/25 14:21:58, ngeoffray wrote:
> Actually, I'd prefer having the applies method taking care of it. If the
target
> does not have a signature, of course it does not apply.

I agree.

[karlklose, 2012-10-26 11:08:48]

http://codereview.chromium.org/11273034/diff/3002/lib/compiler/implementation...
File lib/compiler/implementation/elements/elements.dart (right):

http://codereview.chromium.org/11273034/diff/3002/lib/compiler/implementation...
lib/compiler/implementation/elements/elements.dart:151: bool hasSignature() =>
isFunction() || isConstructor();
On 2012/10/25 14:07:27, ahe wrote:
> Let's try to add documentation comments to these methods. Starting with this
one
> :-)

Done.

http://codereview.chromium.org/11273034/diff/3002/lib/compiler/implementation...
lib/compiler/implementation/elements/elements.dart:1656: static bool
isValid(Element e) => !isUnresolved(e);
On 2012/10/25 14:07:27, ahe wrote:
> A documentation comment would also be great here.

Done.

http://codereview.chromium.org/11273034/diff/3002/lib/compiler/implementation...
File lib/compiler/implementation/resolution/members.dart (right):

http://codereview.chromium.org/11273034/diff/3002/lib/compiler/implementation...
lib/compiler/implementation/resolution/members.dart:1712: if
(Elements.isValid(target) && target.hasSignature()
We only want to warn if the element is known and has a signature, but does not
apply. We don't want to warn for parameters, fields and foreign elements.

I have added the hasSignature() check to applies, too.

http://codereview.chromium.org/11273034/diff/3002/lib/compiler/implementation...
File lib/compiler/implementation/ssa/builder.dart (right):

http://codereview.chromium.org/11273034/diff/3002/lib/compiler/implementation...
lib/compiler/implementation/ssa/builder.dart:3114: {Link<Node> argumentNodes,
Nice catch, removed argumentValues and made argumentNodes a required argument.

[ngeoffray, 2012-10-26 12:15:57]

LGTM

http://codereview.chromium.org/11273034/diff/13001/lib/compiler/implementatio...
File lib/compiler/implementation/elements/elements.dart (right):

http://codereview.chromium.org/11273034/diff/13001/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:1659: * erroneuous elements.
erroneuous -> erroneous

http://codereview.chromium.org/11273034/diff/13001/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:1661: static bool
isValid(Element e) => !isUnresolved(e);
I'd really prefer not to have this method. isValid, isUnresolved, isErroneous is
just too much.

http://codereview.chromium.org/11273034/diff/13001/tests/language/language_da...
File tests/language/language_dart2js.status (right):

http://codereview.chromium.org/11273034/diff/13001/tests/language/language_da...
tests/language/language_dart2js.status:92: named_parameters_aggregated_test/04:
Fail
Why are we failing this one now?

[karlklose, 2012-10-26 12:23:22]

http://codereview.chromium.org/11273034/diff/13001/tests/language/language_da...
File tests/language/language_dart2js.status (right):

http://codereview.chromium.org/11273034/diff/13001/tests/language/language_da...
tests/language/language_dart2js.status:92: named_parameters_aggregated_test/04:
Fail
We detect that the selector does not match, but this is a runtime error now.

It should be a compile time error, because it uses a duplicate named argument,
but we do not check that currently.

[karlklose, 2012-10-26 12:39:43]

http://codereview.chromium.org/11273034/diff/13001/lib/compiler/implementatio...
File lib/compiler/implementation/elements/elements.dart (right):

http://codereview.chromium.org/11273034/diff/13001/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:1661: static bool
isValid(Element e) => !isUnresolved(e);
Done, removed it.