353116 - "has no properties" is misleading and should be replaced with "is null or undefined"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jesse Ruderman]

The "has no properties" error message should be replaced by "is null or undefined" (or two error messages, "is null" and "is undefined").  Here are two examples demonstrating why the current message is confusing.


(1)

js> j = {a:3}
[object Object]
js> j.x.y = 4 
typein:13: TypeError: j.x has no properties

"I don't care that it has no properties.  I'm not trying to access its properties, I'm trying to *give* it a property!"


(2)

js> j = {}
[object Object]
js> j.x.y
typein:2: TypeError: j.x has no properties
js> j.x = {}
[object Object]
js> j.x.y 

"j.x still has no properties, so why did the error go away?"

Comment 1

[Rich Dougherty]

Attachment: Patch v1

This looks like a good first bug. :-)

Here's a patch that gives better error messages. Note that in js_ValueToNonNullObject() I assume that the erroneous value 'v' must be either null or undefined. Unfortunately the exact behaviour of the call to js_ValueToObject() is a little unclear to me so I'm not 100% sure about this assumption.

js> x.y
typein:1: ReferenceError: x is not defined
js> x = undefined
js> x.y
typein:3: TypeError: undefined value x does not support properties
js> x = null
null
js> x.y
typein:5: TypeError: null value x does not support properties

Comment 2

[Jesse Ruderman]

Comment on attachment 270714 [details] [diff] [review]
Patch v1

I'm not in love with the new wording, but maybe Brendan can review the code and suggest better wording.

Would it make sense to make it a single MSG_DEF with a {0} to be replaced by "null" or "undefined"?  That might end up simplifying the code by eliminating the need for separate statement-level branches.

Comment 3

[Jeff Walden [:Waldo]]

Extra brownie points for "not defined" if that's what it is, as opposed to being defined to "undefined", if that doesn't seem overdemanding?  ;-)

Comment 4

[Rich Dougherty]

Attachment: Patch v2

Thanks for the feedback! I've changed the error messages, hopefully they're better? I eliminated the statement-level branches and instead use a conditional expression to pick different error numbers. I didn't change to a single MSG_DEF, because I couldn't think of a better place to store the alternative string literals ("not defined"/"is null") - any tips?

js> x.y
typein:1: ReferenceError: x is not defined
js> x = undefined
js> x.y
typein:3: TypeError: x not defined: has no properties
js> x = null
null
js> x.y
typein:5: TypeError: x is null: has no properties

Comment 5

[Nickolay_Ponomarev]

> js> x = undefined
> js> x.y
> typein:3: TypeError: x not defined: has no properties

x is actually defined to "undefined", isn't it?

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 270816 [details] [diff] [review]
Patch v2

Thanks for taking this on. A few detailed comments, but first, one design-level one:

It's still possible to get "null is null: has no properties" or "undefined not defined: has no properties", and we would like to avoid that. Also, the clipped sentence fragment phrases separated by a colon is not ideal. So it still seems best to me for the format string to say "{0} has no properties" if the argument expanded for {0} is in fact "null" or "undefined", but to say "{0} is {1}, which has no properties" if we have actually decompiled as {0} a meaningful expression that evaluated to null or undefined.

>diff -r f3402a92413d js/src/js.msg
>--- a/js/src/js.msg	Tue Jul 03 10:48:56 2007 +1200
>+++ b/js/src/js.msg	Wed Jul 04 01:23:04 2007 +1200
>@@ -118,7 +118,7 @@ MSG_DEF(JSMSG_CYCLIC_VALUE,            3
> MSG_DEF(JSMSG_CYCLIC_VALUE,            36, 1, JSEXN_ERR, "cyclic {0} value")
> MSG_DEF(JSMSG_COMPILE_EXECED_SCRIPT,   37, 0, JSEXN_TYPEERR, "cannot compile over a script that is currently executing")
> MSG_DEF(JSMSG_CANT_CONVERT_TO,         38, 2, JSEXN_TYPEERR, "can't convert {0} to {1}")
>-MSG_DEF(JSMSG_NO_PROPERTIES,           39, 1, JSEXN_TYPEERR, "{0} has no properties")
>+MSG_DEF(JSMSG_UNDEFINED_NO_PROPERTIES, 39, 1, JSEXN_TYPEERR, "{0} not defined: has no properties")

As Nickolay points out, you want "is undefined", not "not defined" here -- which means as Jesse pointed out you can use one JSMSG_NO_PROPERTIES format taking {1} as the "null" or "undefined" parameter. There's no need for extra msg catalog entries if localizations would never vary the spelling of "null" and "undefined", which are fixed by the JS language's lexical grammar.

>+                    js_ReportValueError(cx,
>+                                        JSVAL_IS_VOID(lval)
>+                                            ? JSMSG_UNDEFINED_NO_PROPERTIES
>+                                            : JSMSG_NULL_NO_PROPERTIES,

Nit: prevailing multiline ?: style underhangs the ? and : in the same column as the condition starts.

>+        js_ReportValueError(cx,
>+                            JSVAL_IS_VOID(v)
>+                                ? JSMSG_UNDEFINED_NO_PROPERTIES
>+                                : JSMSG_NULL_NO_PROPERTIES,

Ditto.

/be

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> (From update of attachment 270816 [details] [diff] [review])
> So it still seems
> best to me for the format string to say "{0} has no properties" if the argument
> expanded for {0} is in fact "null" or "undefined", but to say "{0} is {1},
> which has no properties" if we have actually decompiled as {0} a meaningful
> expression that evaluated to null or undefined.

Of course comment 0 shows a case (assignment to x.y.z where x.y is null or undefined) that (as Jesse argues) goes against "has no properties". It could favor "can't have properties" but that seems like gilding the lilly. For that case, "x.y is null" or "x.y is undefined" is best, I agree. We just need to avoid saying "null is null" or "undefined is undefined", which requires some special casing.

A helper function like js_ReportIsNotDefined, say js_ReportNullOrUndefined, could handle this logic. It would choose between JSMSG_NULL_OR_UNDEFINED, which would format "{0} is {1}" to handle the "x.y is null" etc. case, and JSMSG_NO_PROPERTIES which would say "{0} has no properties" where {0} expands to "null" or "undefined" -- or something better than "has no properties" (but nothing comes to mind at the moment).

BTW, "null" is already commoned as js_null_str, and "undefined" as js_type_strs[JSTYPE_VOID].

/be

Comment 8

[Jesse Ruderman]

What's wrong with "can't have properties"?

Comment 9

[Rich Dougherty]

Attachment: Patch v3

Here's an updated version of the patch. A helper function is now used to pick the error message to display. Once again, thanks for the feedback and advice.

A few questions:

* I detect null/undefined values by calling js_DecompileValueGenerator and then
comparing the result with js_null_str/js_type_strs[JSTYPE_VOID].
Is there a better way to do this?

* The helper function currently takes spindex and fallback as parameters. Both uses of the function pass the same values for these parameters. Should I keep these as parameters, or just hardcode the values into the function?

js> undefined.y
typein:1: TypeError: undefined has no properties
js> null.y
typein:2: TypeError: null has no properties
js> x = undefined; x.y
typein:3: TypeError: x is undefined
js> x = null; x.y
typein:4: TypeError: x is null

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 271327 [details] [diff] [review]
Patch v3

>     if (!obj) {
>-        js_ReportValueError(cx, JSMSG_NO_PROPERTIES,
>-                            JSDVG_SEARCH_STACK, v, NULL);
>+        js_ReportIsNullOrUndefined(cx, JSDVG_SEARCH_STACK, v, NULL);
>     }

Looks good, only nit is here: no need to brace single-line then for single-line if.

Brian, could you land this one with that nit picked? Thanks,

/be

Comment 11

[Brian Crowder]

Comment on attachment 271327 [details] [diff] [review]
Patch v3

Rich:  Sorry, Patch v3 isn't applying cleanly for me, and there isn't quite enough context to figure out where this hunk in jsinterp.c belongs:

***************
*** 5663,5670 ****
                      i = JSProto_Boolean;
                  } else {
                      JS_ASSERT(JSVAL_IS_NULL(lval) || JSVAL_IS_VOID(lval));
-                     js_ReportValueError(cx, JSMSG_NO_PROPERTIES,
-                                         JSDVG_SEARCH_STACK, lval, NULL);
                      ok = JS_FALSE;
                      goto out;
                  }
--- 5663,5670 ----
                      i = JSProto_Boolean;
                  } else {
                      JS_ASSERT(JSVAL_IS_NULL(lval) || JSVAL_IS_VOID(lval));
+                     js_ReportIsNullOrUndefined(cx, JSDVG_SEARCH_STACK, lval,
+                                                NULL);
                      ok = JS_FALSE;
                      goto out;
                  }

Mind posting a fresh patch with a few more lines of context?  Thanks, and sorry for letting this rot on you.

Comment 12

[Jeff Walden [:Waldo]]

|cvs diff -pu8| is what Mozilla traditionally recommends for patches.

Comment 13

[Rich Dougherty]

Attachment: Patch v4

Brian: Rot is probably due to the fact that it took me a couple of weeks
before I realised I needed to set the review flag! Patch should now apply
to HEAD:
- Changed to use JS_TYPE_STR(), rather than js_null_str and js_type_strs[].
- Made change suggested in comment 10.
- Added more context.

Jeff: Thanks! I'm actually using Mercurial to manage local changes. After
installing the extdiff plugin, I used "hg extdiff -o -Npru8".

Comment 14

[Igor Bukanov]

Comment on attachment 275633 [details] [diff] [review]
Patch v4

> JSBool
>+js_ReportIsNullOrUndefined(JSContext *cx, intN spindex, jsval v,
>+                           JSString *fallback)
>+    if (strcmp(bytes, JS_TYPE_STR(JSTYPE_VOID)) == 0 ||
>+        strcmp(bytes, JS_TYPE_STR(JSTYPE_NULL)) == 0) {

Use js_undefined_str or js_null_str in place of JS_TYPE_STR(JSTYPE_VOID) or JS_TYPE_STR(JSTYPE_NULL) here and bellow for smaller code.

Comment 15

[Rich Dougherty]

Attachment: Patch v5

Thanks Igor, updated to reflect your comment. :-)

Comment 16

[Brendan Eich [:brendan]]

Who will land an updated patch? Igor, Brian?

/be

Comment 17

[Brian Crowder]

Attachment: updated version of patch v5

Thanks, Rich.  Landing this version shortly.

Comment 19

[Brian Crowder]

approval-ping

Comment 20

[Reed Loden [:reed]]

(In reply to comment #19)
> approval-ping

Approvals aren't being triaged by drivers until after beta is ready. If you think this should make M9, you need to request approvalM9. Otherwise, the appropriate driver will get to it once beta is over and the tree has been reopened for general check-ins.

Comment 21

[Damon Sicore (:damons)]

Comment on attachment 283221 [details] [diff] [review]
updated version of patch v5

a=release drivers

Comment 22

[Reed Loden [:reed]]

Checking in js/src/js.msg;
/cvsroot/mozilla/js/src/js.msg,v  <--  js.msg
new revision: 3.80; previous revision: 3.79
done
Checking in js/src/jscntxt.c;
/cvsroot/mozilla/js/src/jscntxt.c,v  <--  jscntxt.c
new revision: 3.121; previous revision: 3.120
done
Checking in js/src/jscntxt.h;
/cvsroot/mozilla/js/src/jscntxt.h,v  <--  jscntxt.h
new revision: 3.168; previous revision: 3.167
done
Checking in js/src/jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.379; previous revision: 3.378
done
Checking in js/src/jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.391; previous revision: 3.390
done

Comment 23

[Bob Clary [:bc] (inactive)]

existing tests updated in bug 404355.

Checking in regress-353116.js;
/cvsroot/mozilla/js/tests/js1_8/extensions/regress-353116.js,v  <--  regress-353116.js
initial revision: 1.1
done

Comment 24

[Bob Clary [:bc] (inactive)]

verified fixed 2007-12-09 1.9.0 linux|mac|win

Comment 25

[Jesse Ruderman]

Filed bug 418573 on expanding "is null" / "is undefined" to cover some cases where we currently say "is not a function".