Add representation for meta properties #32
Conversation
|
The |
|
|
|
Inheriting from I agree with @michaelficarra here. I don't really think an ambiguous name ( |
|
I agree with @michaelficarra on all points here. |
Building off that mindset, I don't think that What about |
interface MetaProperty <: Expression {
type: "MetaProperty";
kind: "new.target";
}
|
Is that entirely true? The grammar says: I'm unable to validate at the moment if that means that If it turns out that only To match the terminology used in the spec, rather than the |
|
They are separate tokens, so whitespace is indeed allowed. But that has no bearing on whether we would represent the MetaProperty using two values or one. If we want to start storing concrete syntax, we could simply store each concrete syntax element in separate named fields for the four possible positions whitespace is allowed. We would also need to store the escape sequences used to represent the MetaProperty. Anyway, I don't think we should be concerning ourselves with those things right now, since we currently have no plan to add concrete syntax info to the AST. |
I object to making any "final decisions" on any of the stuff here that then paints us into an awkward corner when (later) trying to even decide if and how we might approach concrete syntax. No, we haven't officially decided to doing so or not, but we also shouldn't design as if there's no knowledge that such a thing might be a concern in the near future. In the same way that we can say "you know, Does it really gain us that much to collapse to |
|
Since the spec appears to simply whitelist it, and further discussion in ES Now, as for "newTarget" being the collapsed form, I find it easier to // my proposal
// "new.target"
// -> keyword = "new"
// -> property = "target"
let [keyword, property] = node.kind.split(".");
// current idea
// "newTarget"
// -> keyword = "new"
// -> property = "target"
let keyword, property;
{
let {kind} = node;
let re = /[A-Z]/;
re.test(kind);
let index = re.lastIndex;
keyword = kind.slice(0, index);
property = kind[index].toLowerCase() +
kind.slice(index + 1);
}I don't see much justification for not collapsing the properties,
|
|
@IMPinball You shouldn't have to resort to hacks like that to get the keyword and property. |
|
Actually, I take back my entire support for collapsing the node, since it would better support position information interesting for refactoring tools. Also, the spec appears to allow syntactically insignificant whitespace on either side of the dot, as in traditional member accessors. It would still be easy to check if left as explicitly extending MemberExpression with no extra properties and some prose to explain its purpose separate from MemberExpression. function isMetaProperty(node, type) {
if (node.type !== "MetaProperty") {
return false;
}
if (type == null) {
return true;
}
let [keyword, property] = type.split(".");
return node.object.name === keyword &&
node.property.name === property;
}
// ...
if (isMetaProperty(node)) {
// it's a meta property
}
if (isMetaProperty(node, "new.target")) {
// it's new.target specifically
} |
👍 |
|
Clarification (I forgot): all of these are equivalent according to the new.target
////////////////////////
new.
target
////////////////////////
new
.targetSorry about it.
|
|
So, to get conclusion on this... I agree with @IMPinball and understand concerns about inheritance from How about following? interface MetaProperty <: Expression {
type: "MetaProperty";
keyword: Identifier;
property: Identifier;
} |
|
LGTM
|
|
Are we (reasonably) certain that all future meta properties are going to be keyword.property? So far, Is it reasonably possible that in the future there's a metaproperty which is not based on a keyword? For example, words like Also, the module export mechanism apparently supports a special token of Just don't want us to regret a name like "keyword" sometime down the road. |
|
Further info on potential future metaproperties: https://github.com/allenwb/ESideas/blob/master/ES7MetaProps.md Looks like all those so far hang off |
|
More info: https://esdiscuss.org/topic/function-name-property That thread starts out talking about Some observations:
|
|
@getify Good catch. Maybe we should come up with some better representation/node name instead of Also, am I the only one wondering about the merge conflicts with this PR? |
|
i'd be ok with "host". I suggested "context" earlier in the thread, too. |
|
After our weekly esprima meeting, it seems most of us are onboard with the structure that @RReverser suggested above, except that So an example of the structure that at least everyone in that meeting was ok with was: The benefits of this structure are (a) it allows things like esprima/acorn/etc to extend the structure and include location info alongside the 'new' and 'target' tokens (since it's possible to have whitespace between them) in a consistent way with how it includes location data for other nodes (b) it makes the overall structure immediately distinct from MemberExpression. |
|
Well, we already have a precedent of using e.g. Also, in: using |
Works for me. I'm not personally tied to any of those names, but I think @michaelficarra found them appealing. I'm mostly interested in the structure (I'd really like for it to be possible to extract location info for |
|
@michaelficarra sometimes tries to patch ESTree to look it more like Shift :P just saying |
|
But yeah, as for structure I'm totally with you and I actually like |
|
|
I had actually forgotten that 👍 @mikesherov. |
|
👍 |
|
@RReverser Abstract it away into helpers then, it's painful to do any AST introspection without them. The Babel equivalent would be |
|
Maybe |
|
Well yeah, utils help a lot :) |
@sebmck: That is in precise contradiction to @RReverser's #32 (comment) :
And that seems to be the real root of disagreement here. |
Why do you think it's a contradiction? |
|
Because one view considers only syntax in designing the AST structure, while the other also factors in semantics. |
|
@gibson042 I never said that semantics don't matter. |
|
Guys, let's not devolve here. The theoretical arguments about syntax vs. semantics can go on for days if we don't curb it, and are completely trumped by how people are actually using the AST. The only argument that is currently contentious at the moment (because we've exhausted the rest), is the one about visitor pattern / inspection. |
|
Here's a real life scenario where splitting up all the meta properties into separate nodes is a terrible idea, at least for this specific scenario. I have a loop with a let variable: function foo() {
for (let num of nums) {}
}It has a let variable. My block scoping transformer just changes that function foo() {
for (var num of nums) {}
}But now I add a reference inside a closure to my loop body: function foo() {
for (let num of nums) {
bar(function () { num; });
}
}Oh no. Now I need to ensure that function foo() {
for (var num of nums) {
(function (num) {
bar(function () { num; });
})(num);
}
}Oh wait, fuck. That means that if someone does: function foo() {
for (let num of nums) {
bar(function () { num; });
function.count;
}
}It'll produce: function foo() {
for (var num of nums) {
(function (num) {
bar(function () { num; });
function.count;
})(num);
}
}Shit. It references the wrong function. Looks like I'll have to memoise those references to before the closure to something like: function foo() {
for (var num of nums) {
var _function$count = function.count;;
(function (num) {
bar(function () { num; });
_function$count;
})(num);
}
}Perfect! Now how would I catch all the With node types for each meta property I need to go in and special case every single one. traverse(node, {
enter(node) {
if (node.type === "FunctionCountMeta" || node.type === ...) {}
}
});With a single node: traverse(node, {
MetaProperty(node) {
if (node.meta.name === "function") {}
}
});Not only does that simplify the visitor logic but it's future proof against new function meta properties. |
|
Also having separate nodes for each meta property is crap for code generation: export function FunctionCountMeta() {
this.push("function.count");
}
export function FunctionCalleeMeta() {
this.push("function.callee");
}
...vs: export function MetaProperty(node, print) {
print(node.meta);
this.push(".");
print(node.property);
}Hell, you could just share the same code generation method as |
|
Also, important to note that ESTree needs to balance the needs of all consumers. A linter doesn't give a shit about the difference between |
|
Also another important thing to note is that you can do: new.
targetCombing both of those distinct properties |
|
Apologies for the long narratives but I'm pretty sure all the issues I raised put the nail in the coffin about distinct node types, happy to be proven wrong. |
|
👍 sounds reasonable to me, let's just leave it as is and stop bike-shedding |
|
Tools aware of the type hierarchy can still do all that logic you just described for Lack of subtypes, on the other hand, could complicate visitor code if meta properties start being used for very different purposes— P.S. Thank you very much for the code examples. They help in clarifying issues more than anything else. P.P.S.
I also like such reuse and want to enable more of it, but note that MetaProperty as committed describes the syntax like |
That's the key. Hierarchy is needed for very few tools, and we shouldn't force all the other tools to complicate their codebase by either checking even more different types or introducing hierarchy. |
Right, in the case I must have misinterpreted what was being proposed. If anything that makes it worse since then you have two pieces of linking information that can go out of sync. It's been said before that changes/additions that duplicate metadata and information wont be accepted due to how dangerous they are. |
An excellent point. I'm sold. |
|
I just wanted to say, now, that this thread is concluded, that having discussion until everyone is satisfied is important, and a different activity then getting a thread to the point of mergeability. While this was a long and arduous thread, the fact that we stuck around and talked it through despite getting heated trumps any frustration any of us experienced. All members should keep this in mind in further discussions: aside from suggestions for throwing out the AST completely, or seriously breaking es5 BC, we must be willing to fully discuss the points here. |
|
So, what the current status? |
|
@monolithed Well it was merged? |
|
@sebmck, I don't think so :) |
|
@monolithed No I think so. I was the one who merged it :) The merge commit is 3560243
|
|
@sebmck, I got it, could you please explain what are we waiting for babel/babel#1088? |
|
@monolithed |
|
@sebmck, thanks! |
ECMAScript gets new syntax thing call "meta properties", which is already used as
new.targetand, according to meeting notes, can be used in future in other places as well.For those cases, we need a new node that (up to discussion) derives from
MemberExpressionbut is restricted to havingIdentifierin bothobject(as it's actually a keyword used asIdentifier) andpropertyand can't becomputed(i.e.new['target']is syntax error).