1105463 - Implement default constructors for ClassDefinitions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eric Faust [:efaust]]

The spec says that if |constructor| is not present as a property, we need to do some extra magic. Split that magic off here. More on this in bug 1066229.

Comment 1

[Eric Faust [:efaust]]

Attachment: Patch \o/

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 8658410 [details] [diff] [review]
Patch \o/

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

Everything looks correct. Needs tests. Clearing review for now.

::: js/src/jsfun.cpp
@@ +1074,5 @@
>          MOZ_ASSERT(!fun->isExprBody());
>  
> +        if (fun->isNative() && fun->native() == js::DefaultDerivedClassConstructor) {
> +            if ((!bodyOnly && !out.append("(...args) {\n    ")) ||
> +                !out.append("super(...args);\n}"))

This spits out a "\n}" that should only appear if !bodyOnly.

::: js/src/vm/Interpreter.cpp
@@ +5029,5 @@
>      ReportUninitializedLexical(cx, name);
>  }
> +
> +bool
> +js::DefaultClassConstructor(JSContext* cx, unsigned argc, Value* vp)

I get that this doesn't fit anywhere, but vm/Interpreter.cpp is an especially crowded place...

A new file would be best (vm/Classes.cpp or vm/DefaultConstructors.cpp). Dump it in jsobj.cpp if you are in a huge hurry.

Comment 3

[Eric Faust [:efaust]]

Attachment: Tests!

These should all look familiar ;)

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 8663135 [details] [diff] [review]
Tests!

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

Looks good.

::: js/src/tests/ecma_6/Class/superCallInvalidBase.js
@@ +4,5 @@
>      constructor() { super(); }
>  }
>  
>  assertThrowsInstanceOf(() => new instance(), TypeError);
> +assertThrowsInstanceOf(() => new class extends null { }(), TypeError);

Please add:

class Foo extends Object {}
Object.setPrototypeOf(Foo, Math.sin);  // not a constructor
assertThrowsInstanceOf(() => new Foo, TypeError);
Object.setPrototypeOf(Foo, Object.create(Object));  // still not a constructor
assertThrowsInstanceOf(() => new Foo, TypeError);

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8658410 [details] [diff] [review]
Patch \o/

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

r=me, let's go

::: js/src/vm/Interpreter.cpp
@@ +5081,5 @@
> +    }
> +
> +    ConstructArgs constArgs(cx);
> +    if (!FillArgumentsFromArraylike(cx, constArgs, args))
> +        return false;

This made me think of a test:

var cls = class {};
for (var i = 0; i < 1000; i++)
    cls = class extends cls {};
assertThrowsInstanceOf(() => new cls, InternalError);  // too much recursion

var manyArgs = [];
manyArgs[30000] = "x";
assertThrowsInstanceOf(() => new cls(...manyArgs), InternalError);  // too much recursion



And, the same thing but with an explicit `constructor(...args) { super(...args); }` please.

(decoder's fuzzer should have that 1000-deep class lineage in its arsenal.)

(I'm not actually sure InternalError is correct, whatever happens is probably OK as long as it doesn't crash... or return normally!)

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf4cdc781c72

Comment 7

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

Backed out for build bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/240096755eec
https://treeherder.mozilla.org/logviewer.html#?job_id=15447172&repo=mozilla-inbound

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/250cd0bf3ce0

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ca52215a62f

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/250cd0bf3ce0
https://hg.mozilla.org/mozilla-central/rev/6ca52215a62f

Comment 11

[Eric Faust [:efaust]]

Got it stuck, canceling ni?

Comment 12

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/constructor#Default_constructors