Closed Bug 596728 Opened 12 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: jimb, Assigned: brendan)

References

(Blocks 1 open bug)

Details

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.
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
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
> 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.
Taking, I have to care about blame since it has been one of our strengths over the years vs. competition.

/be
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Assignee: general → brendan

This seems to be fixed.

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.