785645 - Can't implement constructors in self-hosted JavaScript

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Norbert Lindenberg]

The current infrastructure for self-hosted JavaScript supports only plain functions, not constructors. Constructors are not only for big top-level objects, such as Intl.Collator, where it's somewhat reasonable to require implementing them in C++, but also for little helper objects such as the Record and List objects discussed in bug 462300 comment 23.

Comment 1

[Till Schneidereit [:till]]

Attachment: Add %_MakeConstructible

This adds a new function %_MakeConstructible to the self-hosting environment.

It's a bit annoying to have to be this verbose about constructors, but I don't see any other way to satisfy both the requirement that builtin function not be constructible by default and that we want to be able to self-host constructors.

Comment 2

[Norbert Lindenberg]

Where and how would I call this function?

Ideally, we'd have some label on the function itself that tells the compiler to treat it as a constructor, e.g.:

constructor MyClass(...) { ... }

function MyClass(...) { "constructor"; ... }

function MyClass(...) { %_Constructor(); ... }

Comment 3

[Till Schneidereit [:till]]

(In reply to Norbert Lindenberg from comment #2)
> Where and how would I call this function?
> 
> Ideally, we'd have some label on the function itself that tells the compiler
> to treat it as a constructor, e.g.:

Sorry, I should've given a usage example. As it is right now, you give it a reference to the function you want to make constructible:

function MyClass(){}
%_MakeConstructible(MyClass);

Your proposals are definitely nicer, but they also require changes to the parser and the bytecode emitter, so implementing them isn't nearly as straight-forward as the current solution.

I'd very much like to concentrate on getting the other issues blocking your work out of the way, so if you're ok with that, I'd revisit this at a later date.

Comment 4

[Norbert Lindenberg]

OK, that sounds reasonable.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 655927 [details] [diff] [review]
Add %_MakeConstructible

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

::: js/src/jscntxt.cpp
@@ +272,5 @@
>      }
>  
> +    /* We don't clone if we're operating in the self-hosting global, as that means we're currently
> +     * executing the self-hosting script itself. */
> +    if (cx->global() == selfHostedGlobal_) {

So, is the reason for this that, before, we were cloning scripts on initialization which meant that the flag was set on a clone that immediately became garbage?  How were these clones getting cloned to begin with?

Also, multi-line comments are:
 /*
  * ...
  */

@@ +273,5 @@
>  
> +    /* We don't clone if we're operating in the self-hosting global, as that means we're currently
> +     * executing the self-hosting script itself. */
> +    if (cx->global() == selfHostedGlobal_) {
> +        vp->setObjectOrNull(&funVal.toObject());

*vp = ObjectValue(funVal.toObject());
please (similarly with *clone below)

Comment 6

[Till Schneidereit [:till]]

Attachment: Add %_MakeConstructible, v2

(In reply to Luke Wagner [:luke] from comment #5)

> > +    /* We don't clone if we're operating in the self-hosting global, as that means we're currently
> > +     * executing the self-hosting script itself. */
> > +    if (cx->global() == selfHostedGlobal_) {
> 
> So, is the reason for this that, before, we were cloning scripts on
> initialization which meant that the flag was set on a clone that immediately
> became garbage?  How were these clones getting cloned to begin with?

Pretty much, yes. The reason I didn't hit this earlier is that I didn't have any expressions in the self-hosted code that were executed immediately. Basically, all I had (and all that's needed for many use-cases) were lots of function declarations.

> *vp = ObjectValue(funVal.toObject());
> please (similarly with *clone below)

D'oh!

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 656220 [details] [diff] [review]
Add %_MakeConstructible, v2

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

Nice work, thanks!

::: js/src/jscntxt.cpp
@@ +268,5 @@
>      }
>  
> +    /*
> +     * We don't clone if we're operating in the self-hosting global, as that
> +     * means we're currently executing the self-hosting script itself.

Could you give a little more explanation of when we execute the self-hosted script itself?  Something like "...means we're currently executing the self-hosted script while initializing the runtime (see JSRuntime::initSelfHosting)."

Comment 8

[Till Schneidereit [:till]]

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

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ab88572c117c