Closed Bug 1234702 Opened 4 years ago Closed 4 years ago

Default derived constructor doesn't use standard rest/spread semantics

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: anba, Assigned: efaust)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 2 obsolete files)

Test case:
---
Array.prototype[Symbol.iterator] = function*() { yield 1; yield 2; };

class Base { constructor(a, b) { print(a, b); } }
class Derived extends Base {}

new Derived();
---

Expected: Prints "1 2"
Actual: Prints "undefined undefined"

It works as expected when the default derived constructor is written manually, that means:
---
class OtherDerived extends Base { constructor(...args) { super(...args); } }
new OtherDerived ();
---

prints "1 2" to the console.


Spec ES2015:
- 14.5.14 Runtime Semantics: ClassDefinitionEvaluation, step 10.a.i for `constructor(...args){super(...args)}`
- 13.3.3.6 Runtime Semantics: IteratorBindingInitialization, "BindingRestElement : ... BindingIdentifier", step 3 for `constructor(...args)`
- 12.3.6.1 Runtime Semantics: ArgumentListEvaluation, "ArgumentList : ... AssignmentExpression", step 4 for `super(...args)`
OK, so this has come a long way in a local queue, but I still have a few questions.

When you extend a class, you create two objects
1) You make the [[Prototype]] of the created function (the class constructor) be the [[Prototype]] of the object from which you inherited

2) You make the [[Prototype]] of the created .prototype object be the [[Prototype]] of the .prototype object of the object from which you inherited.

I'm trying to understand what the builtin prototype landscape looks like, and whether it's been reduced to the point that ever wanting to legitimately use classes in self-hosted code is unfeasible.

My current problem is really stupid: the .prototype property gets installed on created classes as totally locked down, which will make it hard to reset when we transplant the class constructor into the new class.

One conservative solution is to define those properties as configurable in self-hosted code, which will allow us to use class syntax in self-hosting in the future, but may break spec-compliance if they choose to write things in terms of classes, as we will struggle to tell this one apart from others.

Another, slightly more attractive (to me at the moment, at any rate) idea is to just totally lock down class syntax in self-hosted code: only allow the one method (constructor()) and just emit the single function (basically just use the class syntax to force the parser to set all the bits we want for that function). This will fix my problem, since we'll never initialize any properties, but also means that classes will be pretty useless in the future, if people want to use them. Unless the builtin climate is too degenerate to support them, there's no good reason that classes "secretly call content", unless you do something obvious like derive from a content-provided function.

Till, do you have any thoughts on this matter? In particular, can you advise about whether you think classes can be viable in self-hosted code?
Flags: needinfo?(till)
(In reply to Eric Faust [:efaust] from comment #1)
> When you extend a class, you create two objects
> 1) You make the [[Prototype]] of the created function (the class
> constructor) be the [[Prototype]] of the object from which you inherited
> 
> 2) You make the [[Prototype]] of the created .prototype object be the
> [[Prototype]] of the .prototype object of the object from which you
> inherited.

I'm assuming you have "the [[Prototype]] of the" too much in both of these, right?

> I'm trying to understand what the builtin prototype landscape looks like,
> and whether it's been reduced to the point that ever wanting to legitimately
> use classes in self-hosted code is unfeasible.

I hope not!

> My current problem is really stupid: the .prototype property gets installed
> on created classes as totally locked down, which will make it hard to reset
> when we transplant the class constructor into the new class.

Why do we need to do this transplantation in the first place? I probably don't fully understand what you mean here, but wouldn't we just clone the entire class definition (potentially including any super classes recursively) when something causes the class to be cloned? The cloning being an internal operation, I don't see why we should be restricted by things such as property flags.

(Note: we don't currently have support for fully self-hosted builtins, with no JSClass definition at all. I want to add that, by providing a "register this as a builtin to be installed on content globals" intrinsic. My current thinking is that that'd work with the resolve hook we have on globals anyway, so cloning would be lazy. Don't know if that is relevant here, but maybe it is.)

> One conservative solution is to define those properties as configurable in
> self-hosted code, which will allow us to use class syntax in self-hosting in
> the future, but may break spec-compliance if they choose to write things in
> terms of classes, as we will struggle to tell this one apart from others.

We shouldn't do that, no.

> Another, slightly more attractive (to me at the moment, at any rate) idea is
> to just totally lock down class syntax in self-hosted code: only allow the
> one method (constructor()) and just emit the single function (basically just
> use the class syntax to force the parser to set all the bits we want for
> that function). This will fix my problem, since we'll never initialize any
> properties, but also means that classes will be pretty useless in the
> future, if people want to use them. Unless the builtin climate is too
> degenerate to support them, there's no good reason that classes "secretly
> call content", unless you do something obvious like derive from a
> content-provided function.

Again, I'm pretty sure that I don't understand the problem. Why would having properties/methods on these classes be problematic?

> Till, do you have any thoughts on this matter? In particular, can you advise
> about whether you think classes can be viable in self-hosted code?

As said above, I very much hope they will be viable. For builtins that don't need too much magic (and really, there are very few builtins that do), I don't see why we shouldn't be able to, in theory at least, define them fully in self-hosted code, ideally as classes. I think that'd be nice, too.
Flags: needinfo?(till)
OK, none of those comments made sense. Hopefully, the upcoming patches will.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8702378 - Flags: review?(till)
This was set and consulted in asserts, but I don't see what it buys us, and it's pretty dead looking. Maybe I missed something, but I don't think so.
Attachment #8702380 - Flags: review?(jwalden+bmo)
This doesn't address a core usability issue for classes in self-hosted code, which is that the class constructors are not marked as CONSTRUCTOR by default. This should be an easy for for anyone that wants to use classes in self-hosted code in future.
Attachment #8702381 - Flags: review?(till)
Attachment #8702379 - Flags: review?(jwalden+bmo) → review+
Attachment #8702380 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8702378 [details] [diff] [review]
Part 1: Allow opt-in calls to content invoking spread ops in self-hosted code.

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

Very straight-forward.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +7289,5 @@
>  bool
> +BytecodeEmitter::emitSelfHostedAllowContentSpread(ParseNode* pn)
> +{
> +    if (pn->pn_count != 2) {
> +        reportError(pn, JSMSG_MORE_ARGS_NEEDED, "allowContentSpread", "1", "");

I'd just assert this. No need to throw runtime errors for wrong usage of self-hosting internals.
Attachment #8702378 - Flags: review?(till) → review+
Comment on attachment 8702381 [details] [diff] [review]
Part 4: Self host default derived class constructor

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

Cancelling review because if my suggestion below works out the resulting patch would look somewhat different.

::: js/src/vm/Interpreter.cpp
@@ +308,5 @@
> +        return nullptr;
> +    }
> +
> +    MOZ_ASSERT(selfHosted.isObject());
> +    MOZ_ASSERT(selfHosted.toObject().is<JSFunction>());

Nit: both of these asserts are contained in the as<JSFunction> cast below and thus redundant.

@@ +319,5 @@
> +    if (!script)
> +        return nullptr;
> +    RootedObject staticScope(cx, script->enclosingStaticScope());
> +
> +    JSFunction* ctor = CloneFunctionAndScript(cx, selfHostedCtor, parent, staticScope,

Do you really need to clone the script here? Can't it be reused, given that the scope is always the same and all that?

And if so, do you even need to eagerly clone it? The alternative would be to do what GlobalObject::getSelfHostedFunction does: create a lazy function and set its LAZY_FUNCTION_NAME_SLOT to cx->names().DefaultDerivedClassConstructor.

@@ +325,5 @@
> +    if (!ctor)
> +        return nullptr;
> +
> +    RootedAtom name(cx, atom == cx->names().empty ? nullptr : atom);
> +    ctor->setRealAtomOnClone(name);

If the above suggestion works, you also wouldn't need to do this, as you'd create a new function with the correct atom right away.
Attachment #8702381 - Flags: review?(till)
Oh also, would it perhaps be possible to store the IS_DEFAULT_CTOR bit on the script instead? It could be set in MakeDefaultConstructor, and it *seems* like it's only requested in situations where a JSContext is available or the function is guaranteed not to be lazy.

I tried doing something similar for self-hosting .bind, but that bit is unfortunately required in situations where a JSContext isn't necessarily available and the function might be lazy.
(In reply to Till Schneidereit [:till] from comment #11)
> Oh also, would it perhaps be possible to store the IS_DEFAULT_CTOR bit on
> the script instead? It could be set in MakeDefaultConstructor, and it
> *seems* like it's only requested in situations where a JSContext is
> available or the function is guaranteed not to be lazy.
> 
> I tried doing something similar for self-hosting .bind, but that bit is
> unfortunately required in situations where a JSContext isn't necessarily
> available and the function might be lazy.

I'll give that a try. I did offer to try other solutions to save this bit for you, so let's see what we get.
With an attempt at all comments.
Attachment #8702381 - Attachment is obsolete: true
Attachment #8703474 - Flags: review?(till)
Attachment #8702382 - Attachment is obsolete: true
Attachment #8702382 - Flags: review?(till)
Comment on attachment 8703475 [details] [diff] [review]
Part 5: Self host default base class constructor for good measure

whoops
Attachment #8703475 - Flags: review?(till)
Comment on attachment 8703474 [details] [diff] [review]
Part 4: Self host default derived class constructor

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

\o/, thank you!

This will need rebasing on top of bug 1235656, which will require changes to some parts of it. Specifically, functions in the self-hosting compartment will be extended in debug builds, so some assumptions don't hold anymore. Talk about bad timing.

::: js/src/builtin/Classes.js
@@ +1,1 @@
> +// Give a builtin constructor that we can use as the default. When we give

This needs a license header.

::: js/src/vm/SelfHosting.cpp
@@ +1980,3 @@
>          js::gc::AllocKind kind = hasName
>                                   ? gc::AllocKind::FUNCTION_EXTENDED
> +                                 : gc::AllocKind::FUNCTION;

I'm afraid this and some other aspects of this patch will have to change once you rebase on top of bug 1235656 :( Sorry about that.
Attachment #8703474 - Flags: review?(till) → review+
Comment on attachment 8703475 [details] [diff] [review]
Part 5: Self host default base class constructor for good measure

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

I like it!
Attachment #8703475 - Flags: review?(till) → review+
You need to log in before you can comment on or make changes to this bug.