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

RESOLVED FIXED in Firefox 55

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
9 months ago

People

(Reporter: paul, Assigned: arai)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [js:p2])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
(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).
It's expected: WeakMap, to be a truly weak map, must map object->*.  You're right the error message could be clearer, tho.
Summary: Can't use a number as a key for WeakMap → Error message when using a number as a key for WeakMap isn't very good
(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.
(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"?
Whiteboard: [js:p2]
Assignee: general → nobody
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?
Blocks: 622261
(Assignee)

Comment 5

10 months ago
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 ;)
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Might as well make it "must be an object" -- object by definition is non-null, broken stuff like |typeof| aside.
(Assignee)

Comment 7

10 months ago
Created attachment 8836288 [details] [diff] [review]
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.
Attachment #8836288 - Flags: review?(jwalden+bmo)
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.
Attachment #8836288 - Flags: review?(jwalden+bmo) → feedback+
(Assignee)

Comment 9

9 months ago
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)
(Assignee)

Comment 10

9 months ago
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.
(Assignee)

Comment 11

9 months ago
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.
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.)
(Assignee)

Comment 13

9 months ago
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.
(Assignee)

Comment 14

9 months ago
to avoid side effect as much as possible, we could just drop the "got SOMETHING_ELSE" part, or reword somehow.
But stringifying the value doesn't have side effects if it's not an object, right?
(Assignee)

Comment 16

9 months ago
(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
(Assignee)

Comment 17

9 months ago
Created attachment 8843658 [details] [diff] [review]
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.
Attachment #8836288 - Attachment is obsolete: true
Attachment #8843658 - Flags: review?(jwalden+bmo)
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.
Attachment #8843658 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 19

9 months ago
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

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d8176df5f67e
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.