Closed Bug 1112778 Opened 5 years ago Closed 5 years ago

Retire namespace baseops

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: jorendorff, Assigned: jorendorff)

Details

Attachments

(10 files)

79.50 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
13.20 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
57.69 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
7.45 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
96.72 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
30.68 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
115.28 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
17.17 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
22.74 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
42.40 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
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.
>        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.
Big paint job, but v8's v8::internal idea grows more and more appealing, versus sibling namespaces...
(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?
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.
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
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.
Attachment #8539276 - Flags: review?(jwalden+bmo)
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
Attachment #8539358 - Flags: review?(jwalden+bmo)
</rant>
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.
Attachment #8539272 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #8539276 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #8539308 - Flags: review?(jwalden+bmo) → review+
Attachment #8539319 - Flags: review?(jwalden+bmo) → review+
(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 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.
Attachment #8539330 - Flags: review?(jwalden+bmo) → review+
Attachment #8539343 - Flags: review?(jwalden+bmo) → review+
Attachment #8539348 - Flags: review?(jwalden+bmo) → review+
Attachment #8539352 - Flags: review?(jwalden+bmo) → review+
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.  :-)
Attachment #8539425 - Flags: review?(jwalden+bmo) → review+
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.)
Attachment #8539358 - Flags: review?(jwalden+bmo) → review+
(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.
You need to log in before you can comment on or make changes to this bug.