1112778 - Retire namespace baseops

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Working in NativeObject.cpp, there are constant reminders of what a mess everything is. Let's make some superficial changes around SM's meta-object protocol. Here's the direction I want to go:

1.  Call these "internal methods" from now on.

    The 14 internal methods defined in ES6 are "standard internal methods".
    We'll also continue to have a bunch of nonstandard internal methods,
    like `hasOwn`.

2.  Continue to align with ES6.

    This means, for example, dropping the `strict` argument on SetProperty
    in favor of some kind of outparam indicating success.

    I don't want to be dogmatic about forcing the ES6 names. Some are too
    short ([[Get]]) and some are hideous ([[GetPrototypeOf]]). Instead,
    pick a friendly name for each standard internal method:

        [[GetPrototypeOf]]: GetPrototype
        [[SetPrototypeOf]]: SetPrototype
        [[IsExtensible]]: IsExtensible
        [[PreventExtensions]]: PreventExtensions
        [[GetOwnProperty]]: GetOwnPropertyDescriptor
        [[DefineOwnProperty]]: DefineOwnProperty
        [[HasProperty]]: HasProperty
        [[Get]]: GetProperty
        [[Set]]: SetProperty
        [[Delete]]: DeleteProperty
        [[Enumerate]]: Enumerate
        [[OwnPropertyKeys]]: GetOwnPropertyKeys
        [[Call]]: Call
        [[Construct]]: Construct

    and let's start using these pretty much everywhere.

3.  Rework namespaces.

        JS_GetProperty              -> JS::GetProperty
        JSObject::getProperty       -> js::GetProperty
        js::baseops::GetProperty    -> js::NativeGetProperty
        js::Proxy::GetProperty      -> js::ProxyGetProperty
        js::BaseProxyHandler::getProperty  (this one is fine)

    I find that the js::baseops namespace and js::Proxy class are kind of
    nice-in-theory, ugly-in-practice. Extra explicit namespace qualification
    ends up being needed in more places than you'd think. It doesn't pay off.

    And it hurts: having multiple functions with identical names in different
    namespaces is guaranteed to trigger usability gotchas in tools. gdb,
    MXR, DXR, idutils... "NativeGetProperty" is a good blue-collar name.
    Let's use that.

4.  Delete some code.  (You'll see.)

This list is just to show you where I'm going. In this bug, I'm only doing some of the naming/namespacing stuff from items 2 and 3.

Comment 1

[Jason Orendorff [:jorendorff]]

>        JS_GetProperty              -> JS::GetProperty
>        JSObject::getProperty       -> js::GetProperty

Oh wait. This is not going to work out. The compiler gets confused if you `using namespace` both namespaces.

Well, maybe the best thing is actually to leave the JS_ prefix in place.

Comment 2

[Jeff Walden [:Waldo]]

Big paint job, but v8's v8::internal idea grows more and more appealing, versus sibling namespaces...

Comment 3

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Big paint job, but v8's v8::internal idea grows more and more appealing,
> versus sibling namespaces...

Can you elaborate on that?

Comment 4

[Jeff Walden [:Waldo]]

v8 uses |namespace v8 {}| for all their APIs.  Then they nest all implementation details, internals, &c. in |namespace v8::internal {}| while explicitly disclaiming anything in it as public API.  This means that in any internal method, they don't have to qualify any access to a public API, because name lookup will find it in an enclosing namespace.  And in the case where a public API and an internal API have the same name, in the ordinary case where a function is being called with different arguments in each case, the distinct overloads will both naturally be found as requested, without need for disambiguation.  The only exception I'm aware of is those rare cases where an ambiguous name is used, in a context where its type can't be inferred.

This scheme eliminates the need for using-statements, because any internal definition is already within the public API's namespace.

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: part 1 - Remove namespace baseops. Rename js::baseops::DefineGeneric -> js::NativeDefineProperty and so on

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: part 2 - Rename js::DefaultValue -> js::OrdinaryToPrimitive

Since JS and js are sibling namespaces, and in some scopes both are opened (for example,
due to argument-dependent name lookup; or because someone carelessly did `using namespace`
for both) it's a bad idea to have both JS::OrdinaryToPrimitive and js::OrdinaryToPrimitive.
So I have grudgingly changed the public API to JS_OrdinaryToPrimitive.

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: part 3 - Rename JSObject::preventExtensions -> js::PreventExtensions and a few others, and move them to jsobj.cpp. Uninline several functions that have no business being inlined

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: part 4 - Rename a static function HasProperty -> GetPropertyIfPresent, to get it out of the way for js::HasProperty, which is coming in a subsequent patch. Remove the js::GetOwnPropertyDescriptor signature that makes an object: it was only used in one pla

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: part 5 - Rename JSObject::defineGeneric etc. -> js::DefineProperty, js::DefineOwnProperty -> js::StandardDefineProperty (for the moment--the plan is to unite these soon, both the name and the implementation)

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: part 6 - Rename JSObject::lookupGeneric, etc. -> js::LookupProperty and JSObject::hasProperty -> js::HasProperty

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: part 7 - Rename JSObject::{get,set}Generic etc. -> js::{Get,Set}Property and add a few comments

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: part 8 - Rename JSObject::deleteGeneric -> js::DeleteProperty

Comment 13

[Jason Orendorff [:jorendorff]]

Attachment: part 9 - Rename JSObject::freeze -> FreezeObject and others

Some of the bits of implementation added for ES5 have been given names in ES6, so
    JSObject::sealOrFreeze -> js::SetIntegrityLevel
    JSObject::isSealedOrFrozen -> js::TestIntegrityLevel
    JSObject::ImmutabilityType -> js::IntegrityLevel

Comment 14

[Jason Orendorff [:jorendorff]]

Attachment: part 10 - Rename the remaining nonstandard internal methods, such as JSObject::getGenericAttributes -> js::GetPropertyAttributes

Comment 15

[Jason Orendorff [:jorendorff]]

</rant>

Comment 16

[Jeff Walden [:Waldo]]

Comment on attachment 8539272 [details] [diff] [review]
part 1 - Remove namespace baseops. Rename js::baseops::DefineGeneric -> js::NativeDefineProperty and so on

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

::: js/src/jit/VMFunctions.cpp
@@ +507,5 @@
>              cx, obj.as<NativeObject>(), obj.as<NativeObject>(), id,
>              (op == JSOP_SETNAME || op == JSOP_STRICTSETNAME ||
>               op == JSOP_SETGNAME || op == JSOP_STRICTSETGNAME)
> +            ? Unqualified
> +            : Qualified,

These unqualified (heh) names are a little generic.  I'd kind of prefer if they were QualifiedSet and UnqualifiedSet.  Could do in a separate patch, no need for me to see it.

::: js/src/jsproxy.h
@@ +76,4 @@
>   *
>   * 2.  Certain non-native objects have internal methods that are implemented as
>   *     magical js::ObjectOps hooks. We're trying to get rid of these.
>   * 

Maybe remove trailing WS while you're in the area?

::: js/src/vm/NativeObject.h
@@ +1411,5 @@
>  {
>      js::LookupElementOp op = obj->getOps()->lookupElement;
>      if (op)
>          return op(cx, obj, index, objp, propp);
> +    return NativeLookupElement(cx, obj.as<js::NativeObject>(), index, objp, propp);

Lack of js:: qualification here (and a bunch of other places below) makes me think this is only working because of unified builds.  Do a --disable-unified-compilation build before landing this.

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 8539276 [details] [diff] [review]
part 2 - Rename js::DefaultValue -> js::OrdinaryToPrimitive

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

So this is fine enough, but I don't understand why JS_OrdinaryToPrimitive has to be a thing.  If js::OrdinaryToPrimitive and JS_OrdinaryToPrimitive have exactly the same behavior, and they do, why have two methods?  Why not have just JS::OrdinaryToPrimitive for everything?  The call overhead is irrelevant for any use case I can think of.  I don't think we should have JS_OrdinaryToPrimitive as a thing here.

Comment 18

[Jeff Walden [:Waldo]]

Comment on attachment 8539308 [details] [diff] [review]
part 3 - Rename JSObject::preventExtensions -> js::PreventExtensions and a few others, and move them to jsobj.cpp. Uninline several functions that have no business being inlined

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

::: js/src/jsarray.cpp
@@ +452,5 @@
>          if (obj->is<ArrayObject>()) {
>              vp.setNumber(obj->as<ArrayObject>().length());
>              return true;
>          }
> +        if (!GetPrototype(cx, obj, &obj))

This sort of reuse of a root as an in-param and an outparam gives me the willies.  Could you add another root and store into that, please?

::: js/src/jsobj.cpp
@@ +3419,5 @@
> +bool
> +js::PreventExtensions(JSContext *cx, HandleObject obj, bool *succeeded)
> +{
> +    if (obj->is<ProxyObject>())
> +        return js::Proxy::preventExtensions(cx, obj, succeeded);

Could get rid of the js:: prefix here and below, as you're inside the js namespace.

@@ +3682,5 @@
>  js::IsDelegateOfObject(JSContext *cx, HandleObject protoObj, JSObject* obj, bool *result)
>  {
>      RootedObject obj2(cx, obj);
>      for (;;) {
> +        if (!GetPrototype(cx, obj2, &obj2))

Again a bit leery of same location passed as a value and as an outparam at once.

Comment 19

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #16)
> ::: js/src/vm/NativeObject.h
> @@ +1411,5 @@
> >  {
> >      js::LookupElementOp op = obj->getOps()->lookupElement;
> >      if (op)
> >          return op(cx, obj, index, objp, propp);
> > +    return NativeLookupElement(cx, obj.as<js::NativeObject>(), index, objp, propp);
> 
> Lack of js:: qualification here (and a bunch of other places below) makes me
> think this is only working because of unified builds.  Do a
> --disable-unified-compilation build before landing this.

That particular one is OK because of argument-dependent lookup, but good idea.

Comment 20

[Jeff Walden [:Waldo]]

Comment on attachment 8539330 [details] [diff] [review]
part 5 - Rename JSObject::defineGeneric etc. -> js::DefineProperty, js::DefineOwnProperty -> js::StandardDefineProperty (for the moment--the plan is to unite these soon, both the name and the implementation)

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

::: js/src/builtin/TypedObject.cpp
@@ +827,1 @@
>              return nullptr;

{} while here?

@@ +866,1 @@
>              return nullptr;

{} while here?

::: js/src/jsobj.cpp
@@ +930,3 @@
>          return false;
>      *bp = !!rval;
>      return true;

This can all just be

    return StandardDefineProperty(cx, obj, id, desc, true, bp);

now that we're three years (!!!) past JSBool.

Comment 21

[Jeff Walden [:Waldo]]

Comment on attachment 8539425 [details] [diff] [review]
part 10 - Rename the remaining nonstandard internal methods, such as JSObject::getGenericAttributes -> js::GetPropertyAttributes

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

If you're wondering about Part 9, I didn't have a new-enough ES6 draft on hand to review it on the drive to Chicago.  Monday, I think.

::: js/src/jsobj.h
@@ +957,5 @@
>                            typename MaybeRooted<JSObject*, allowGC>::MutableHandleType objp,
>                            typename MaybeRooted<Shape*, allowGC>::MutableHandleType propp);
>  
> +/*
> + * An easier-to-use version of LookupProperty that returns only the property attributes.

Add an "Equally deprecated." comment here.  :-)

Comment 22

[Jeff Walden [:Waldo]]

Comment on attachment 8539358 [details] [diff] [review]
part 9 - Rename JSObject::freeze -> FreezeObject and others

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

::: js/src/jsobj.cpp
@@ +1030,4 @@
>  {
>      assertSameCompartment(cx, obj);
> +
> +    // ES6 rev 29 (6 Dec 2014) 7.3.13.

Seems like this should move above the method signature.

@@ +1035,4 @@
>      bool succeeded;
>      if (!PreventExtensions(cx, obj, &succeeded))
>          return false;
>      if (!succeeded) {

Could you rename s/succeeded/status/g if that's what the spec's going to use?

@@ +1134,2 @@
>  {
> +    // ES6 rev 29 (6 Dec 2014) 7.3.14.

Again comment above method.

@@ +1137,4 @@
>      bool extensible;
>      if (!IsExtensible(cx, obj, &extensible))
>          return false;
>      if (extensible) {

Looks like s/extensible/status/g as above.

@@ +1147,5 @@
>              // Typed arrays are always sealed.
>              *resultp = true;
>          } else {
>              // Typed arrays cannot be frozen, but an empty typed array is
>              // trivially frozen.

Neither of these is true any more, what with the ability to add properties to typed arrays now.  Could you file a bug on this?

@@ +1159,5 @@
>      if (!GetPropertyKeys(cx, obj, JSITER_HIDDEN | JSITER_OWNONLY | JSITER_SYMBOLS, &props))
>          return false;
>  
> +    // Steps 9-11. The spec does not permit stopping as soon as we find out
> +    // the answer is false, so we are cheating a little here.

Add an "(observably)" in here, and perhaps cite a bug number.  This is observable with a proxy with behavior just so.

::: js/src/jsobj.h
@@ +1265,5 @@
>  
>  extern bool
>  NativeUnwatch(JSContext *cx, JS::HandleObject obj, JS::HandleId id);
>  
> +enum IntegrityLevel { SEALED, FROZEN };

Rather than unadorned names that potentially will conflict with macros defined elsewhere, let's use the new swanky supported-everywhere:

enum class IntegrityLevel
{
  Sealed,
  Frozen
};

and then IntegrityLevel::Sealed and such at use sites.

@@ +1269,5 @@
> +enum IntegrityLevel { SEALED, FROZEN };
> +
> +/*
> + * ES6 rev 29 (6 Dec 2014) 7.3.13. Mark obj as non-extensible, and adjust each
> + * if obj's own properties' attributes appropriately: each property becomes

of

@@ +1270,5 @@
> +
> +/*
> + * ES6 rev 29 (6 Dec 2014) 7.3.13. Mark obj as non-extensible, and adjust each
> + * if obj's own properties' attributes appropriately: each property becomes
> + * non-configurable, and if level == FROZEN, data properties become read-only

non-writable, for consistency in all places with spec language

@@ +1279,5 @@
> +
> +inline bool
> +FreezeObject(JSContext *cx, HandleObject obj)
> +{
> +    return SetIntegrityLevel(cx, obj, FROZEN);

I'd rather see callers just do SetIntegrityLevel themselves, versus calling a micro-helper that doesn't exist in the spec.  Anyone writing SpiderMonkey code should be familiarizing themselves with the spec language, and the spec language is SetIntegrityLevel/TestIntegrityLevel, nothing more.

@@ +1287,5 @@
> + * ES6 rev 29 (6 Dec 2014) 7.3.14. Code shared by Object.isSealed and
> + * Object.isFrozen.
> + */
> +extern bool
> +TestIntegrityLevel(JSContext *cx, HandleObject obj, IntegrityLevel level, bool *resultp);

Not a fan of "p" prefixes or suffixes this way, honestly.  I'd rather see just "result" here.

::: js/src/vm/Debugger.cpp
@@ +6416,5 @@
>      ac.emplace(cx, obj);
>      ErrorCopier ec(ac);
>      bool ok;
>      if (op == OpSeal) {
> +        ok = SetIntegrityLevel(cx, obj, SEALED);

I'm not sure the centralization of all these methods in one "helper" method really helps.  It'd be nice in a followup to separate them -- or at least to separate extensibility out and have a DebuggerObject_setIntegrityLevel function to handle the seal/freeze cases.  Followup bug?

(Same idea applies to the testing function later in this file, too.)

Comment 23

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #22)
> >              // Typed arrays are always sealed.
> >[...]
> >              // [...] an empty typed array is
> >              // trivially frozen.
> 
> Neither of these is true any more, what with the ability to add properties
> to typed arrays now.  Could you file a bug on this?

Bug 1120503. I modified both comments to cite the bug.

> > +    // Steps 9-11. The spec does not permit stopping as soon as we find out
> > +    // the answer is false, so we are cheating a little here.
> 
> Add an "(observably)" in here, and perhaps cite a bug number.

Bug 1120512. The comment now cites the bug.

> @@ +1279,5 @@
> > +
> > +inline bool
> > +FreezeObject(JSContext *cx, HandleObject obj)
> > +{
> > +    return SetIntegrityLevel(cx, obj, FROZEN);
> 
> I'd rather see callers just do SetIntegrityLevel themselves, versus calling
> a micro-helper that doesn't exist in the spec.

I see --- but I want to keep this. Object.freeze is just such a micro-helper, and it'll be familiar to far more readers than SetIntegrityLevel, even among people very familiar with the spec.

FreezeObject has about 10 call sites, and every single one of them is in high-level code where "FreezeObject(cx, obj)" is just what you mean and "SetIntegrityLevel(cx, obj, IntegrityLevel::Freeze)" is just bureaucratese. If you count engine-internal JS_FreezeObject calls, it's ~50 call sites.

Reflecting a bit: micro-helpers are not all bad. After all TC39 spec'd Object.seal and Object.freeze rather than a single Object.setIntegrityLevel API for *some* reason. I think because "seal" and "freeze" are so different practically. In a given situation you want one or the other. So the flexibility value in having a single API that can do either is practically nil; and the readability value of the micro-helpers is nice to have.


(re: DebuggerObject_sealHelper)
> I'm not sure the centralization of all these methods in one "helper" method
> really helps.  It'd be nice in a followup to separate them -- or at least to
> separate extensibility out and have a DebuggerObject_setIntegrityLevel
> function to handle the seal/freeze cases.  Followup bug?

I think this is a grass-is-greener thing: you'll find the code duplication after is just as annoying as the if-else chain is now. But feel free to file/patch if you think it's worthwhile.

I took all the other comments. Thanks for the detailed review.

Comment 24

[Jason Orendorff [:jorendorff]]

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

Comment 25

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=93bfa5559fb9

Comment 26

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c51dcd10c79e
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec05328eb325
https://hg.mozilla.org/integration/mozilla-inbound/rev/42e833ab7dea
https://hg.mozilla.org/integration/mozilla-inbound/rev/d172d554c732
https://hg.mozilla.org/integration/mozilla-inbound/rev/2575c07d5b5b
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2d2f1d20039
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0bcff4b675e
https://hg.mozilla.org/integration/mozilla-inbound/rev/373c4f89625e
https://hg.mozilla.org/integration/mozilla-inbound/rev/8330481355e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1aa2f649c70

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c51dcd10c79e
https://hg.mozilla.org/mozilla-central/rev/ec05328eb325
https://hg.mozilla.org/mozilla-central/rev/42e833ab7dea
https://hg.mozilla.org/mozilla-central/rev/d172d554c732
https://hg.mozilla.org/mozilla-central/rev/2575c07d5b5b
https://hg.mozilla.org/mozilla-central/rev/e2d2f1d20039
https://hg.mozilla.org/mozilla-central/rev/a0bcff4b675e
https://hg.mozilla.org/mozilla-central/rev/373c4f89625e
https://hg.mozilla.org/mozilla-central/rev/8330481355e1
https://hg.mozilla.org/mozilla-central/rev/f1aa2f649c70