All users were logged out of Bugzilla on October 13th, 2018

Can't implement constructors in self-hosted JavaScript

RESOLVED FIXED in mozilla18

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mozillabugs, Assigned: till)

Tracking

(Blocks: 1 bug)

Other Branch
mozilla18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 655927 [details] [diff] [review]
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.
Assignee: general → tschneidereit
Status: NEW → ASSIGNED
Attachment #655927 - Flags: review?(luke)
(Reporter)

Comment 2

6 years ago
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(); ... }
(Assignee)

Comment 3

6 years ago
(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.
(Reporter)

Comment 4

6 years ago
OK, that sounds reasonable.

Comment 5

6 years ago
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)
(Assignee)

Comment 6

6 years ago
Created attachment 656220 [details] [diff] [review]
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!
Attachment #655927 - Attachment is obsolete: true
Attachment #655927 - Flags: review?(luke)
Attachment #656220 - Flags: review?(luke)

Comment 7

6 years ago
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)."
Attachment #656220 - Flags: review?(luke) → review+

Comment 9

6 years ago
https://hg.mozilla.org/mozilla-central/rev/ab88572c117c
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(Reporter)

Updated

6 years ago
Blocks: 769872
You need to log in before you can comment on or make changes to this bug.