Closed Bug 924672 Opened 6 years ago Closed 5 years ago

Implement ES6 Method Definitions

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: bbenvie, Assigned: gupta.rajagopal)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS])

Attachments

(1 file, 3 obsolete files)

ES6 introduces MethodDefinitions to ObjectExpression and ClassDefinitions. MethodDefinitions are shorthand for a function assigned to the method's name.

> var obj = {
>   foo() {},
>   bar() {}
> };

is nearly the same as:

> var obj = {
>   foo: function() {},
>   bar: function() {}
> };

a generator method looks like:

> var obj = {
>   *generator() {}
> };


One difference between methods and their ES5 desugaring is that we'd like `method.name` to match the provided name (without bringing it into scope.

> ({ method() {} }).method.name === "method"
> ({ method() { method() } }).method() // ReferenceError "method" is undefined


Relevant grammar:

> MethodDefinition :
>   PropertyName ( StrictFormalParameters ) { FunctionBody }
>   GeneratorMethod
>   get PropertyName ( ) { FunctionBody }
>   set PropertyName ( PropertySetParameterList ) { FunctionBody }
>
> GeneratorMethod :
>   * PropertyName ( StrictFormalParameters ) { FunctionBody }

(getters and setters have also been moved to live under MethodDefinition)


See ES6 draft spec (September 2013 revision) section 14.3.
ObjectLiteral, not ObjectExpression.
Assignee: nobody → gupta.rajagopal
Status: NEW → ASSIGNED
The basic functionality works fine.

Computed property names + method definitions, though, don't work entirely as expected.

I'm unable to do SetFunctionName on a function when the name is a computed property name. The parameter to functionDef in the parser is a HandlePropertyName. This is stored in JSFunction as a HeapPtrAtom. I don't believe this is touched in BytecodeEmitter. We'd ideally want to transmit the computed property name as a parse node to the BytecodeEmitter, and then explicitly set the name property. I'm not sure if this is the right approach. Any hints will be appreciated.

Also, do I need more tests?
Attachment #8468125 - Flags: feedback?(jorendorff)
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
(In reply to guptha from comment #2)
> I'm unable to do SetFunctionName on a function when the name is a computed
> property name. The parameter to functionDef in the parser is a
> HandlePropertyName. This is stored in JSFunction as a HeapPtrAtom. I don't
> believe this is touched in BytecodeEmitter.

By the way, the difference between js::PropertyName and JSAtom is explained in the comment on class PropertyName in vm/String.h.

> [...] We'd ideally want to transmit
> the computed property name as a parse node to the BytecodeEmitter, and then
> explicitly set the name property.

Yes!

We'll have to name the function at run time. (14.3.9 figures out the property key as part of step 1, and it directly calls SetFunctionName in step 3. It all has to happen at run time; compile time is too early; we don't know what the property key will be yet.)

It seems like setting the name might require a new bytecode. I think I'd be OK with cheating a little and leaving the name undefined for now, and filing a followup bug to fix it.

Reviewing your patch now.
Just fixed the TODO - renaming a function name
Attachment #8468125 - Attachment is obsolete: true
Attachment #8468125 - Flags: feedback?(jorendorff)
Attachment #8471703 - Flags: review?(jorendorff)
Comment on attachment 8468125 [details] [diff] [review]
Patch to implement method definitions v0

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

Here are some comments I had on the old version of this patch. Not sure if they still apply.

::: js/src/frontend/Parser.cpp
@@ +7353,5 @@
>            }
>  
>            default:
>              report(ParseError, false, null(), JSMSG_BAD_PROP_ID);
>              return null();

This code contains a pre-existing bug!

If the tokenizer has already reported an error, this report() clobbers that error. To fix this, please add another case at the top of this switch statement:

>          case TOK_ERROR:
>            return null();

@@ +7402,5 @@
>                  if (!handler.addPropertyDefinition(literal, propname, ident, true))
>                      return null();
> +            } else if (tt == TOK_LP) {
> +                tokenStream.ungetToken();
> +                if (!createAndAddFunctionAsProperty(literal, propname, Normal, Method,

Could createAndAddFunctionAsProperty be called methodDefinition() to match the ES6 construction?

@@ +7403,5 @@
>                      return null();
> +            } else if (tt == TOK_LP) {
> +                tokenStream.ungetToken();
> +                if (!createAndAddFunctionAsProperty(literal, propname, Normal, Method,
> +                                                    NotGenerator, op)) {

isGenerator ? StarGenerator : NotGenerator
perhaps?
(In reply to Jason Orendorff [:jorendorff] from comment #5)
> Could createAndAddFunctionAsProperty be called methodDefinition() to match
> the ES6 construction?

No, I use it for getter/setter logic too. It'd be kinda weird.

> isGenerator ? StarGenerator : NotGenerator
> perhaps?

I messed up around here. There's some weird syntax involving * that gets allowed. I will change stuff and add tests.
Changed logic for generators so that illegal syntax does not get through. Sorry about this - I should have caught it before.
Attachment #8471703 - Attachment is obsolete: true
Attachment #8471703 - Flags: review?(jorendorff)
Attachment #8472713 - Flags: review?(jorendorff)
(In reply to guptha from comment #6)
> (In reply to Jason Orendorff [:jorendorff] from comment #5)
> > Could createAndAddFunctionAsProperty be called methodDefinition() to match
> > the ES6 construction?
> 
> No, I use it for getter/setter logic too. It'd be kinda weird.

Getters and setters are also MethodDefinitions in the ES6 grammar:
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-method-definitions

More importantly, the functions created when you evaluate a getter or setter are in fact methods (which means that super works in them):
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-method-definitions-runtime-semantics-propertydefinitionevaluation

I still think methodDefinition is a better name.
Comment on attachment 8472713 [details] [diff] [review]
Patch to implement method definitions v2

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

Looks good! r=me with these changes.

::: js/src/frontend/FullParseHandler.h
@@ +298,5 @@
>          literal->append(propdef);
>          return true;
>      }
>  
> +    bool addFunctionPropertyDefinition(ParseNode *literal, ParseNode *name, ParseNode *fn, JSOp op)

It seems like this should use the word "Method" rather than "FunctionProperty" as well.

::: js/src/frontend/Parser.cpp
@@ +7507,5 @@
> +    if (!fn)
> +        return false;
> +    if (!handler.addFunctionPropertyDefinition(literal, propname, fn, op))
> +        return false;
> +    return true;

Simplify to:
    return handler.addMethodDefinition(...);

::: js/src/tests/ecma_6/Class/methDefn.js
@@ +1,2 @@
> +var BUGNUMBER = 924672;
> +var summary = 'Method Definitions'

This is a great example of something that could be broken into several test files.

Please also test that |this| is correctly set when a method is called, including if the method is inherited via the prototype chain or called using obj.method.call(otherValue).

Assuming your other patch lands first, test that a method can overwrite an earlier property definition in the same object literal, and vice versa.

@@ +29,5 @@
> +syntaxError("b = {a){}");
> +syntaxError("b = {a}}");
> +syntaxError("b = {a{}}");
> +syntaxError("b = {a({}}");
> +syntaxError("b = {a@(){}}");

also {a() => 0}.

@@ +58,5 @@
> +syntaxError("b = {*set a(c){}}");
> +syntaxError("b = {set *a(c){}}");
> +syntaxError("b = {set a*(c){}}");
> +syntaxError("b = {*a : 1}");
> +syntaxError("b = {a* : 1}");

also {a*(){}}.
Attachment #8472713 - Flags: review?(jorendorff) → review+
Addressed review comments.
Attachment #8472713 - Attachment is obsolete: true
Attachment #8473972 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/1369bf46b89f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Depends on: 1054835
Depends on: 1076283
No longer depends on: 1076283
You need to log in before you can comment on or make changes to this bug.