Implement default constructors for ClassDefinitions

RESOLVED FIXED in Firefox 44

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: efaust, Assigned: efaust)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla44
x86_64
Linux
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

(Whiteboard: [DocArea=JS])

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
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.
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
(Assignee)

Comment 1

2 years ago
Created attachment 8658410 [details] [diff] [review]
Patch \o/
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8658410 - Flags: review?(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.
Attachment #8658410 - Flags: review?(jorendorff)
(Assignee)

Comment 3

2 years ago
Created attachment 8663135 [details] [diff] [review]
Tests!

These should all look familiar ;)
Attachment #8663135 - Flags: review?(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);
Attachment #8663135 - Flags: review?(jorendorff) → review+
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!)
Attachment #8658410 - Flags: review+

Comment 6

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf4cdc781c72
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
Flags: needinfo?(efaustbmo)

Comment 8

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

Comment 9

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ca52215a62f
https://hg.mozilla.org/mozilla-central/rev/250cd0bf3ce0
https://hg.mozilla.org/mozilla-central/rev/6ca52215a62f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
(Assignee)

Comment 11

2 years ago
Got it stuck, canceling ni?
Flags: needinfo?(efaustbmo)
Depends on: 1214397
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/constructor#Default_constructors
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 1214970
Depends on: 1233100
Depends on: 1233149
Depends on: 1236548
Depends on: 1236600
Depends on: 1254122
Depends on: 1311060
You need to log in before you can comment on or make changes to this bug.