Brandon Benvie [:benvie] Reporter
	Description • 11 years ago

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.

	Brandon Benvie [:benvie] Reporter
	Comment 1 • 11 years ago

ObjectLiteral, not ObjectExpression.

	guptha Assignee
	Comment 2 • 10 years ago

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?

	Jason Orendorff [:jorendorff]
	Comment 3 • 10 years ago

(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.

	guptha Assignee
	Comment 4 • 10 years ago

Just fixed the TODO - renaming a function name

	Jason Orendorff [:jorendorff]
	Comment 5 • 10 years ago

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?

	guptha Assignee
	Comment 6 • 10 years ago

(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.

	guptha Assignee
	Comment 7 • 10 years ago

Changed logic for generators so that illegal syntax does not get through. Sorry about this - I should have caught it before.

	Jason Orendorff [:jorendorff]
	Comment 8 • 10 years ago

(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.

	Jason Orendorff [:jorendorff]
	Comment 9 • 10 years ago

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*(){}}.

	guptha Assignee
	Comment 10 • 10 years ago

Addressed review comments.

	guptha Assignee
	Comment 11 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=fde6a31e046e

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/1369bf46b89f

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/1369bf46b89f

	Florian Scholz (Open Web Docs)
	Comment 14 • 10 years ago

Main doc:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Method_definitions

Linked from:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions#Defining_method_functions
https://developer.mozilla.org/en-US/Firefox/Releases/34#JavaScript

Reviews appreciated.