774744 - Error message when using a number as a key for WeakMap isn't very good

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Paul Rouget [:paul]]

(new WeakMap()).set(5, document);
-> TypeError: value is not a non-null object

I don't know if this is the expected behavior or not. If it is, we should have a message a little more understandable (value being the key, not the value).

Comment 1

[Jeff Walden [:Waldo]]

It's expected: WeakMap, to be a truly weak map, must map object->*.  You're right the error message could be clearer, tho.

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to Jeff Walden [:Waldo] (gone July 18-August 26) from comment #1)
> It's expected: WeakMap, to be a truly weak map, must map object->*.  You're
> right the error message could be clearer, tho.

You might transform that into a warning such as:

    the key is pass by copy, no entry added.

Comment 3

[Jason Orendorff [:jorendorff]]

(In reply to Nicolas B. Pierron [:pierron] from comment #2)
> You might transform that into a warning such as:
> 
>     the key is pass by copy, no entry added.

I can't tell what this means.

How about: "WeakMap key must be an object"?

Comment 4

[André Bargull [:anba]]

The error message now reads:
> js> (new WeakMap()).set(5, "");       
> typein:1:1 TypeError: 5 is not a non-null object

Is this error message meaningful enough or are additional changes needed?

Comment 5

[Tooru Fujisawa [:arai]]

I'll make it say "WeakMap key must be a non-null object, got 5".
let me know if there's better wording for ", got 5" part ;)

Comment 6

[Jeff Walden [:Waldo]]

Might as well make it "must be an object" -- object by definition is non-null, broken stuff like |typeof| aside.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Clarify the argument position or role of the value in some NOT_NONNULL_OBJECT error.

Added 2 errors and helper functions:
  * NonNullObjectWithName
    "{0} must be an object, got {1}"
  * NonNullObjectArg
    "{0} argument of {1} must be an object, got {2}"

actually former one can handle most case, but I added latter one to handle ProxyCreate case, that function name is variable.

and used either of them where appropriate.

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 8836288 [details] [diff] [review]
Clarify the argument position or role of the value in some NOT_NONNULL_OBJECT error.

Review of attachment 8836288 [details] [diff] [review]:
-----------------------------------------------------------------

::: addon-sdk/source/test/test-weak-set.js
@@ +80,5 @@
>  
>    assert.throws(() => {
>      add(items, 'foo');
>    },
> +  /is not a non-null object|must be an object/,

It'd be really nice if these tests were changed to *not* check the error message, which is non-standard, will not be standardized, and (ahem) changes from time to time.  I should say, I'm also a little confused why the tests are for *alternations* -- why not just the new string?  Is it that these they have to handle both behaviors temporarily?  Seems like a great reason to remove the message testing entirely.

::: js/src/builtin/Reflect.cpp
@@ +26,5 @@
>      CallArgs args = CallArgsFromVp(argc, vp);
>  
>      // Step 1.
> +    RootedObject obj(cx, NonNullObjectArg(cx, "1st", "Reflect.defineProperty",
> +                                          args.get(0)));

Seems like NonNullObjectArg should be

  static bool
  NonNullObjectArg(JSContext* cx, const JS::CallArgs& args, uint32_t argno, const char* func);

and then that internally can map |argno| to the right string -- either manually, 0 to "First" and so on, or by generating it into a fresh string, or by both (manually for small numbers, then programmatically as fallback).

::: js/src/builtin/Reflect.js
@@ +35,5 @@
>  
>      // Step 2.
> +    if (!IsObject(argumentsList)) {
> +        ThrowTypeError(JSMSG_NOT_NONNULL_OBJECT_ARG, "3rd", "Reflect.apply",
> +                       DecompileArg(2, argumentsList));

If we had to add another intrinsic to permit passing along an argument number and not a string, for consistency with the C++ caller interface, it wouldn't be the end of the world, IMO.

Alternatively -- the argument *number* is perhaps easiest to slot in without thinking.  But don't users really want to see some sort of description of exactly what the argument *means*?  Like, wouldn't it be better to have as error message

"""
`argumentsList` argument to Reflect.apply must be an object
"""

instead of this awkward thing of

"""
3rd argument to Reflect.apply must be an object
"""

?  This is somewhat a genuine question, not a demand for such change.  The nature of the argument is more informative -- if you know the argument ordering (or slightly forgot it).  But if you don't know the ordering (why are you calling a function whose ordering you don't know?), literal numeric index is at least precise.

Comment 9

[Tooru Fujisawa [:arai]]

Thank you for your feedback :)

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)
> Alternatively -- the argument *number* is perhaps easiest to slot in without
> thinking.  But don't users really want to see some sort of description of
> exactly what the argument *means*?  Like, wouldn't it be better to have as
> error message
> 
> """
> `argumentsList` argument to Reflect.apply must be an object
> """
> 
> instead of this awkward thing of
> 
> """
> 3rd argument to Reflect.apply must be an object
> """
> 
> ?  This is somewhat a genuine question, not a demand for such change.  The
> nature of the argument is more informative -- if you know the argument
> ordering (or slightly forgot it).  But if you don't know the ordering (why
> are you calling a function whose ordering you don't know?), literal numeric
> index is at least precise.

There are 3 issues to use argument name (parameter name?) in general

  1. the ECMAScript spec sometimes uses single-character names, like following:
       * Object.getOwnPropertyDescriptor ( O, P )
       * RegExp.prototype.test ( S )
       * Promise.reject ( r )
       * Reflect.set ( target, propertyKey, V [ , receiver ] )
     of course in other most case the name is descriptive enough and we can use it tho

  2. we should make sure MDN documentations are using the same name as the spec.

  3. if the user mis-remembers the order and that's the reason of the error,
     saying the argument name may be misleading
     (perhaps we should say both index and name...?)

printing an error note with the function signature might help some case,
but signature is not so much helpful compared to C++ error, since it doesn't have types for each argument... :/

  Error: `argumentsList` argument to Reflect.apply must be an object
    at foo.js:1:1
  Note: for Reflect.apply(target, thisArgument, argumentsList)

Comment 10

[Tooru Fujisawa [:arai]]

apart from the above issues, it might be nice to link to MDN documentation of each function, just like [Learn More] for error message specific to builtin function parameters.

Comment 11

[Tooru Fujisawa [:arai]]

so far, confirmed all cases touched by this patch is not single-character name and the same name is used between documentation and the spec.

Comment 12

[Jeff Walden [:Waldo]]

A long time ago bterlson told me he was 100% on board with doing s/O/obj/g on the spec for readability, and for more easily distinguishing O from 0.  I expect it would be fairly easy to extend the idea to forbid all single-letter variable names.  (But of course someone has to do both bits of work, and I've been a bit lazy about actually getting to it.)

Comment 13

[Tooru Fujisawa [:arai]]

With "SOMETHING must be an object, got SOMETHING_ELSE" message, expression decompilation doesn't fit.

code:      new WeakMap([[1, 2]])
current:   TypeError: nextItem[0] is not a non-null object
WIP patch: TypeError: WeakMap key must be an object, got nextItem[0]

maybe I should just stringify the value in that case.

Comment 14

[Tooru Fujisawa [:arai]]

to avoid side effect as much as possible, we could just drop the "got SOMETHING_ELSE" part, or reword somehow.

Comment 15

[Jeff Walden [:Waldo]]

But stringifying the value doesn't have side effects if it's not an object, right?

Comment 16

[Tooru Fujisawa [:arai]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #15)
> But stringifying the value doesn't have side effects if it's not an object,
> right?

oh, good point :)
okay, I'll stringify it

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Clarify the parameter name or role of the value in some NOT_NONNULL_OBJECT error.

Changed to say the parameter name, and also show the source of the value.

Comment 18

[Jeff Walden [:Waldo]]

Comment on attachment 8843658 [details] [diff] [review]
Clarify the parameter name or role of the value in some NOT_NONNULL_OBJECT error.

Review of attachment 8843658 [details] [diff] [review]:
-----------------------------------------------------------------

Leaning fairly hard on Bugzilla interdiff, looks fine with that in mind.

::: js/src/jsobj.cpp
@@ +97,5 @@
> +    JSAutoByteString bytes;
> +    const char* chars = ValueToSourceForError(cx, value, bytes);
> +    if (chars)
> +        JS_ReportErrorNumberLatin1(cx, GetErrorMessage, nullptr, JSMSG_NOT_NONNULL_OBJECT_ARG,
> +                                   nth, fun, chars);

if (const char* chars = ValueToSourceForError(...))
    JS_ReportErrorNumberLatin1(...);

and if that JS_RENL1 doesn't fit in 99ch, add braces.

@@ +110,5 @@
> +    JSAutoByteString bytes;
> +    const char* chars = ValueToSourceForError(cx, value, bytes);
> +    if (chars)
> +        JS_ReportErrorNumberLatin1(cx, GetErrorMessage, nullptr, JSMSG_NOT_NONNULL_OBJECT_NAME,
> +                                   name, chars);

Same thing as above.

::: js/src/jsobj.h
@@ +1346,5 @@
> +extern void
> +ReportNotObjectArg(JSContext* cx, const char* nth, const char* fun, const Value& v);
> +
> +inline JSObject*
> +NonNullObjectArg(JSContext* cx, const char* nth, const char* fun, const Value& v)

Make these |const Value&| arguments be |Handle<Value>| instead so there's no possible GC-dodginess anyone must consider.  Everyone's passing handles anyway, so that should be just as efficient.

::: js/src/vm/SelfHosting.cpp
@@ +154,5 @@
> +intrinsic_ToSource(JSContext* cx, unsigned argc, Value* vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    RootedString str(cx, ValueToSource(cx, args[0]));
> +    if (!str)

Just |JSString* str =|, as the value's immediately returned so no real need for rooting.

Comment 19

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8176df5f67ef927bf88bfcf26265e74f2ad7d18
Bug 774744 - Clarify the parameter name or role of the value in some NOT_NONNULL_OBJECT error. r=jwalden

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d8176df5f67e