Change the field and constructor parameter types of NoSuchMethodError to Symbol.

Change the field and constructor parameter types of NoSuchMethodError to Symbol.

The NSME uses strings for names. It should use symbols.

This change modifies the VM libraries only enough to make the signature change
not crash. It will probably need cleanup to ensure that symbols are passed
to the internal constructor directly, instead of strings that have to be
converted.

(Also fixes a type error in Map.fromIterable that was hit by the code).

BUG= http://dartbug.com/11190
R=ahe@google.com, regis@google.com

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

[regis, 2013-08-29 23:42:42]

LGTM

Cc'ing Ryan who is currently working in that area with mirrors.

[Lasse Reichstein Nielsen, 2013-08-30 05:49:09]

https://codereview.chromium.org/23486007/diff/1/sdk/lib/_internal/lib/js_mirr...
File sdk/lib/_internal/lib/js_mirrors.dart (right):

https://codereview.chromium.org/23486007/diff/1/sdk/lib/_internal/lib/js_mirr...
sdk/lib/_internal/lib/js_mirrors.dart:512: // TODO(ahe): Pass namedArguments
when NoSuchMethodError has
Just noticed this TODO, will do that also.

https://codereview.chromium.org/23486007/diff/1/sdk/lib/_internal/lib/js_mirr...
sdk/lib/_internal/lib/js_mirrors.dart:519: // TODO(ahe): What receiver to use?
this.reflectee? I think that is what the non-mirror call would use.

[ahe, 2013-08-30 06:46:35]

sdk/... and tests/... LGTM (assuming you rename the method "a").

https://codereview.chromium.org/23486007/diff/5001/sdk/lib/_internal/lib/js_m...
File sdk/lib/_internal/lib/js_mirrors.dart (right):

https://codereview.chromium.org/23486007/diff/5001/sdk/lib/_internal/lib/js_m...
sdk/lib/_internal/lib/js_mirrors.dart:381: Symbol a(Symbol symbol) =>
s("${n(symbol)}=");
Please use a descriptive name. We only use "s" and "n" because they are
extremely frequent, this is not a pattern to follow for a method that is called
three times.

https://codereview.chromium.org/23486007/diff/5001/sdk/lib/collection/hash_ma...
File sdk/lib/collection/hash_map.dart (right):

https://codereview.chromium.org/23486007/diff/5001/sdk/lib/collection/hash_ma...
sdk/lib/collection/hash_map.dart:41: factory HashMap.fromIterable(Iterable
iterable,
Unrelated change, I think.

https://codereview.chromium.org/23486007/diff/5001/sdk/lib/core/errors.dart
File sdk/lib/core/errors.dart (right):

https://codereview.chromium.org/23486007/diff/5001/sdk/lib/core/errors.dart#n...
sdk/lib/core/errors.dart:191: NoSuchMethodError(Object receiver,
FWIW, this constructor is somewhat useless for dart2js, and so are the private
fields.

We need to implement the interface of this class, but that should cause type
warnings as we cannot implement the private fields.

https://codereview.chromium.org/23486007/diff/5001/sdk/lib/core/errors.dart#n...
sdk/lib/core/errors.dart:196: : _receiver = receiver,
Why not use "this" parameters?

[ngeoffray, 2013-08-30 06:51:11]

DBC

https://codereview.chromium.org/23486007/diff/5001/sdk/lib/_internal/lib/js_m...
File sdk/lib/_internal/lib/js_mirrors.dart (right):

https://codereview.chromium.org/23486007/diff/5001/sdk/lib/_internal/lib/js_m...
sdk/lib/_internal/lib/js_mirrors.dart:256: throw new NoSuchMethodError(this,
filedName, [], null);
filedName ? -> fieldName

https://codereview.chromium.org/23486007/diff/5001/sdk/lib/_internal/lib/js_m...
sdk/lib/_internal/lib/js_mirrors.dart:1318: [], null);
one line?

[Lasse Reichstein Nielsen, 2013-08-30 14:06:02]

https://codereview.chromium.org/23486007/diff/5001/sdk/lib/_internal/lib/js_m...
File sdk/lib/_internal/lib/js_mirrors.dart (right):

https://codereview.chromium.org/23486007/diff/5001/sdk/lib/_internal/lib/js_m...
sdk/lib/_internal/lib/js_mirrors.dart:256: throw new NoSuchMethodError(this,
filedName, [], null);
On 2013/08/30 06:51:12, ngeoffray wrote:
> filedName ? -> fieldName

Done.

https://codereview.chromium.org/23486007/diff/5001/sdk/lib/_internal/lib/js_m...
sdk/lib/_internal/lib/js_mirrors.dart:381: Symbol a(Symbol symbol) =>
s("${n(symbol)}=");
Changed to "setterSymbol".

https://codereview.chromium.org/23486007/diff/5001/sdk/lib/_internal/lib/js_m...
sdk/lib/_internal/lib/js_mirrors.dart:1318: [], null);
On 2013/08/30 06:51:12, ngeoffray wrote:
> one line?

Done.

https://codereview.chromium.org/23486007/diff/5001/sdk/lib/collection/hash_ma...
File sdk/lib/collection/hash_map.dart (right):

https://codereview.chromium.org/23486007/diff/5001/sdk/lib/collection/hash_ma...
sdk/lib/collection/hash_map.dart:41: factory HashMap.fromIterable(Iterable
iterable,
Just a bug that I happened to hit with this change, so I had to fix it.

https://codereview.chromium.org/23486007/diff/5001/sdk/lib/core/errors.dart
File sdk/lib/core/errors.dart (right):

https://codereview.chromium.org/23486007/diff/5001/sdk/lib/core/errors.dart#n...
sdk/lib/core/errors.dart:191: NoSuchMethodError(Object receiver,
I agree that this is an odd signature for a class that may need different
implementations.

We should probably switch this to be an abstract class with an external
constructor.
If we need the constructor at all - it's only here because the specification
uses it to specify what is thrown in no-such-method cases. It could equally well
specify that "some error is thrown", and let each implementation handle it the
best way it can.

We can still do that, and keep an abstract interface and a default
user-accessible implementation.

https://codereview.chromium.org/23486007/diff/5001/sdk/lib/core/errors.dart#n...
sdk/lib/core/errors.dart:196: : _receiver = receiver,
This is a public constructor, so that would make the documented name of a
"this._memberName" parameter the private name "_memberName".
For documentation, I prefer to use public names for parameters of public
functions and constructors, and only use "this.foo" if the foo field itself is
also public.

[floitsch, 2013-08-30 15:36:44]

DBC.

https://codereview.chromium.org/23486007/diff/14001/sdk/lib/core/errors.dart
File sdk/lib/core/errors.dart (right):

https://codereview.chromium.org/23486007/diff/14001/sdk/lib/core/errors.dart#...
sdk/lib/core/errors.dart:172: * That is, the object on which the method was
attempted called.
attempted to be called.

https://codereview.chromium.org/23486007/diff/14001/sdk/lib/core/errors.dart#...
sdk/lib/core/errors.dart:173: * If this is `null`, it is interpreted as a call
to a top-level function
If it is `null` ... (don't use "this")

https://codereview.chromium.org/23486007/diff/14001/sdk/lib/core/errors.dart#...
sdk/lib/core/errors.dart:176: * The [memberNamed] is a [Symbol] representing the
name of the called method
memberName

[Lasse Reichstein Nielsen, 2013-09-02 05:58:17]

Committed patchset #4 manually as r26985 (presubmit successful).

[Lasse Reichstein Nielsen, 2013-09-02 09:36:28]

https://codereview.chromium.org/23486007/diff/14001/sdk/lib/core/errors.dart
File sdk/lib/core/errors.dart (right):

https://codereview.chromium.org/23486007/diff/14001/sdk/lib/core/errors.dart#...
sdk/lib/core/errors.dart:172: * That is, the object on which the method was
attempted called.
I think the original sounds more correct, "attempted to be called" doesn't sound
right.

https://codereview.chromium.org/23486007/diff/14001/sdk/lib/core/errors.dart#...
sdk/lib/core/errors.dart:173: * If this is `null`, it is interpreted as a call
to a top-level function
As a rule of thumb, I try to avoid "it" as well. It too easily becomes
ambiguous, and since I know what it means, it's hard to see. Change to "If the
receiver is `null`,"

https://codereview.chromium.org/23486007/diff/14001/sdk/lib/core/errors.dart#...
sdk/lib/core/errors.dart:176: * The [memberNamed] is a [Symbol] representing the
name of the called method
On 2013/08/30 15:36:44, floitsch wrote:
> memberName

Done.