914220 - Move TypedObject names into a "TypedObject" quasi-module

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

Eventually typed object names will be part of a Typed Object global. For now, we will expose a single global (`TypedObject`) and put all names within there.

Comment 1

[Niko Matsakis [:nmatsakis]]

Attachment: Bug914220.diff

Waldo -- I put you down for review because I wanted advice on whether I'm using the various initialization mechanisms correctly. Basically what I wanted to do was just to have the first access to the global `TypedObject` (which contains all the typed object types globals) populate and create the typed object types.

Comment 2

[Niko Matsakis [:nmatsakis]]

Adding dependency on bug 898349 because the patches are rebased on top of those, not because of a logical dependency.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

nmatsakis noted on IRC that "the various globals related to typed objects (ArrayType etc) are being moved into a "module", so you would access them like `TypedObject.ArrayType`.

Comment 4

[Niko Matsakis [:nmatsakis]]

Attachment: Bug914220.diff

Rebased, fixed various bugs I encountered later when doing more extensive testing.

Comment 5

[Niko Matsakis [:nmatsakis]]

Waldo -- to answer your question from IRC, the *precise* prototype chain that will be used for typed objects is still somewhat TBD, but the reason for ArrayType.prototype.prototype is that ArrayType produces *type constructors*. Consider:

    var MyArrayType = new ArrayType(...);
        // MyArrayType.__proto__ == ArrayType.prototype
        // MyArrayType.prototype.__proto__ == ArrayType.prototype.prototype
    var myArray = new MyArrayType();
        // myArray.__proto__.__proto__ == MyArrayType.prototype

Comment 6

[Niko Matsakis [:nmatsakis]]

Shu pointed out that I made a mistake in my final comment. I should have written:

// myArray.__proto__ == MyArrayType.prototype

Comment 7

[Niko Matsakis [:nmatsakis]]

Attachment: Bug914220.diff

Further rebased, bug fixed.

Comment 8

[Niko Matsakis [:nmatsakis]]

Attachment: Bug914220.diff

Rebased, fixed a few more minor bugs -- most notably changed def'n of enumerable properties for arrays to exclude length.

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 807230 [details] [diff] [review]
Bug914220.diff

Review of attachment 807230 [details] [diff] [review]:
-----------------------------------------------------------------

This is enough little nitpicks that arguably I should see this a second time.  But, I'm afraid if I do see it again, I'll be delaying stuff even more, for what overall is mostly nitpicks (with some substantive things in there, but realistically the set of attributes on properties is a spec-conformance thing, not something that horribly breaks people if we have it temporarily wrong).  So let's try a little compromise: make the set of changes, attach them as a secondary diff atop the patch here (rebased however has been necessary since this patch was posted), and land the two patches combined into a single one.  Then I can take a second look at stuff, without possibly holding up the overall forward progress here.

::: js/src/builtin/TypedObject.cpp
@@ +26,5 @@
>  #include "vm/Shape-inl.h"
>  
> +#define NORMAL_JS_PROPS (JSPROP_ENUMERATE |                                   \
> +                         JSPROP_READONLY |                                    \
> +                         JSPROP_PERMANENT)

I'd prefer one-lining here.

@@ +202,5 @@
>      return &block->getFixedSlot(SLOT_DATATYPE).toObject();
>  }
>  
> +static inline js::PropertyName *
> +PropertyNameFromCString(JSContext *cx, const char *cstr)

It's mostly an anti-pattern to have common names, used as JS strings or property names or whatever, computed from C strings.  See comments by uses regarding how to get rid of this method.

@@ +206,5 @@
> +PropertyNameFromCString(JSContext *cx, const char *cstr)
> +{
> +    RootedAtom atom(cx, Atomize(cx, cstr, strlen(cstr)));
> +    if (!atom)
> +        return NULL;

nullptr now.

@@ +425,5 @@
> +template<ScalarTypeRepresentation::Type N>
> +static bool
> +NumericTypeToString(JSContext *cx, unsigned int argc, Value *vp)
> +{
> +    JS_STATIC_ASSERT(N < ScalarTypeRepresentation::TYPE_MAX);

Use static_assert(..., "bad numeric type") instead.  JS_STATIC_ASSERT will die once someone can go and convert all the remaining uses of it to the native C++11 functionality.

@@ +429,5 @@
> +    JS_STATIC_ASSERT(N < ScalarTypeRepresentation::TYPE_MAX);
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    JSString *s = JS_NewStringCopyZ(cx, ScalarTypeRepresentation::typeName(N));
> +    if (!s)
> +        return false;

Looks to me like you can use the same alternative mechanism I propose for DefineNumericType below, here, too -- double reason to introduce it.

@@ +430,5 @@
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    JSString *s = JS_NewStringCopyZ(cx, ScalarTypeRepresentation::typeName(N));
> +    if (!s)
> +        return false;
> +    args.rval().set(StringValue(s));

setString(s)

@@ +507,5 @@
>   *
> + * then
> + *
> + *   A.prototype.__proto__ === ArrayType.prototype.prototype
> + *   S.prototype.__proto__ === StructType.prototype.prototype

This seems slightly better explained in prose, and at greater length than this fairly-terse assertion of protoype-chain appearance.  Something like:

"""
For code like:

  var A = new TypedObject.ArrayType(uint8, 10);
  var S = new TypedObject.StructType({...});

As usual, the [[Prototype]] of A is TypedObject.ArrayType.prototype.  This permits adding methods to all ArrayType types, by setting TypedObject.ArrayType.prototype.methodName = function() { ... }.  The same holds for S with respect to TypedObject.StructType.

We may also want to add methods to *instances* of an ArrayType:

  var a = new A();
  var s = new S();

As usual, the [[Prototype]] of a is A.prototype.  What's A.prototype?  It's an empty object, and you can set A.prototype.methodName = function() { ... } to add a method to all A instances.  (And the same with respect to s and S.)

But what if you want to add a method to all ArrayType instances, not just all A instances?  (Or to all StructType instances.)  The [[Prototype]] of the A.prototype empty object is TypedObject.ArrayType.prototype.prototype (two .prototype levels!).  So just set TypedObject.ArrayType.prototype.prototype.methodName = function() { ... } to add a method to all ArrayType instances.  (And, again, same with respect to s and S.)

This function creates the A.prototype/S.prototype object.  It takes as an argument either the TypedObject.ArrayType or the TypedObject.StructType constructor function, then returns an empty object with the .prototype.prototype object as its [[Prototype]].
"""

@@ +514,5 @@
>   * returns a JSObject which can be set as A.prototype.
>   */
>  JSObject *
> +CreatePrototypeObjectForComplexTypeInstance(JSContext *cx,
> +                                            HandleObject typeObjectCtor)

CreateTypeInstancePrototype seems like it might be a nicer name here, that's at least as clear.

@@ +531,5 @@
> +    if (!JSObject::getProperty(cx, ctorPrototypeObj, ctorPrototypeObj,
> +                               cx->names().classPrototype,
> +                               &ctorPrototypePrototypeVal))
> +    {
> +        return NULL;

nullptr throughout, actually.

@@ +536,5 @@
> +    }
> +
> +    JS_ASSERT(ctorPrototypePrototypeVal.isObject()); // immutable binding
> +    RootedObject proto(cx, &ctorPrototypePrototypeVal.toObject());
> +    return NewObjectWithGivenProto(cx, &JSObject::class_, proto, NULL);

Use typeObjectCtor->global() for that last argument.

Glad to see JS_SetPrototype going away!

@@ +542,5 @@
> +
> +template<typename T>
> +static JSObject *
> +InitClass(JSContext *cx,
> +          Handle<GlobalObject*> global)

InitClass is a horrible name for this.  How about CreateTypedObjectConstructor?

@@ +547,3 @@
>  {
> +    RootedAtom className(cx, Atomize(cx, T::class_.name,
> +                                     strlen(T::class_.name)));

Add a T::name(cx) method that returns cx->names().ArrayType or cx->names().StructType as appropriate.

@@ +555,3 @@
>          return NULL;
>  
> +    // Create ctor.prototype, which inherits from Function.__proto__

Function.prototype

I guess we have to do all this approximately by hand because TypedObject.{Array,Struct}Type.prototype are neither StructTypes or ArrayTypes (as String.prototype is a String object), nor "blank"-inheriting prototypes (because CreateBlankProto asserts that the class isn't JSObject::class_).  Is that accurate?  This seems...well, not "fine" exactly, but "eh"...for now.  But a followup to think up something saner for this seems a good idea.  Maybe a new GlobalObject::createFunctionalPrototype method.  Or something.

@@ +558,5 @@
>  
> +    RootedObject proto(
> +        cx,
> +        NewObjectWithGivenProto(cx, &JSObject::class_, funcProto,
> +                                global, SingletonObject));

There's no good way to format this, but I'd rather see |RootedObject proto(cx);| and a separate assignment afterward, rather than this very-odd indenting.

@@ +570,5 @@
> +        return NULL;
> +    RootedObject protoProto(
> +        cx,
> +        NewObjectWithGivenProto(cx, &JSObject::class_, objProto,
> +                                global, SingletonObject));

Same separate-assignment nit.

@@ +578,5 @@
> +    RootedValue protoProtoValue(cx, ObjectValue(*protoProto));
> +    if (!JSObject::defineProperty(cx, proto, cx->names().classPrototype,
> +                                  protoProtoValue,
> +                                  NULL, NULL,
> +                                  NORMAL_JS_PROPS))

It is *not* "normal" for the .prototype on a built-in function to be enumerable.  Just spell out the attributes manually here, please.  Also, add a test that double-checks attributes here, please, with Object.getOwnPropertyDescriptor.  Maybe making this a static const GlobalObject::constructorAttributes, and sharing with LinkConstructorAndPrototype, would be a good idea here.

@@ -636,5 @@
>      return true;
>  }
>  
>  static bool
> -DataInstanceUpdate(JSContext *cx, unsigned argc, Value *vp)

I mentioned it on IRC, but putting these feature-removal bits in separate patches is better for everyone, usually.

@@ +807,5 @@
>   * TypedArray would be negative, it is clamped to zero.
>   * see: http://www.khronos.org/registry/typedarray/specs/latest/#7
>   *
>   */
> +/*static*/ bool

/* static */

@@ +862,5 @@
>  
>      RootedObject globalObj(cx, cx->compartment()->maybeGlobal());
>      JS_ASSERT(globalObj);
>      Rooted<GlobalObject*> global(cx, &globalObj->as<GlobalObject>());
> +    RootedObject arrayTypeGlobal(cx, global->getArrayType(cx));

Hmm.  If this can be infallible, which I think you're right that it can, we could probably use the infallible versions of these a good bit more.  (Nothing for you to do here, just me musing.)

@@ +940,5 @@
>      RootedValue typeByteLength(cx, NumberValue(typeRepr->size()));
>      if (!JSObject::defineProperty(cx, obj, cx->names().byteLength,
>                                    typeByteLength,
>                                    NULL, NULL,
> +                                  NORMAL_JS_PROPS))

Did you really mean to make this enumerable?

@@ +950,5 @@
>      RootedValue typeByteAlignment(cx, NumberValue(typeRepr->alignment()));
>      if (!JSObject::defineProperty(cx, obj, cx->names().byteAlignment,
>                                    typeByteAlignment,
>                                    NULL, NULL,
> +                                  NORMAL_JS_PROPS))

Or this.

@@ +960,5 @@
>      RootedValue variable(cx, JSVAL_FALSE);
>      if (!JSObject::defineProperty(cx, obj, cx->names().variable,
>                                    variable,
>                                    NULL, NULL,
> +                                  NORMAL_JS_PROPS))

Or this.

@@ +994,5 @@
> +    JS_ASSERT(prototypeVal.isObject()); // immutable binding
> +
> +    RootedObject obj(
> +        cx, NewObjectWithClassProto(cx, &ArrayType::class_,
> +                                    &prototypeVal.toObject(), NULL));

cx->global() for the last.

@@ +1002,5 @@
>  
>      RootedValue elementTypeVal(cx, ObjectValue(*elementType));
>      if (!JSObject::defineProperty(cx, obj, cx->names().elementType,
>                                    elementTypeVal, NULL, NULL,
> +                                  NORMAL_JS_PROPS))

This is supposed to change to be enumerable too?

@@ +1012,5 @@
>  
>      RootedValue lengthVal(cx, Int32Value(length));
>      if (!JSObject::defineProperty(cx, obj, cx->names().length,
>                                    lengthVal, NULL, NULL,
> +                                  NORMAL_JS_PROPS))

And this.

@@ +1098,4 @@
>  const Class StructType::class_ = {
>      "StructType",
>      JSCLASS_HAS_RESERVED_SLOTS(STRUCT_TYPE_RESERVED_SLOTS) |
> +    JSCLASS_HAS_PRIVATE, // used to store FieldList

Hmm.  Please file a followup to convert this into an extra reserved slot.  (Privates are on their way out the door right now, and in fact they're implemented *exactly* as reserved slots under the hood.)

@@ +1250,5 @@
>          return false;
>      RootedValue fieldNamesVecValue(cx, ObjectValue(*fieldNamesVec));
>      if (!JSObject::defineProperty(cx, structType, cx->names().fieldNames,
>                                    fieldNamesVecValue, NULL, NULL,
> +                                  NORMAL_JS_PROPS))

This is supposed to become enumerable as well?

@@ +1271,5 @@
>          // fieldOffsets[id] = offset
>          RootedValue offset(cx, NumberValue(field.offset));
>          if (!JSObject::defineGeneric(cx, fieldOffsets, fieldId,
>                                       offset, NULL, NULL,
> +                                     NORMAL_JS_PROPS))

The enumerability changes here seem pretty weird.  Nothing on, say, Uint8Array.prototype is enumerable.  Same for all the other builtins (I can't think of exceptions offhand, could be some, tho).  Why are these different?

@@ +1303,5 @@
>  {
> +    RootedValue prototypeVal(cx);
> +    if (!JSObject::getProperty(cx, metaTypeObject, metaTypeObject,
> +                               cx->names().classPrototype,
> +                               &prototypeVal))

I think in a followup we might want to consider storing these prototype objects in reserved global slots.  These are intrinsics, basically -- we shouldn't have to grovel through an object graph to find them.  That also makes the "immutable binding" bits self-evident.

@@ +1311,5 @@
> +    JS_ASSERT(prototypeVal.isObject()); // immutable binding
> +
> +    RootedObject obj(
> +        cx, NewObjectWithClassProto(cx, &StructType::class_,
> +                                    &prototypeVal.toObject(), NULL));

cx->global()

@@ +1450,5 @@
> +//
> +// Each of these is a function and hence their prototype is
> +// `Function.__proto__` (in terms of the JS Engine, they are not
> +// JSFunctions but rather instances of their own respective JSClasses
> +// which override the call and construct operations).

Shorter restatement of this paragraph:

These are all callable/constructable objects whose [[Prototype]]s are Function.prototype, but they are not functions in the JSAPI sense (as in having the class JSFunction::class_).

@@ +1460,5 @@
> +//        |
> +//    prototype
> +//        |
> +//        v
> +//       { } -----__proto__--> Function.__proto__

These diagrams are confusing because the meanings of the dashes and lines are never really clarified.  Do that, don't refer to __proto__ (you want [[Prototype]] to refer to the intrinsic concept).  Perhaps something like "Horizontal lines describe the [[Prototype]] chain relationship; vertical lines describe regular property relationships."

@@ +1480,5 @@
>  
> +template<ScalarTypeRepresentation::Type type>
> +static bool
> +DefineNumericClass(JSContext *cx, HandleObject global,
> +                   HandleObject module, const char *name);

Make the last argument HandlePropertyName, and pass cx->names().name_ in the callers.  const char* is bad!

@@ +1496,5 @@
> +     * Note that the various TypedObject types, like ArrayType, uint8,
> +     * etc, are not entered into jsprototypes.h nor do they
> +     * participate in the usual initialization mechanisms. Instead
> +     * they are just eagerly created here. This is largely because
> +     * they do not define global names.

I don't think we need to discuss this, really -- I'd honestly remove the entire comment.  (Or, at most, have a description of the TypedObject object, and the graph underneath it, as a file-level overview comment.)  Intl has the same strategy, it's not really that confusing, and the whole point of this method is exactly to define a global property.  What goes on on the object that's stored as that global property, really doesn't matter all that much.

@@ +1505,2 @@
>  
> +    RootedObject objProto(cx, JS_GetObjectPrototype(cx, global));

global().getOrCreateObjectPrototype()

@@ +1505,3 @@
>  
> +    RootedObject objProto(cx, JS_GetObjectPrototype(cx, global));
> +    JS_ASSERT(objProto);

We're in primordial guts, so it's possible Object.prototype hasn't been created yet, and therefore JS_GetObjectPrototype (and the underlying method) is fallible.  Null-check.

@@ +1513,4 @@
>  
> +    RootedValue moduleValue(cx, ObjectValue(*module));
> +    global->setReservedSlot(JSProto_TypedObject, moduleValue);
> +    if (!JSObject::defineProperty(cx, global, ClassName(JSProto_TypedObject, cx),

cx->names().TypedObject

@@ +1515,5 @@
> +    global->setReservedSlot(JSProto_TypedObject, moduleValue);
> +    if (!JSObject::defineProperty(cx, global, ClassName(JSProto_TypedObject, cx),
> +                                  moduleValue,
> +                                  NULL, NULL,
> +                                  0))

Please perform this definition, and then the setReservedSlot and the global->setArrayType, as the very last steps of this method, to preserve a semblance of correctness in case of failure.

@@ +1539,2 @@
>  
> +    RootedPropertyName arrayTypeName(cx, PropertyNameFromCString(cx, ArrayType::class_.name));

Add ArrayType to vm/CommonPropertyNames.h and use cx->names().ArrayType instead of this.

@@ +1544,5 @@
> +    RootedValue arrayTypeValue(cx, ObjectValue(*arrayType));
> +    if (!JSObject::defineProperty(cx, module, arrayTypeName,
> +                                  arrayTypeValue,
> +                                  NULL, NULL,
> +                                  NORMAL_JS_PROPS))

Enumerable, again?  And these are really supposed to be non-writable?  That's inconsistent with Intl, and basically with everything else in JS.

@@ +1557,3 @@
>          return NULL;
>  
> +    RootedPropertyName structTypeName(cx, PropertyNameFromCString(cx, StructType::class_.name));

cx->names().StructType the same way.

@@ +1564,5 @@
> +    if (!JSObject::defineProperty(cx, module, structTypeName,
> +                                  structTypeValue,
> +                                  NULL, NULL,
> +                                  NORMAL_JS_PROPS))
> +        return NULL;

Needs braces.

@@ +1576,5 @@
> +    /*
> +     * This function is entered into the jsprototypes.h table
> +     * as the initializer for `TypedObject`. As far as I can tell,
> +     * though, this code never runs because instead the `TypedObject`
> +     * type is initialized via the `standard_class_atoms` mechanism.

This is reachable if one does

  JSObject* obj;
  JS_GetClassObject(cx, global, JSProto_TypedObject, &obj);

But JS_GetClassObject is not a good API, and nobody should ever be calling it that way, so that's not a problem.  However, the message is wrong, as you can do the above even before TypedObject's been initialized.  So how about this message instead:

  MOZ_ASSUME_UNREACHABLE("shouldn't be initializing TypedObject via the JSProtoKey initializer mechanism")

Or something.  The whole initialization system is really kind of a mess, honestly.

@@ +1591,4 @@
>                     const char *name)
>  {
> +    RootedObject funcProto(cx, JS_GetFunctionPrototype(cx, global));
> +    JS_ASSERT(funcProto);

Again use getOrCreateFunctionPrototype, and null-check.

@@ +1598,2 @@
>      if (!numFun)
> +        return NULL;

All these NULLs are going to cause tinderbox compilation errors about converting NULL to bool.  Return false or true as appropriate here.

@@ +1619,2 @@
>  
> +    RootedPropertyName className(cx, PropertyNameFromCString(cx, name));

This one's trickier.  Please use a mechanism akin to what js::TypeName uses to handle this: the relevant type names, in the correct order, in CommonPropertyNames.h, then array-indexed into using the enum value.

::: js/src/builtin/TypedObject.h
@@ +92,5 @@
> +    static const JSFunctionSpec typedObjectMethods[];
> +
> +    // This is the function that gets called when the user
> +    // does `new ArrayType(elem)`. It produces an array type object.
> +    static bool construct(JSContext *cx, unsigned int argc, jsval *vp);

Generally we just do unsigned in these, rather than fully spell out unsigned int.  Also you want Value, not jsval (which is deprecated).  Feel free to convert all these to Value in a followup.

::: js/src/jit-test/tests/TypedObject/fuzz1.js
@@ +2,1 @@
>    quit();

Bleh, is this really what jit-tests do for this gunk?  :-(

@@ +8,1 @@
>  (eval())

Could you add some semicolons here so this is readable without the reader having to apply ASI?

  new TypedObject.StructType();
  eval();

::: js/src/jit-test/tests/TypedObject/fuzz10.js
@@ +4,3 @@
>    throw new Error("type too large");
>  
> +var AA = new TypedObject.ArrayType(new ArrayType(TypedObject.uint8, (2147483647)), 5);

Get rid of the extra parentheses here.  It's okay for a fuzzer to generate that stuff, because it's stupid.  But any test we add to our tree should be reasonably formatted so that extraneous garbage like this is gone.

::: js/src/jit-test/tests/TypedObject/fuzz11.js
@@ +4,3 @@
>    throw new Error("type too large");
>  
> +var A = new TypedObject.ArrayType(TypedObject.uint8, (2147483647));

Remove parens, again.

@@ +8,4 @@
>                          b: A,
>                          c: A,
>                          d: A,
>                          e: A});

Indent all these consistently, whatever consistently should be, I don't care.

::: js/src/jit-test/tests/TypedObject/fuzz2.js
@@ +2,3 @@
>    quit();
>  
> +new TypedObject.StructType([])

Add the semicolon, please.

::: js/src/tests/ecma_6/TypedObject/architecture.js
@@ +34,3 @@
>  
> +  assertEq(true, ArrayType instanceof Function);
> +  assertEq(true, ArrayType.prototype instanceof Function);

These are somewhat weird to assert, given that instanceof is fairly bizarre.  I'd rather see typeof === "function" assertions.

Also, assertEq takes (actual, expected), so these all should be reversed.

@@ +37,3 @@
>  
> +  assertEq(ArrayType.__proto__, Function.__proto__);
> +  assertEq(ArrayType.prototype.__proto__, Function.__proto__);

Use Object.getPrototypeOf for the left-hand sides, and use Function.prototype for the right-hand sides.

(You have actual, expected ordering correct for these, in contrast.)

@@ +40,3 @@
>  
> +  assertEq(true, StructType instanceof Function);
> +  assertEq(true, StructType.prototype instanceof Function);

(but not for these)

I'd do typeof === "function" checks again here.  instanceof is really not very often what you want, when you're able to do more exact checks.

@@ +43,3 @@
>  
> +  assertEq(StructType.__proto__, Function.__proto__);
> +  assertEq(StructType.prototype.__proto__, Function.__proto__);

Object.getPrototypeOf, Function.prototype

::: js/src/tests/ecma_6/TypedObject/memory.js
@@ +63,5 @@
>      spin();
>      assertEq(middleBand['r'] == 0 && middleBand['g'] == 0 && middleBand['b'] == 0, true);
> +    middleBand.r = 255;
> +    middleBand.g = 207;
> +    middleBand.b = 142;

Not sure why this change is being made, in this patch, but I'm guessing it's at least not wrong.  :-)

::: js/src/tests/ecma_6/TypedObject/structtypeprototype.js
@@ +20,5 @@
>    var Fade1 = new StructType({from: RgbColor1, to: RgbColor1});
>    var Fade2 = new StructType({from: RgbColor2, to: RgbColor2});
>  
> +  // Available on all struct types
> +  StructType.prototype.prototype.sub = function(c) {

Add a parenthetical (but only used/meaningful for RgbColor1 and RgbColor2 instances), I think.

::: js/src/vm/GlobalObject.cpp
@@ +631,5 @@
>  }
> +
> +
> +void
> +GlobalObject::setArrayType(JSObject *obj) {

{ goes on new line.

@@ +632,5 @@
> +
> +
> +void
> +GlobalObject::setArrayType(JSObject *obj) {
> +    initSlot(ARRAY_TYPE, ObjectValue(*obj));

initReservedSlot is better.  (And why isn't this method in GlobalObject.h?)

::: js/src/vm/GlobalObject.h
@@ +364,5 @@
>      JSObject *getIteratorPrototype() {
>          return &getPrototype(JSProto_Iterator).toObject();
>      }
>  
> +    JSObject *getArrayType(JSContext *cx) {

Get rid of the JSContext* argument.  If this is just consulting a slot, there's no reason for it to take a cx, and hint at its being fallible.  (Along those lines, "get" prefixes indicate fallibility usually.  So this should probably be arrayType, or maybe arrayTypeConstructor.)

@@ +365,5 @@
>          return &getPrototype(JSProto_Iterator).toObject();
>      }
>  
> +    JSObject *getArrayType(JSContext *cx) {
> +        Value v = getSlotRef(ARRAY_TYPE);

Doesn't really matter, but if you're going to use the ref version, might as well use a ref here too.

@@ +371,5 @@
> +        // This method is only called from within TypedObject.cpp,
> +        // and in order for the relevant code to be executing,
> +        // TypedObject must have been initialized, which should
> +        // invoke setArrayType() below.
> +        JS_ASSERT(v.isObject());

MOZ_ASSERT(v.isObject(),
           "GlobalObject::arrayType must only be called from "
           "TypedObject code that can assume TypedObject has "
           "been initialized");

That also lets you get rid of the comment.

Comment 10

[Niko Matsakis [:nmatsakis]]

Attachment: Bug914220.diff

Rebased, carrying over r+ from waldo

Comment 11

[Niko Matsakis [:nmatsakis]]

Attachment: Bug914220-Waldo.diff

Additional changes to address Waldo's comments (and in some cases to get the code to compile after rebasing).

Waldo -- to be honest, the spec is quite vague about whether or not properties should be enumerable. I (incorrectly, it seems) assumed that this would be the normal state of affairs. Therefore, I have made the various properties non-enumerable. I left them as immutable for now, including the properties of TypedObject. My reasoning is that the "TypedObject" pseudo-module is not a permanent thing, and by the time this code is made public, will be replaced by an ES6 module, which should use static binding that is effectively immutable.

There are some cases (notably the `prototype` property of type objects) that I believe should become mutable, but I will leave that for another patch.

Comment 12

[Niko Matsakis [:nmatsakis]]

Try run looks green, though there is one Android test coming up orange. I suspect this as a pre-existing issue and have tagged it appropriately:

https://tbpl.mozilla.org/?tree=Try&rev=06990c03c308

Comment 13

[Niko Matsakis [:nmatsakis]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/89600e659123

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/89600e659123

Comment 15

[Jeff Walden [:Waldo]]

Comment on attachment 817753 [details] [diff] [review]
Bug914220-Waldo.diff

Review of attachment 817753 [details] [diff] [review]:
-----------------------------------------------------------------

Broadly good but missed a few things, plus I saw a couple new (?) issues.

::: js/src/builtin/TypedObject.cpp
@@ +504,5 @@
>  
>      JS_ASSERT(ctorPrototypePrototypeVal.isObject()); // immutable binding
>      RootedObject proto(cx, &ctorPrototypePrototypeVal.toObject());
> +    return NewObjectWithGivenProto(cx, &JSObject::class_, proto,
> +                                   &typeObjectCtor->global());

Hm, JSObject::global() doesn't return a handle.  Surprising.  Probably should fix that sometime.

@@ +515,3 @@
>  {
>      RootedAtom className(cx, Atomize(cx, T::class_.name,
>                                       strlen(T::class_.name)));

This needs static HandlePropertyName ArrayType::className(JSContext* cx) { return cx->names().ArrayType; } (and similar for StructType), then T::className() should be used.

@@ +521,5 @@
>      RootedObject funcProto(cx, global->getOrCreateFunctionPrototype(cx));
>      if (!funcProto)
>          return nullptr;
>  
> +    // Create ctor.prototype, which inherits from Function.prototype

Period at end of complete sentences.

@@ +526,4 @@
>  
>      RootedObject proto(
> +        cx, NewObjectWithGivenProto(cx, &JSObject::class_, funcProto,
> +                                    global, SingletonObject));

RootedObject proto(cx);
proto = NewObjectWithGivenProto(...);

and same style for the other instances like this.  Or

    RootedObject proto(cx, NewObjectWithGivenProto(cx, &JSObject::class_, funcProto, global,
                                                   SingletonObject));

Offhand I don't think I've ever seen ( at the end of a line in SpiderMonkey code.

@@ +532,2 @@
>  
>      // Create ctor.prototype.prototype, which inherits from Object.__proto__

Object.prototype, not Object.__proto__ (that's a different value!).

@@ +550,5 @@
>  
>      // Create ctor itself
>  
> +    RootedFunction ctor(
> +        cx, global->createConstructor(cx, T::construct, className, 2));

Hm, if we don't need className til here, move computation of that down to here.

@@ +1419,2 @@
>  //
> +//   StructType --[[P]]--> Function.[[P]]

Function.prototype (the [[Prototype]] of the Function object happens to also be Function.prototype, but really that prototypal-in-the-[[]]-sense meaning is not what you want)

@@ +1421,5 @@
> +//        |
> +//    prototype
> +//        |
> +//        v
> +//       { } -----[[P]]--> Function.[[P]]

Again, Function.prototype.

@@ +1426,5 @@
>  //        |
>  //    prototype
>  //        |
>  //        v
> +//       { } -----[[P]]--> Object.[[P]]

Object.prototype; Object.[[P]] is *not* what you want, as that would be Function.prototype (but data instances aren't callable-ish things).

@@ +1445,5 @@
> +//    object -[[P]]--> MyStruct.prototype
> +//
> +// This structure permits users to install methods for all struct
> +// types (by modifying StructType.prototype); for all struct instances
> +// (by modifying StructType.prototype.prototype); or for all instances

Pedantically, you want commas here, as you don't have any extra punctuation in the various clauses.

@@ +1474,3 @@
>  
>      RootedObject module(cx, NewObjectWithClassProto(cx, &JSObject::class_,
>                                                      objProto, global));

NewObjectWithGivenProto and SingletonObject.

@@ +1498,4 @@
>                                    arrayTypeValue,
> +                                  nullptr, nullptr,
> +                                  JSPROP_READONLY | JSPROP_PERMANENT))
> +        return nullptr;

Needs {}, both aligned with 'i' in 'if'.

@@ +1511,4 @@
>                                    structTypeValue,
> +                                  nullptr, nullptr,
> +                                  JSPROP_READONLY | JSPROP_PERMANENT))
> +        return nullptr;

{}

@@ +1517,5 @@
> +    if (!JSObject::defineProperty(cx, global, cx->names().TypedObject,
> +                                  moduleValue,
> +                                  nullptr, nullptr,
> +                                  0))
> +        return nullptr;

{}

@@ +1551,3 @@
>  
>      RootedObject numFun(cx, NewObjectWithClassProto(cx, &NumericTypeClasses[type],
>                                                      funcProto, global));

NewObjectWithGivenProto, and a SingletonObject

@@ +1576,4 @@
>      RootedValue numFunValue(cx, ObjectValue(*numFun));
>      if (!JSObject::defineProperty(cx, module, className,
> +                                  numFunValue, nullptr, nullptr, 0))
> +        return false;

Just return this value, get rid of the extra return true/false.

::: js/src/jit-test/tests/TypedObject/fuzz10.js
@@ +2,5 @@
>  
>  if (!this.hasOwnProperty("TypedObject"))
>    throw new Error("type too large");
>  
> +var AA = new TypedObject.ArrayType(new ArrayType(TypedObject.uint8, 2147483647), 5);

Aren't you missing a TypedObject. in the middle there?  Any way to finesse this test so that this error wouldn't have been silently passing when it shouldn't?

::: js/src/tests/ecma_6/TypedObject/architecture.js
@@ +37,3 @@
>  
>    assertEq(ArrayType.__proto__, Function.__proto__);
>    assertEq(ArrayType.prototype.__proto__, Function.__proto__);

assertEq(Object.getPrototypeOf(ArrayType), Function.prototype);
assertEq(Object.getPrototypeOf(ArrayType.prototype), Function.prototype);

@@ +43,3 @@
>  
> +  assertEq(Object.getPrototypeOf(StructType),
> +           Object.getPrototypeOf(Function));

Object.getPrototypeOf(Function) is Function.prototype -- just use that instead, to avoid needless indirection:

assertEq(Object.getPrototypeOf(StructType), Function.prototype);

@@ +43,5 @@
>  
> +  assertEq(Object.getPrototypeOf(StructType),
> +           Object.getPrototypeOf(Function));
> +  assertEq(Object.getPrototypeOf(StructType.prototype),
> +           Object.getPrototypeOf(Function));

Also:

assertEq(Object.getPrototypeOf(StructType.prototype), Function.prototype);