Concise methods node

[nzakas]

Esprima currently represents concise methods as Property whose value is a FunctionExpression. This is pretty confusing because ES5 style properties with function values are represented the same way. Even though the method flag is true, that still means the FunctionExpression node represents different syntax in each situation, and that can cause errors such as eslint/eslint#1677

[michaelficarra]

But getters and setters are also represented with a FunctionExpression node. It is only appropriate to re-use it yet again for methods.

[nzakas]

Good point, and similar problem. Maybe a new property on FunctionExpression to make this distinction? As an AST consumer, having FunctionExpression representing different syntax depending on the parent makes things pretty confusing.

[michaelficarra]

Tell me about it. But that problem exists all over the SpiderMonkey AST. Identifier and BlockStatement are possibly the worst offenders.

[dead-claudia]

@michaelficarra Identifier, in my experience, is only really for one purpose, to represent an identifier, whether used for an unquoted property name, variable, or function name. In my opinion, it just makes perfect sense. The problem with BlockStatement is that it's somewhat inconsistently used, and it's a little late to change it now, since tooling has largely gotten used to it being stable.

[sebmck]

@IMPinball We have the opportunity to change things like that with ESTree.

[RReverser]

@sebmck We should not change pre-ES6 types cardinally though, as I don't think we want to break all the existing tools.

[sebmck]

@RReverser Sure, I agree, I just don't think these types of things should be taken off the table for consideration.

[michaelficarra]

@sebmck: That's a slippery slope. If you're going to make pre-ES6 breaking changes, there's not much argument against just adopting Shift. We shouldn't be considering breaking changes like those in this repo.

[dead-claudia]

I like the original proposal in this bug, a new boolean .method property for ES6 methods. I highly doubt that someone is already checking method: false everywhere, and in tests, it's relatively simple (albeit repetitive) to fix.

@sebmck I was merely clarifying a couple specific points on why certain nodes were used, and I highly doubt any of that's going to change now (not to mention it is a little unintelligent to do so, since patching all the tools isn't exactly trivial for anything). I actually disagreed with one of them, and the other isn't something easily replaced.

I do find node.body.body[i] a bit annoying, but it's easier to learn to live with it than change hardcoded assumptions likely to be all over the place.

[nzakas]

Adding a new property to FunctionExpression wouldn't break back-compat. IMHO, that's where a new property should be added so we don't always need to look at the parent to figure out what form the function is taking.

[mikesherov]

Additive changes to any of the data structures can not be considered breaking.

@michaelficarra, we have already agreed on a few non-BC changes already, but for the sake of interop.

If we all agree to add a prop to function expression, I don't see how that's a breaking change.

I don't think any discussion should be off the table. We're here to make iterative improvements, and that doesn't mean we have to move to shift to merely discuss them.

[nzakas]

To be more concrete, here's my proposal:

Add a property kind to FunctionExpression that would have the following possible values:

• "method" for concise object literal methods and class methods
• "getter" for getters
• "setter" for setters
• "default" for the traditional (ES5) form of function expression

[ikarienator]

Maybe the kinds should be 'method', 'get', 'set' and 'init' to achieve maximum backward compatibility? There is little value to change 'get' to 'getter' and etc. IMO.

[dead-claudia]

It's easy to get dependent on the number of property types. And "init" is
still somewhat relevant to methods.
On Feb 14, 2015 9:58 PM, "Bei Zhang - Ikarienator" notifications@github.com
wrote:

Maybe the kinds should be 'method', 'get', 'set' and 'init' to achieve
maximum backward compatibility? There is little value to change 'get' to
'getter' and etc. IMO.

—
Reply to this email directly or view it on GitHub
#5 (comment).

[ikarienator]

@IMPinball What do you mean by "get dependent on the number of property types"? Can you elaborate more about it?

[ariya]

Adding a new property will not break any existing tools, and that's quite straightforward since the tools typically ignore any unknown properties. Still, that does not mean we should not come up with a proper justification as to why it is needed. Duplicating kind (init, get, set) of the Property node into kind of the corresponding FunctionExpression needs a stronger argument that just a traversal convenience.

CMIIW @dherman, the SpiderMonkey AST already sets the precedent that a node does not always know what "role" it plays (rightfully so, although @michaelficarra or @ikarienator can say more about how it is same/different in Shift AST). Consider the Identifier node that serves as the property key; it does not convey any information that it is a property key. And even such a kind property exists for FunctionExpression, what do we do for a function expression as the initialization value in a variable declaration? It will bear the kind value that is redundant in that context.

[RReverser]

Agree with @ariya here. Making some node types contextual while entire AST by design is not, doesn't seem to be legit.

[dead-claudia]

@ikarienator Imagine this kind of check:

function handleProperty(node) {
  if (node.kind === 'get') handleGetter(node);
  else if (node.kind = 'set') handleSetter(node);
  else handleInit(node);
}

You don't know if it's going to be an init or method as the kind in the last case. This kind of thing is easy to do in other situations as well, so that's why I'm generally against it. And also, isn't it effectively equivalent to this? Even the this handling is identical.

foo() {}
foo: function () {}

I don't think the method value quite works in this property. I would be a little more supportive of a .method property on the node itself. It would also be easier to check.

[dead-claudia]

Well, by the looks of it, it may be best to add a .method boolean property, according to this comment by @RReverser. (Correct me if I'm wrong)

[RReverser]

@IMPinball I don't see how my comment proves that it's best to add extra boolean property on function itself; as I wrote earlier, I'm actually against it.

[dead-claudia]

@RReverser Okay. Then I take that back, since I really misinterpreted it.

[nzakas]

@ariya Identifier is a bad example because it always has the same syntactic form regardless of where it is used. So it really doesn't matter where I encounter an Identifier because I know it always looks the same. FunctionExpression doesn't always look the same and that's a problem for tools. I mean, I'm assuming that's why we have ArrowFunctionExpression instead of using FunctionExpression to represent arrow functions - the only difference there is syntax but it represents the same thing.

And to be clear, I'm talking about more than traversal here. I'm also talking about consistently representing syntax.

Methods/getters/setters are a different syntactic form but you can't tell that from the node. That seems broken.

[ikarienator]

nit: Identifiers in SM API are used for both Identifier and
IdentifierNames, and there is a clear difference between them. In ES6, it
can be many more things.

That said I totally agree that it's wrong represent getters and setter
using FunctionExpressions. But I think that case you should be more
interested in Shift AST format:)? (Sorry for the advertisement)
On Sun, Feb 15, 2015 at 09:28 Nicholas C. Zakas notifications@github.com
wrote:

@ariya https://github.com/ariya Identifier is a bad example because it
always has the same syntactic form regardless of where it is used. So it
really doesn't matter where I encounter an Identifier because I know it
always looks the same. FunctionExpression doesn't always look the same and
that's a problem for tools. I mean, I'm assuming that's why we have
ArrowFunctionExpression instead of using FunctionExpression to represent
arrow functions - the only difference there is syntax but it represents the
same thing.

And to be clear, I'm talking about more than traversal here. I'm also
talking about consistently representing syntax.

Methods/getters/setters are a different syntactic form but you can't tell
that from the node. That seems broken.

—
Reply to this email directly or view it on GitHub
#5 (comment).

[michaelficarra]

I don't see how changing the getter/setter representation can even be considered at this point, given it is an ES5 feature that all existing tools will rely upon.

[nzakas]

@michaelficarra no one is suggesting changing anything. I'm suggesting adding something. Back compat would not be affected.

[getify]

I don't see how changing the getter/setter representation can even be considered at this point

If we cannot be breaking BC, then it moots your argument of doing something new (methods) "consistently" with the old stuff (getters/setters). That is, I would agree that having one way to distinguish the role/kind would be nice, but since that ship already sailed when getters/setters were represented in a non-generically-extendable way, we already accepted the (future, aka now) cost of other kinds.

I like having a meta flag to indicate this shorthand form. I'm not sure method is the right name, though. What about concise properties? Could we call it concise so it could apply to both?

[RReverser]

concise is more confusive IMO and is clear only for those who know ES spec itself, while method is pretty clear for anybody who is familiar with ES6.

[sebmck]

Can we get some consensus on this?

[getify]

I would still like there to be a consistent way that concise methods and concise properties are indicated as concise. My vote remains concise. If we use method, what then do we use for concise properties?

[sebmck]

@getify Are you referring to object property shorthand? ie. var foo = { bar }; If so, Property already has a shorthand property.

[getify]

I am referring to that, yes, and I'm saying that conceptually these two separate cases seem like the same, and are often documented the same... that is, "concise properties" and "concise methods".

So I think it's at least reasonable to entertain the notion that they should be notated the same... ie, with the word "concise" rather than with "shorthand" for properties and "method" for methods.

[mikesherov]

From the perspective of a style enforcer that must care about concrete form, having a new property (which doesn't break BC) on Function nodes would be hugely beneficial.

@nzakas is correct in describing what a pain it is without this. Once must check to see if the parent is an ObjectExpression, then traverse all properties of that object to see which property is equivalent to that function expression, and then check the kind property of that property node.

For those that implement visitor pattern (ESLint), this is unnecessarily convoluted.

Seems as if the Esprima team is not all on the same page. We'll resolve that tomorrow at our team meeting and report back.

[sebmck]

@mikesherov I'm not against this but you're grossly overestimating how hard it is without. On visit of a FunctionExpression you can easily check if it's a concise method with:

parent.type === "Property" && parent.method && parent.value === node

[mikesherov]

Right. Forgot prop was parent. In that case, seems duplicative.

[nzakas]

Once again I'll point out how silly it is to have to look at a node's parent to determine its syntactic form. This is the only place in ESTree where this happens.

[getify]

how silly it is

yeah, and how silly it is that concise props have the "shorthand" flag but concise methods would not.

[michaelficarra]

@getify: What? What would a shorthand flag mean on concise methods?

[getify]

I'm not sure how I can be any clearer about what I'm suggesting? It seems silly to me that the node for a "concise property" would have a "shorthand" moniker, but the node for a "concise method" wouldn't have a similar flag (if named differently). It also seems silly to me that the flag would be named differently in these two cases.

[mikesherov]

The point @michaelficarra is making that a shorthand literal property doesn't have the shorthand property on the Literal that is the value of the Property, it instead has shorthand on the Property node itself.

In this same manner, the function expression that is the value of the Property doesn't have a shorthand property. It instead lives on the Property node itself.

[michaelficarra]

Not only that, I was pointing out that methods don't have a non-shorthand variant.

[getify]

OK, my attempts at brevity in this thread have apparently significantly misled on my point. Let me try to be more specific.

1. I'm not arguing to add a flag to the parent... I know it exists in both cases. I'm arguing against any suggestion (which perhaps I misunderstood?) of the fact that it could be removed or somehow inferred.

2. I was bikeshedding on the fact that "shorthand" doesn't seem appropriate since the common name for both of these is more widely "concise" than "shorthand", especially in the spec.

3. I'm siding with @nzakas that it's quite unfortunate that we have to look at the parent node for this flag. In the case of the concise property, the value doesn't represent itself differently, but in the case of the concise method, the value does represent itself differently.

4. Following from (3), I was arguing that for consistency and convenience sake, we could also represent this information on the value node (that is, with a "concise" flag), for both the concise property value node and the concise method value node.

   That is, in { a: a } shortened to { a } or { b: function() {} } shortened to { b() {} }, the a: and the b: are ignored/dropped, and the value node in both cases which stays displayed decides if it should represent itself differently.

   So you can make the case that both the parent and the child need to know this information.

5. Following from (4), however, since the property parent node in both cases is hidden, and preserving any information about its display is not relevant, that parent node wouldn't necessarily need a flag to say it was "shorthand", it would just simply not have any identifier info at all, and thus you could infer that it shouldn't be represented in the program.

   Which then makes the case that instead of duplicating "shorthand" or "concise" on both parent and child nodes, we could just move that information from the parent node to the value node, since it's actually the value node which legitimately uses that information (at least in one of the two cases).

[nzakas]

We're getting into way too many tangential topics here.

@getify - I think everyone understands your point fine, it's just that we don't share your opinion. I think it's best that we focus on the original topic so we can finally close this discussion out.

Can we just get a quick +1/-1 check for adding some indicator of concise syntax on a FunctionExpression? If there's general agreement, we then figure out the best property.

[getify]

it's just that we don't share your opinion

fine. if my opinion isn't shared (even though I thought I was backing you up, specifically), then it's also probably not important for a consensus vote check, but I'd be a -1 for adding something in an inconsistent fashion, which it sounds like you'd be headed towards.

[sebmck]

👎 to an additional property on FunctionExpression. The beauty of ESTree is that I can swap nodes in and out without worrying about them containing parent information. ESTree already sets a precedence where nodes aren't aware of the role they play (and this is a good thing sometimes!) so this seems like a slippery slope.

[nzakas]

@sebmck I'm curious, why would you consider this containing parent information? To me, the parent is just the property name whereas FunctionExpression is everything after it (which is how the parsers represent it currently).

[RReverser]

@nzakas Consider code generator that would rely on FunctionExpression.method or similar property to generate (args) { ... } instead of function (args) { ... }.

In that case, you can't just extract function expression from object's property into other place (i.e. Object.defineProperty call or external variable) without manually setting it's method to false, and it's just one property. If we set such a precedent, it might become even more difficult to move nodes around without breaking resulting structure, as you will have to always think about all possible combinations of properties that end up with invalid state when value is moved to different parent.

[nzakas]

@RReverser Interesting, this is where I thought having such a property would help. I see your point. Thanks for explaining.

Okay, I'm convinced it's not useful and potentially hazardous to add a property such as this, so I'm withdrawing the proposal. Thanks for the consideration, all.

[mikesherov]

Thanks for explaining more eloquently than I coul @RReverser. This principle of context free replacement has come up several times. Perhaps we can add this as a guiding principe?

[nzakas]

+1

[RReverser]

Perhaps we can add this as a guiding principe?

Sounds good. However I'll spend next 18 hours in KBP ✈ SFO, so will do it after unless someone gets to this earlier.