596728 - Error messages produced by Array.prototype functions that encounter read-only properties are unclear

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

This seemed unhelpful to me:

js> a=Object.defineProperty([1,2,3],0,{writable:false})
[1, 2, 3]
js> a.reverse()
typein:2: TypeError: a.reverse() is read-only

This message works nicely for assignments to properties that appear directly in JavaScript code (|a[0] = 1| would get you "TypeError: a[0] is read-only), but when it's an Array.prototype function that elicits the error, it's pretty bad: a.reverse() is not a property.

It would be nicer if the message mentioned '0' in addition to 'a.reverse()'.

I believe cases where a function tries to delete a non-configurable property are similar.

ES5 requires that these functions throw a TypeError rather than failing silently, so that's all correct.

Comment 1

[Brendan Eich [:brendan]]

This seems like a simple matter of giving js_DecompileValueGenerator more info:

diff -u b/js/src/jsobj.cpp b/js/src/jsobj.cpp
--- b/js/src/jsobj.cpp
+++ b/js/src/jsobj.cpp
@@ -4913,7 +4913,7 @@
 ReportReadOnly(JSContext* cx, jsid id, uintN flags)
 {
     return js_ReportValueErrorFlags(cx, flags, JSMSG_READ_ONLY,
-                                    JSDVG_IGNORE_STACK, IdToValue(id), NULL,
+                                    JSDVG_SEARCH_STACK, IdToValue(id), NULL,
                                     NULL, NULL);
 }
 
yields

typein:2: TypeError: 0 is read-only

We can do even better, but this is a good start.

/be

Comment 2

[Brendan Eich [:brendan]]

The general value error reporter interface is

/*
 * Report error using js_DecompileValueGenerator(cx, spindex, v, fallback) as
 * the first argument for the error message. If the error message has less
 * then 3 arguments, use null for arg1 or arg2.
 */
extern JSBool
js_ReportValueErrorFlags(JSContext *cx, uintN flags, const uintN errorNumber,
                         intN spindex, const js::Value &v, JSString *fallback,
                         const char *arg1, const char *arg2);

This is the targeted solution, if you can propagate the correct spindex in.

/be

Comment 3

[Luke Wagner [:luke]]

> This is the targeted solution, if you can propagate the correct spindex in.

Btw, this means that you must know that the function is being called directly from script and not from js::Invoke (which is often not true for a JSNative).  Alternatively, you can do what js_ReportIsNotFunction does and use a Value* to dynamically figure out if there is an spindex.  We talked earlier about factoring this functionality out of js_ReportIsNotFunction so that other error-reporting functions can use it.

Comment 4

[Brendan Eich [:brendan]]

Taking, I have to care about blame since it has been one of our strengths over the years vs. competition.

/be

Comment 5

[Tom S [:evilpie]]

This seems to be fixed.