1216630 - Function.prototype.toString() on class must not return function source code string

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

Shell output:
---
js> (class C { constructor(){} }) 
(function C(){
"use strict";
})
js> (class C {})
function C() {
    
}
---

Expected: String representation is not a FunctionDeclaration/FunctionExpression
Actual: FunctionDeclaration syntax is used

Spec: ES2015 19.2.3.5 Function.prototype.toString ( )

Representing classes as FunctionDeclaration/FunctionExpression violates the third requirement in 19.2.3.5:

> If the object was defined using ECMAScript code and the returned string
> representation is not in the form of a MethodDefinition or GeneratorMethod
> then the representation must be such that if the string is evaluated, using
> eval in a lexical context that is equivalent to the lexical context used to
> create the original object, it will result in a new functionally equivalent
> object. 

Eval'ing the current string representation does not return a functionally equivalent object.

Comment 1

[ljharb]

Specifically, stringifying a "class" constructor should have the "class" keyword in the output.

Comment 2

[Di Weng]

FYI, as mentioned in https://github.com/angular/angular.js/issues/14240, this bug is breaking angular.js in the latest Firefox where ES6 classes are unable to function as controllers.

Comment 3

[Jan de Mooij [:jandem]]

We should probably get this fixed because it breaks real code, per comment 2.

Comment 4

[ljharb]

It also violates the upcoming spec for `Function#toString` https://github.com/tc39/Function-prototype-toString-revision

Comment 5

[jomerinog]

Would appreciate a fix for this since it stops people from using ES6 fully in angular. (comment 2)

Comment 6

[Eric Faust [:efaust]]

Attachment: Part 1: Move default class constructor creation to frontend

Comment 7

[Eric Faust [:efaust]]

Attachment: Part 2: Factor SourceLocation out from JSScript

Maybe I should also try to factor this out from LazyScript.

Comment 8

[Eric Faust [:efaust]]

Attachment: Part 3: Return entire class body from F.p.toString on classes

Till, can you make sure the unmarking of SELF_HOSTED here is kosher. Nothing seems to break, and we discussed it on IRC, but just to be sure.

Comment 9

[Eric Faust [:efaust]]

Attachment: Part 4: Remove now unused JSFunction::isDefaultClassConstructor and affiliated accessors.

Comment 10

[Till Schneidereit [:till]]

Comment on attachment 8783143 [details] [diff] [review]
Part 1: Move default class constructor creation to frontend

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

I'm sorry you had to write this patch :(

::: js/src/jsfun.cpp
@@ +1325,5 @@
>  
>      MOZ_ASSERT_IF(isDefault, isConstructor());
>      MOZ_ASSERT_IF(isDefault, isClassConstructor());
>      return isDefault;
> +

Nit: remove blank line.

@@ +1334,3 @@
>  {
>      bool derived;
>      if (isInterpretedLazy()) {

Shouldn't you be able to simplify this in the same manner as infallibleIsDefaultClassConstructor?

::: js/src/jsfun.h
@@ +354,5 @@
>      }
>  
> +    // Before we clone them out, the self-hosted default constructors have several
> +    // unfortunate properties. They are marked as self-hosted, and, because of
> +    // limitartions on self-hosted code, they are marked as having a guessed

s/limitartions/limitations/

Also, we probably need to find a solution for this limitartion anyway. See e.g. bug 1296237, bug 1296234, and bug 1296235.

@@ +357,5 @@
> +    // unfortunate properties. They are marked as self-hosted, and, because of
> +    // limitartions on self-hosted code, they are marked as having a guessed
> +    // name. Since the clones are always going to have explicit names, and
> +    // since we are going to set up source information for them, prepare the clone
> +    // by unsetting some unfortnuately flags.

s/unfortnuately/unfortunate/

Also, they really have exactly one unfortunate property, right? Perhaps simplify the comment to that effect?

::: js/src/jsscript.cpp
@@ +797,5 @@
>                  }
>  
> +                if (funEnclosingScope == &cx->global()->emptyGlobalScope()) {
> +                    MOZ_ASSERT(function->isClassConstructor());
> +

Nit: remove blank line.

::: js/src/jsscriptinlines.h
@@ +198,5 @@
> +        return false;
> +
> +    // Default class constructors. The world is not prepared for self-hosted
> +    // functions without the name in a slot.
> +    if (selfHosted() && functionNonDelazifying()->isClassConstructor())

One thing I've been thinking about is to store a pointer to the original self-hosted function instead of the name. We'd probably want to do that as a private value and pretend it never happened to the rest of the engine, but I don't see a fundamental reason it wouldn't work.

::: js/src/vm/Interpreter.cpp
@@ +4039,4 @@
>      PUSH_MAGIC(JS_IS_CONSTRUCTING);
>  END_CASE(JSOP_IS_CONSTRUCTING)
>  
> +

Nit: remove blank line.

Comment 11

[Till Schneidereit [:till]]

Comment on attachment 8783147 [details] [diff] [review]
Part 3: Return entire class body from F.p.toString on classes

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

I'm not sure I understand how this works at all. We don't keep the sources for self-hosted code around, so how can you use them for F.p.toString? Or, rather, if that is what you're doing and it works, then we *do* keep the sources around.

In the latter case, we should stop doing it. Instead, either just embed the source version of the class constructors in js::FunctionToString and emit them manually, or add support for re-reading the source, including unzipping it. The former seems preferable to me.

Note that I fully expect that I'm overlooking something silly and this all works in completely different ways. Canceling review given my lack of understanding of how this works.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +8724,3 @@
>  {
> +    RootedFunction target(cx, NewFunctionWithProto(cx, nullptr, isDerived,
> +                                                JSFunction::INTERPRETED_CLASS_CONSTRUCTOR,

Nit: indentation off for this and the following lines.

::: js/src/jsfun.h
@@ +363,5 @@
>          MOZ_ASSERT(isClassConstructor());
>          MOZ_ASSERT(isSelfHostedBuiltin());
>          MOZ_ASSERT(hasGuessedAtom());
>  
> +        flags_ &= ~(HAS_GUESSED_ATOM | SELF_HOSTED);

Ah, I see this answers one of my questions from the previous review.

Comment 12

[Till Schneidereit [:till]]

Comment on attachment 8783147 [details] [diff] [review]
Part 3: Return entire class body from F.p.toString on classes

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

efaust explained this to me on IRC. The gist is that this just uses the sources provided by the content we're using a default constructor for, so no self-hosted code is harmed in the making of it.

With that, r=me on the self-hosting-related parts. I mostly skimmed the rest.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +8724,3 @@
>  {
> +    RootedFunction target(cx, NewFunctionWithProto(cx, nullptr, isDerived,
> +                                                JSFunction::INTERPRETED_CLASS_CONSTRUCTOR,

Nit: indentation off for this and the following lines.

::: js/src/jsfun.h
@@ +363,5 @@
>          MOZ_ASSERT(isClassConstructor());
>          MOZ_ASSERT(isSelfHostedBuiltin());
>          MOZ_ASSERT(hasGuessedAtom());
>  
> +        flags_ &= ~(HAS_GUESSED_ATOM | SELF_HOSTED);

Ah, I see this answers one of my questions from the previous review.

Comment 13

[Shu-yu Guo [:shu]]

Comment on attachment 8783143 [details] [diff] [review]
Part 1: Move default class constructor creation to frontend

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

That asJSContext() would crash on async-parsed scripts with classes. Need to figure something out there.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +8712,5 @@
>      return emitInitializeName(pn, assertLexical);
>  }
>  
> +bool
> +BytecodeEmitter::emitDefaultClassConstructor(bool isDerived, HandleAtom name)

This function is kinda scary. Please have an explanatory comment for why it's making a new function, cloning a self-hosted script into it (to hold the source for toString).

@@ +8720,5 @@
> +        RootedFunction target(cx, NewFunctionWithProto(cx, nullptr, isDerived,
> +                                                    JSFunction::INTERPRETED_CLASS_CONSTRUCTOR,
> +                                                    nullptr, name, nullptr,
> +                                                    gc::AllocKind::FUNCTION_EXTENDED,
> +                                                    TenuredObject));

Arguments don't line up properly.

@@ +8729,5 @@
> +            target->setHasRest();
> +
> +        RootedPropertyName selfHostedName(cx, isDerived ? cx->names().DefaultDerivedClassConstructor
> +                                                        : cx->names().DefaultBaseClassConstructor);
> +        JSContext* context = cx->asJSContext();

This doesn't seem okay. How are you ensuring classes aren't parsed/emitted off-thread?

@@ +8740,5 @@
> +        MOZ_ASSERT(target->isConstructor());
> +        MOZ_ASSERT(target->isClassConstructor());
> +        MOZ_ASSERT(!target->hasGuessedAtom());
> +
> +        FunctionBox* box = parser->newFunctionBox(nullptr, target, Directives(sc->strict()), NotGenerator, false);

Aren't classes always strict?

::: js/src/frontend/BytecodeEmitter.h
@@ +333,5 @@
>      }
>  
> +    unsigned defaultConstructorIndex(bool isDerived) {
> +        return isDerived ? defaultDerivedConstructorIndex
> +                         : defaultBaseConstructorIndex;

Small nit: change to if-else style so visually matches with setDefaultConstructorIndex below.

::: js/src/jsscript.cpp
@@ +796,5 @@
>                      return false;
>                  }
>  
> +                if (funEnclosingScope == &cx->global()->emptyGlobalScope()) {
> +                    MOZ_ASSERT(function->isClassConstructor());

Just when I removed special UINT32_MAX magic during XDR. :(

It would be clearer if the check is |if (function->isClassConstructor())|.

::: js/src/vm/Interpreter.cpp
@@ +3413,5 @@
>      /* Load the specified function object literal. */
>      ReservedRooted<JSFunction*> fun(&rootFunction0, script->getFunction(GET_UINT32_INDEX(REGS.pc)));
> +    ReservedRooted<JSObject*> env(&rootObject0, JSOp(*REGS.pc) == JSOP_CLASSCONSTRUCTOR
> +                                                    ? &cx->global()->lexicalEnvironment()
> +                                                    : REGS.fp()->environmentChain());

Indentation of ? : seems odd.

@@ +3924,5 @@
>      ReservedRooted<JSFunction*> fun(&rootFunction0, script->getFunction(GET_UINT32_INDEX(REGS.pc)));
>  
> +    ReservedRooted<JSObject*> env(&rootObject1, JSOp(*REGS.pc) == JSOP_DERIVEDCONSTRUCTOR
> +                                                  ? &cx->global()->lexicalEnvironment()
> +                                                  : REGS.fp()->environmentChain());

Indentation of ? : seems odd.

Comment 14

[Shu-yu Guo [:shu]]

Comment on attachment 8783145 [details] [diff] [review]
Part 2: Factor SourceLocation out from JSScript

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

::: js/src/frontend/SharedContext.h
@@ +456,5 @@
> +    }
> +
> +    void setEnd(uint32_t end) {
> +        bufEnd = end;
> +    }

Any point in having setEnd since this is a struct?

Comment 15

[Shu-yu Guo [:shu]]

Comment on attachment 8783147 [details] [diff] [review]
Part 3: Return entire class body from F.p.toString on classes

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

r=me conditioned on figuring something out for the asJSContext() issue that's carried over from part 1.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +8735,4 @@
>  
> +    RootedPropertyName selfHostedName(cx, isDerived ? cx->names().DefaultDerivedClassConstructor
> +                                                    : cx->names().DefaultBaseClassConstructor);
> +    JSContext* context = cx->asJSContext();

Can't do this, same problem as part 1.

@@ +8825,3 @@
>              return false;
>      }
>  

MOZ_ASSERT(constructor), since now a default one or a user-supplied one must be found.

::: js/src/frontend/ParseNode.h
@@ +1249,4 @@
>      }
>  };
>  
> +struct ClassDefaultConstructor : public ParseNode {

Nit: { on own line

::: js/src/frontend/Parser.cpp
@@ +6107,4 @@
>  
>      bool savedStrictness = setLocalStrictMode(true);
>  
> +    SourceLocation* sourceLoc = newSourceLocation(tokenStream);

Now that I'm reading its use, SourceLocation is better named SourceLocationSpan.

@@ +6178,4 @@
>      if (!classMethods)
>          return null();
>  
> +    FunctionBox *seenConstructor = nullptr;

FunctionBox*

::: js/src/jsfun.h
@@ +358,4 @@
>      // limitartions on self-hosted code, they are marked as having a guessed
>      // name. Since the clones are always going to have explicit names, and
>      // since we are going to set up source information for them, prepare the clone
> +    // by unsetting some unfortnuate flags.

unfortunate

::: js/src/jsscript.cpp
@@ +982,4 @@
>                    HandleFunction, MutableHandle<LazyScript*>);
>  
>  void
> +JSScript::setSourceLocation(SourceLocation* sourceLoc)

Similarly, I think this is better named setSourceLocationSpan

Comment 16

[Shu-yu Guo [:shu]]

Comment on attachment 8783147 [details] [diff] [review]
Part 3: Return entire class body from F.p.toString on classes

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

::: js/src/frontend/ParseNode.h
@@ +632,4 @@
>                                             1. array initialiser has holes
>                                             2. array initializer has spread node */
>  #define PNX_NONCONST    0x08            /* initialiser has non-constants */
> +#define PNX_CLASSHASDEFAULT 0x10        /* class method list contains default constructor */

This flag seems to only be used to make Reflect.parse's life easier. Please delete the flag and just use append on the methods vector instead of reserve + infallibleAppend.

Comment 18

[Michał Gołębiowski-Owczarek [:mgol]]

Is there anything blocking this bug? It's gotten its review finished as far as I understand.

I'm asking because it still breaks Angular 1.x when ES6 classes are used as component/directive constructors.

Comment 19

[Jan de Mooij [:jandem]]

(In reply to Michał Gołębiowski [:m_gol] from comment #18)
> Is there anything blocking this bug? It's gotten its review finished as far
> as I understand.
> 
> I'm asking because it still breaks Angular 1.x when ES6 classes are used as
> component/directive constructors.

Eric left Mozilla. Shu, since you reviewed most of the patches, maybe you want to finish this?

Comment 20

[Shu-yu Guo [:shu]]

(In reply to Jan de Mooij [:jandem] from comment #19)
> (In reply to Michał Gołębiowski [:m_gol] from comment #18)
> > Is there anything blocking this bug? It's gotten its review finished as far
> > as I understand.
> > 
> > I'm asking because it still breaks Angular 1.x when ES6 classes are used as
> > component/directive constructors.
> 
> Eric left Mozilla. Shu, since you reviewed most of the patches, maybe you
> want to finish this?

Okay, need to page this back in.

Comment 21

[Michał Gołębiowski-Owczarek [:mgol]]

(In reply to Shu-yu Guo [:shu] from comment #20)
> (In reply to Jan de Mooij [:jandem] from comment #19)
> > Eric left Mozilla. Shu, since you reviewed most of the patches, maybe you
> > want to finish this?
> 
> Okay, need to page this back in.

Shu, are there any major roadblocks to this landing? I'm considering writing a Babel plugin to workaround the issue for the sake of AngularJS (basically tagging all classes with an internal property that AngularJS uses to cache the class detection test results) but if this issue is going to get fixed soon, I'd rather just wait a little.

Comment 22

[Jeff Walden [:Waldo]]

efaust indicated last week over IRL and IRC that he was going to pick this up again, during an interregnum between non-Mozilla employments, with the plan of adjusting the approach to not be quite so fragile as the current one.  Under that understanding, I don't believe we plan to take the current patches.  (Not to mention, based on his apparent grumbling over IRL, the patches needed substantial work to be rebased and so *couldn't* just be directly landed now even if we wanted them to.)

Comment 23

[Shu-yu Guo [:shu]]

Attachment: Print class source when calling toString on the constructor.

This is accomplished in the following ways.

LazyScripts and JSScripts now have 4 offsets:

 - Source begin and end for the actual source. This is used for lazy
   parsing.

 - toString begin and end for toString. Some kinds of functions, like
   async, only have a different begin offset. Class constructors have
   different offsets for both begin and end.

For syntactically present (i.e. non-default) constructors, the class
source span is remembered directly on the LazyScript or JSScript. The
toString implementation then splices out the substring directly.

For default constructors, a new SRC_CLASS SrcNote type is added. It's
binary and has as its arguments the begin and end offsets of the class
expression or statement. MakeDefaultConstructor reads the note and
overrides the cloned self-hosted function's source object. This is
probably the least intrusive way to accomplish this.

Comment 24

[Shu-yu Guo [:shu]]

Attachment: Rename preludeStart and postludeEnd to toStringStart and toStringEnd and misc fixes.

Comment 25

[Shu-yu Guo [:shu]]

Attachment: Unskip test262 class toString tests.

Comment 26

[Shu-yu Guo [:shu]]

Comment on attachment 8857273 [details] [diff] [review]
Print class source when calling toString on the constructor.

Redirecting review to Yoric to spread around expertise.

Comment 27

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8857273 [details] [diff] [review]
Print class source when calling toString on the constructor.

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

Sorry for the delay, Firefox and/or Splinter managed to lose my review for your patch twice.

r=me, with a number of questions because I'm not familiar with the code yet

::: js/src/frontend/BytecodeCompiler.cpp
@@ +62,5 @@
>      bool maybeCompressSource();
>      bool canLazilyParse();
>      bool createParser();
>      bool createSourceAndParser(const Maybe<uint32_t>& parameterListEnd = Nothing());
> +    bool createScript(uint32_t preludeStart = 0, uint32_t postludeEnd = 0);

At this point in code, `preludeStart` and `postludeEnd` are not self-documenting. Could you add a link to their doc?

::: js/src/frontend/Parser.cpp
@@ +7039,5 @@
>                                        DefaultHandling defaultHandling)
>  {
>      MOZ_ASSERT(tokenStream.isCurrentTokenType(TOK_CLASS));
>  
> +    uint32_t classStartOffset = pos().begin;

const?

@@ +7151,5 @@
>              propType = PropertyType::GetterNoExpressionClosure;
>          if (propType == PropertyType::Setter)
>              propType = PropertyType::SetterNoExpressionClosure;
> +
> +        bool isConstructor = !isStatic && propAtom == context->names().constructor;

const?

@@ -7196,5 @@
>          methodsOrBlock = classBlock;
>  
>          // Pop the inner scope.
>          classScope.reset();
> -        classStmt.reset();

Why isn't this needed anymore?

::: js/src/frontend/SharedContext.h
@@ +451,5 @@
>      uint32_t        bufEnd;
>      uint32_t        startLine;
>      uint32_t        startColumn;
>      uint32_t        preludeStart;
> +    uint32_t        postludeEnd;

Can you clarify what that means?

::: js/src/jsfun.cpp
@@ +1023,5 @@
> +    // Default class constructors are self-hosted, but have their source
> +    // objects overridden to refer to the span of the class statement or
> +    // expression. Non-default class constructors are never self-hosted. So,
> +    // all class constructors always have source.
> +    bool haveSource = fun->isInterpreted() && (fun->isClassConstructor() ||

const?

::: js/src/jsscript.cpp
@@ +237,5 @@
>      {
>          uint32_t begin = script->sourceStart();
>          uint32_t end = script->sourceEnd();
>          uint32_t preludeStart = script->preludeStart();
> +        uint32_t postludeEnd = script->postludeEnd();

const?

@@ +258,5 @@
>  
>          if (!xdr->codeUint64(&packedFields))
>              return false;
>  
>          if (mode == XDR_DECODE) {

Out of curiosity, when do we use XDR to (de)serialize code?

@@ +263,5 @@
>              RootedScriptSource sourceObject(cx, &script->scriptSourceUnwrap());
>              lazy.set(LazyScript::Create(cx, fun, script, enclosingScope, sourceObject,
>                                          packedFields, begin, end, preludeStart, lineno, column));
> +            if (!lazy)
> +                return false;

That's in case of OOM, right?

Out of curiosity, could we imagine a static analysis that would let the compiler/analyzer detect that we haven't checked for nullptr returns in functions that may return OOM?

::: js/src/jsscript.h
@@ +1090,5 @@
>      // Add padding so JSScript is gc::Cell aligned. Make padding protected
>      // instead of private to suppress -Wunused-private-field compiler warnings.
>    protected:
>  #if JS_BITS_PER_WORD == 32
> +    // Currently no padding is needed.

Out of curiosity, could this be checked by a static assert? Maybe using an `offsetof` and a `void` field?

@@ -1242,5 @@
>      uint32_t sourceEnd() const {
>          return sourceEnd_;
>      }
>  
> -    size_t preludeStart() const {

Having it a `size_t` was a bug, right?

::: js/src/vm/Interpreter.cpp
@@ +250,3 @@
>  {
> +    JSOp op = JSOp(*pc);
> +    JSAtom* atom = script->getAtom(pc);

Doesn't the atom need to be rooted?

@@ +279,5 @@
> +    // source for just the constructor function.
> +    JSScript *ctorScript = JSFunction::getOrCreateScript(cx, ctor);
> +    if (!ctorScript)
> +        return nullptr;
> +    uint32_t classStartOffset = GetSrcNoteOffset(classNote, 0);

const?

@@ +280,5 @@
> +    JSScript *ctorScript = JSFunction::getOrCreateScript(cx, ctor);
> +    if (!ctorScript)
> +        return nullptr;
> +    uint32_t classStartOffset = GetSrcNoteOffset(classNote, 0);
> +    uint32_t classEndOffset = GetSrcNoteOffset(classNote, 1);

const?

Comment 28

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8857274 [details] [diff] [review]
Rename preludeStart and postludeEnd to toStringStart and toStringEnd and misc fixes.

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

::: js/src/frontend/BytecodeCompiler.cpp
@@ +62,5 @@
>      bool maybeCompressSource();
>      bool canLazilyParse();
>      bool createParser();
>      bool createSourceAndParser(const Maybe<uint32_t>& parameterListEnd = Nothing());
> +    bool createScript();

Makes sense, but this documentation, too, to let devs find out which method they should use.

::: js/src/jsfun.cpp
@@ +882,5 @@
>                                               sourceObject,
>                                               begin,
>                                               ss->length(),
> +                                             0,
> +                                             ss->length()));

Oh, the 0 was a bug, right?

::: js/src/jsscript.cpp
@@ +2550,4 @@
>  {
>      MOZ_ASSERT(bufStart <= bufEnd);
> +    MOZ_ASSERT(toStringStart <= toStringEnd);
> +    MOZ_ASSERT(toStringStart <= bufStart);

If that's the case, I'm not clear what `bufStart` and `bufEnd` mean.

Comment 29

[Shu-yu Guo [:shu]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #27)
> Comment on attachment 8857273 [details] [diff] [review]
> Print class source when calling toString on the constructor.
> 
> Review of attachment 8857273 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the delay, Firefox and/or Splinter managed to lose my review for
> your patch twice.
> 
> r=me, with a number of questions because I'm not familiar with the code yet
> 
> ::: js/src/frontend/BytecodeCompiler.cpp
> @@ +62,5 @@
> >      bool maybeCompressSource();
> >      bool canLazilyParse();
> >      bool createParser();
> >      bool createSourceAndParser(const Maybe<uint32_t>& parameterListEnd = Nothing());
> > +    bool createScript(uint32_t preludeStart = 0, uint32_t postludeEnd = 0);
> 
> At this point in code, `preludeStart` and `postludeEnd` are not
> self-documenting. Could you add a link to their doc?

These are renamed toStringStart/End in the next patch. I'll write comments there for what they mean. The breakdown is:

bufStart and bufEnd are the offsets in the ScriptSource used for parsing the contents of the script, in the case of delazification and relazification.

toStringStart and toStringEnd are for generating the string returned by Function.prototype.toString().

> 
> ::: js/src/frontend/Parser.cpp
> @@ +7039,5 @@
> >                                        DefaultHandling defaultHandling)
> >  {
> >      MOZ_ASSERT(tokenStream.isCurrentTokenType(TOK_CLASS));
> >  
> > +    uint32_t classStartOffset = pos().begin;
> 
> const?

For these and other const comments below, those are fine but nothing else in the engine really uses the style of qualifying machine types like that with const. For uniformity I'd rather leave them unqualified.

> 
> @@ -7196,5 @@
> >          methodsOrBlock = classBlock;
> >  
> >          // Pop the inner scope.
> >          classScope.reset();
> > -        classStmt.reset();
> 
> Why isn't this needed anymore?

Good catch! This is a bug. A couple background things here.

1. JS doesn't allow unbraced lexical declarations, e.g. |if (1) let x| is a SyntaxError, whereas |if (1) { let x }| is allowed.

2. ParseContext::*Statement classes are RAII things for keeping track of the "statement stack", which is what tracks if we are currently in a braced or unbraced context.

3. If a JS class has a name, it introduces that name as a 'const' binding into a new lexical scope, so that code inside the class body can refer to the name of the class. This is the "inner name". This only happens if the JS class has a name.

So because of 3, classStmt was previously wrapped in a Maybe<> and only instantiated if we had a name, to allow the 'const' lexical binding to be declared even if the class statement/expression is in an unbraced context in source.

With this patch, I use the ParseContext::Statement stack to also track the FunctionBox of the class constructor, if one is present. So now I unconditionally instantiate the ParseContext::Statement, and I thought we don't need to reset it. But actually the ParseContext::ClassStatement shouldn't be considered braced, and we still should push a block statement in the case of a named class.

> 
> @@ +258,5 @@
> >  
> >          if (!xdr->codeUint64(&packedFields))
> >              return false;
> >  
> >          if (mode == XDR_DECODE) {
> 
> Out of curiosity, when do we use XDR to (de)serialize code?
> 

(Some?) chrome code is serialized and loaded on startup with XDR. :nbp's bytecode cache also uses XDR.

> @@ +263,5 @@
> >              RootedScriptSource sourceObject(cx, &script->scriptSourceUnwrap());
> >              lazy.set(LazyScript::Create(cx, fun, script, enclosingScope, sourceObject,
> >                                          packedFields, begin, end, preludeStart, lineno, column));
> > +            if (!lazy)
> > +                return false;
> 
> That's in case of OOM, right?
> 
> Out of curiosity, could we imagine a static analysis that would let the
> compiler/analyzer detect that we haven't checked for nullptr returns in
> functions that may return OOM?
> 

We have that MOZ_MUST_USE annotation for bool. There's also a possibly halted but very invasive large-scale refactoring to have fallible functions return JS::Result. See js/public/Result.h. Most of the engine isn't converted yet, though.

> ::: js/src/jsscript.h
> @@ +1090,5 @@
> >      // Add padding so JSScript is gc::Cell aligned. Make padding protected
> >      // instead of private to suppress -Wunused-private-field compiler warnings.
> >    protected:
> >  #if JS_BITS_PER_WORD == 32
> > +    // Currently no padding is needed.
> 
> Out of curiosity, could this be checked by a static assert? Maybe using an
> `offsetof` and a `void` field?

We have one at the end of the class definition.

> 
> @@ -1242,5 @@
> >      uint32_t sourceEnd() const {
> >          return sourceEnd_;
> >      }
> >  
> > -    size_t preludeStart() const {
> 
> Having it a `size_t` was a bug, right?

Yeah.

> 
> ::: js/src/vm/Interpreter.cpp
> @@ +250,3 @@
> >  {
> > +    JSOp op = JSOp(*pc);
> > +    JSAtom* atom = script->getAtom(pc);
> 
> Doesn't the atom need to be rooted?
> 

It's not live across any can-GC function calls. js::GetSrcNote can't GC.

Comment 30

[Shu-yu Guo [:shu]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #28)
> Comment on attachment 8857274 [details] [diff] [review]
> Rename preludeStart and postludeEnd to toStringStart and toStringEnd and
> misc fixes.
> 
> Review of attachment 8857274 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/frontend/BytecodeCompiler.cpp
> @@ +62,5 @@
> >      bool maybeCompressSource();
> >      bool canLazilyParse();
> >      bool createParser();
> >      bool createSourceAndParser(const Maybe<uint32_t>& parameterListEnd = Nothing());
> > +    bool createScript();
> 
> Makes sense, but this documentation, too, to let devs find out which method
> they should use.
> 

Sure I'll comment.

> ::: js/src/jsfun.cpp
> @@ +882,5 @@
> >                                               sourceObject,
> >                                               begin,
> >                                               ss->length(),
> > +                                             0,
> > +                                             ss->length()));
> 
> Oh, the 0 was a bug, right?
> 

Yeah.

> ::: js/src/jsscript.cpp
> @@ +2550,4 @@
> >  {
> >      MOZ_ASSERT(bufStart <= bufEnd);
> > +    MOZ_ASSERT(toStringStart <= toStringEnd);
> > +    MOZ_ASSERT(toStringStart <= bufStart);
> 
> If that's the case, I'm not clear what `bufStart` and `bufEnd` mean.

I'll comment. bufStart and bufEnd are really for the parser: where to start and stop parsing.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/751cc121aa3f
Print class source when calling toString on the constructor. (r=Yoric)
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f3e4478d23a
Rename preludeStart and postludeEnd to toStringStart and toStringEnd and misc fixes. (r=Yoric)
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb53383c8cd3
Unskip test262 class toString tests. (r=Yoric)

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/751cc121aa3f
https://hg.mozilla.org/mozilla-central/rev/8f3e4478d23a
https://hg.mozilla.org/mozilla-central/rev/bb53383c8cd3

Comment 33

[Michał Gołębiowski-Owczarek [:mgol]]

I've found a bug in the current implementation: filed as bug 1357483.