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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jeff Walden [:Waldo]]

+++ 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]]

Attachment: 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]

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]]

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]]

(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]]

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]]

Attachment: With those changes

Comment 7

[Brendan Eich [: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.

/be

Comment 8

[Jeff Walden [:Waldo]]

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]]

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 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/6d7cf54e67f1

Comment 11

[Jeff Walden [:Waldo]]

http://whereswalden.com/2009/12/21/ecma-262-ed-5-backwards-incompatible-change-coming-to-spidermonkey-and-to-gecko-based-browsers/

Comment 12

[Jeff Walden [:Waldo]]

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]]

(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-4, no longer working on Mozilla)]

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]]

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]]

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]]

I believe this is covered adequately here:

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