Assignments to a property that has a getter but not a setter should only throw a TypeError in strict mode

RESOLVED FIXED in mozilla1.9.3a1



JavaScript Engine
8 years ago
7 years ago


(Reporter: Waldo, Assigned: Waldo)


({dev-doc-complete, testcase})

dev-doc-complete, testcase
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 alpha1+)


(Whiteboard: fixed-in-tracemonkey)


(1 attachment, 1 obsolete attachment)

+++ This bug was initially created as a clone of Bug #523846 +++

var o = { get p() { return "a"; } };
o.p = "b";  // [1]
assertEq(o.p, "a");

function T() {}
T.prototype = o;
y = new T;
y.p = "b";  // [2]
assertEq(y.p, "a");

Assignments [1] and [2] must pass silently in non-strict mode, and both must throw a TypeError in strict mode.

Attachment 411592 [details] [diff] should fix this, perhaps with a few tweaks for the backout performs of bug 478047.  We should hold off on this until 3.6 is out the door, or much more close to it, so we don't stumble over an incompatibility like this while writing tests or porting patches.
Blocks: 445494
No longer blocks: 478047
Keywords: regression
Priority: -- → P1
Target Milestone: --- → mozilla1.9.3a1
blocking2.0: --- → ?
Created attachment 416589 [details] [diff] [review]

There's a part of me that thinks the API consumers with stubbed-setter case should also throw.  I haven't done it here because that would require a little jsapi-test hacking-up and because just removing the else-block doesn't actually solve the problem (it'd try calling a NULL sprop->setter, at a glance), but it wouldn't be hard to implement if wanted.
Assignee: general → jwalden+bmo
Attachment #416589 - Flags: review?(brendan)

Comment 2

8 years ago
Hi Jeff, I don't understand your comment. Does the issue you explain affect the semantics seen by JavaScript code?
It's a SpiderMonkey implementation detail, not germane to ES5 as best as I can tell, of one representation of the get/set field.  In SpiderMonkey a descriptor is effectively like so:

  struct PropertyDescriptor
    boolean enumerable, configurable;
        value val;
        boolean writable;
          NativeFunction native;
          value val;
        } get;
          NativeFunction native;
          value val;
        } set;

A NativeFunction is a C++ function called when the property is accessed or set.  We use it for such things as accessing /regex/.lastIndex, when the value represented by such a property gets used a lot internally.  Another example is [].length, where, given a pointer to an object, the length is always stored in one particular location that's a fixed offset from the object pointer.  This representation was entirely hidden before getters and setters.  __define{G,S}etter__ treated such properties as data descriptors (and current thinking is that Object.getOwnPropertyDescriptor does and will do likewise), so from all appearances such properties are just plain old data descriptors.

The question is what to do when C++ defines a property with get.native and set.native = the default property-set function.  The default function arguably represents the "absence" of a property-set function, and assigning to such a property does nothing.  The question is whether we really think it's a do-nothing setter or the lack of one (either way still exposing the property as a data descriptor).  If it isn't, then it would be fine to have the set silently do nothing.  If it is, setting such a property should throw if strict.

Thinking more about the above, I think we really do want get.native/set.native=default to act as though it were a simple data descriptor.  I'll post an updated patch that makes that change in a second.
(The other part of it is that get.native/set.native=default is invariably accompanied by JSPROP_READONLY, indicating a permanent property which would throw-on-set in strict mode.)
Hmm, "invariably" is too strong a word.  Still, the normal use of such things isn't for get/set-style access with full side effects, just for looking up some value stored somewhat unusually.
Created attachment 416776 [details] [diff] [review]
With those changes
Attachment #416589 - Attachment is obsolete: true
Attachment #416776 - Flags: review?(brendan)
Attachment #416589 - Flags: review?(brendan)
Comment on attachment 416776 [details] [diff] [review]
With those changes

The new tests cuddle catch with } before it, at least one existing does not --
fix while there?

The tests use foo = bar === baz || quux === whatever; and similar style. One
place house style over-parenthesizes is such run-together ... = ... ==....

Why not make js_ReportGetterOnlyAssignment's return type bool and avoid !! in
some calls?

I hope we can get away with this, it's an improvement to the silent-&-strange
old behavior.

Attachment #416776 - Flags: review?(brendan) → review+
js_RGOA mixes with JSBool-returning functions, and it looks like you pay about the same penalty either way.

The test style stuff is, as far as I can tell, consistent with the existing tests (and they're not even JS tests anyway, so style is all over the map, most of it not SpiderMonkey house style) -- or at least internally consistent.  In any case I don't think we've ever imposed style requirements on test code.

I think we'll get away with it -- but we have to do it at the start of a development cycle for it to stick.
Whiteboard: fixed-in-tracemonkey
Returning bool values from a JSBool-return-type function requires no !! or other overhead. We do that all over. What am I missing?


Comment 10

8 years ago
Last Resolved: 8 years ago
Resolution: --- → FIXED
Documentation is for a change for 3.7 -- not 3.6 -- so it's not super-tippy-top priority given 3.6 work to do.  However, it's important it be done sooner than later, so people have as much time to adjust as possible.  My post in comment 11 gives all the details which should be needed to update MDC.  A note should probably go in a "what's new" document, somewhere in existing documentation on getters/setters, and maybe some other places that don't immediately come to memory now.
Keywords: dev-doc-needed
(In reply to comment #9)
> Returning bool values from a JSBool-return-type function requires no !! or
> other overhead. We do that all over. What am I missing?

The point is bool injects into JSBool but not vice versa. We should not treat them as symmetrically related and thereby create extra !! noise or actual test instructions.

Blocks: 480206
This (I think; I don't see anything else likely in the range) seems to have changed our behavior for assignments to properties declared "readonly" in IDL.  See bug 480206.  Was that an expected effect of this change?
That this fixed makes me happy.

That it's an incompatible change to remove an exception makes me slightly less happy, but can we let it ride? It helps unify the behavior of JS "native object" vs. XPConnected "host object" readonly attributes (the former expressible with our const and now with ES5 support).

What's more, this change lets "use strict" enable exceptions uniformly across  native JS and host XPConnect objects. That's a win.

Yes.  Readonly DOM properties in general have to be represented by a getter, so they hit this case as well, intentionally.


8 years ago
blocking2.0: ? → alpha1
I believe this is covered adequately here:
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.