Last Comment Bug 529404 - Assignments to a property that has a getter but not a setter should only throw a TypeError in strict mode
: Assignments to a property that has a getter but not a setter should only thro...
Status: RESOLVED FIXED
fixed-in-tracemonkey
: dev-doc-complete, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.9.3a1
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 523846
Blocks: es5 480206
  Show dependency treegraph
 
Reported: 2009-11-17 15:30 PST by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-02-03 11:52 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
alpha1+


Attachments
Patch (14.51 KB, patch)
2009-12-08 10:23 PST, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
With those changes (17.87 KB, patch)
2009-12-09 10:25 PST, Jeff Walden [:Waldo] (remove +bmo to email)
brendan: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2009-11-17 15:30:55 PST
+++ 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 http://hg.mozilla.org/tracemonkey/rev/ac278013c799 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.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2009-12-08 10:23:48 PST
Created attachment 416589 [details] [diff] [review]
Patch

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.
Comment 2 Mark S. Miller 2009-12-08 15:22:13 PST
Hi Jeff, I don't understand your comment. Does the issue you explain affect the semantics seen by JavaScript code?
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2009-12-08 16:23:47 PST
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;
    union
    {
      struct
      {
        value val;
        boolean writable;
      };
      struct
      {
        union
        {
          NativeFunction native;
          value val;
        } get;
        union
        {
          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.
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2009-12-08 16:36:26 PST
(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.)
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2009-12-09 10:23:23 PST
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.
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2009-12-09 10:25:12 PST
Created attachment 416776 [details] [diff] [review]
With those changes
Comment 7 Brendan Eich [:brendan] 2009-12-17 21:47:20 PST
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.

/be
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2009-12-18 10:27:52 PST
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.

http://hg.mozilla.org/tracemonkey/rev/6d7cf54e67f1
Comment 9 Brendan Eich [:brendan] 2009-12-20 10:58:30 PST
Returning bool values from a JSBool-return-type function requires no !! or other overhead. We do that all over. What am I missing?

/be
Comment 12 Jeff Walden [:Waldo] (remove +bmo to email) 2009-12-21 14:24:56 PST
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.
Comment 13 Brendan Eich [:brendan] 2009-12-21 20:14:24 PST
(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.

/be
Comment 14 David Baron :dbaron: ⌚️UTC-10 (vacation, returning December 19) 2009-12-29 07:02:12 PST
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?
Comment 15 Brendan Eich [:brendan] 2009-12-29 07:40:52 PST
That this fixed virginamerica.com 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.

/be
Comment 16 Jeff Walden [:Waldo] (remove +bmo to email) 2009-12-29 10:47:04 PST
Yes.  Readonly DOM properties in general have to be represented by a getter, so they hit this case as well, intentionally.
Comment 17 Eric Shepherd [:sheppy] 2011-02-03 11:52:53 PST
I believe this is covered adequately here:

https://developer.mozilla.org/en/JavaScript/Strict_mode#Converting_mistakes_into_errors

Note You need to log in before you can comment on or make changes to this bug.