Allow null for FunctionDeclaration and ClassDeclaration id

[nzakas]

FunctionDeclaration and ClassDeclaration don't allow null in its id property.

• https://github.com/estree/estree/blob/master/spec.md#functiondeclaration
• https://github.com/estree/estree/blob/master/es6.md#classdeclaration

However, both can be default exports without an ID.

espree.parse(
  "export default function() {}",
  {ecmaFeatures: {modules: true}}
);
console.log(JSON.stringify(ast, null, 4));

{
    "type": "Program",
    "body": [
        {
            "type": "ExportDefaultDeclaration",
            "declaration": {
                "type": "FunctionDeclaration",
                "id": null,
                "params": [],
                "body": {
                    "type": "BlockStatement",
                    "body": []
                },
                "generator": false,
                "expression": false
            }
        }
    ],
    "sourceType": "module"
}

I believe Acorn and Esprima produce the same.

I think this was an oversight and the spec needs updating.

[jmm]

That's interesting. Acorn / Babylon currently parse that as FunctionExpression, which seems to be an error.

[RReverser]

No, it was intended decision (and there were debates on this) because allowing null as id in FunctionDeclaration nodes breaks backward compatibility for tools that rely on safely using id as non-null Identifier. Also, it's easier to distinguish export default declarations that can be hoisted from those that can't by simply checking whether it's expression (that is, in export default function f() function can be simply moved out, while in export default 42 or export default function () or export default () => ... it can't).

[jmm]

@RReverser Interesting. (For anyone arriving at this issue, I found some of that previous discussion in #38, jquery/esprima#311, acornjs/acorn#208.)

[nzakas]

That's interesting, I hadn't thought about hoistable vs. not. Still, it seems that something has to change here, either Acorn or the spec + Esprima/Espree because right now, this is an incompatibility that isn't allowed by the spec: https://github.com/estree/estree/blob/master/es6.md#exportdefaultdeclaration

@jeffmo?

[jeffmo]

@RReverser mentions a debate -- which I think he and I were on opposite sides of :)

Ultimately I don't remember whether we all gave up talking about it (and thus status-quo remained) or if I just gave up and went with the wind, heh. But I mostly agree with the premise of the original post -- I wish 'id' could just be null and for estree to match the changing reality of the actual ES spec.

A good chunk of the original discussion can be found in the second half of this thread if you want to read through it: jquery/esprima#311

[michaelficarra]

Using a FunctionExpression here causes an ambiguity between NFEs and FDs. I just tried using babel to compile

export default (function a(){});

and sure enough, it exposes a to the surrounding scope. I'll report the bug over there. This should be clear evidence that FunctionDeclaration or some other custom node is the appropriate choice.

[sebmck]

The Babel bug is irrelevant and just the result of some sloppy parsing.

FWIW I've switched my opinion on this, I didn't think the hoisting mattered which is why I was in favour of reusing a FunctionExpression but it's important for circular dependencies.

On Sun, Aug 2, 2015 at 4:54 PM, Michael Ficarra notifications@github.com
wrote:

Using a FunctionExpression here causes an ambiguity between NFEs and FDs. I just tried using babel to compile

export default (function a(){});

and sure enough, it exposes a to the surrounding scope. I'll report the bug over there. This should be clear evidence that FunctionDeclaration or some other custom node is the appropriate choice.

Reply to this email directly or view it on GitHub:
#98 (comment)

[michaelficarra]

@sebmck then how should that be parsed?

[sebmck]

@michaelficarra Didn't mean to imply that the bug was incorrect just that a nullable id would not have prevented this. Here are the lines that cause this. The only relevant line is this one which would have still caused the bug to be present since it's not checking the starting token.

[nzakas]

Ping @caridy

I can see arguments both ways, I'd just like us all to agree. The thing I keep coming back to I'd the difference between these:

export default function() {}

// and

export default (function() {});

According to the spec, these are not the same (HoistableDeclaration vs. AssignmentExpression).

[RReverser]

That's true (unfortunately), they're not precisely the same.

I don't have that strong opinion on any resolution of this (this was anyway too long debate :) ), it's just that breaking backwards compatibility on such a low level as declaration feels wrong to me, as before this we had that any declaration node is not just hoistable, but well, it declares something in scope, and tools could safely rely that id is always present and always an Identifier. Alternatively, this could be solved by creating separate node types (like we discussed earlier), but that probably looks even uglier.

I hope we could find some way of representing those without introducing incompatibilities or unnecessary node types, but if not, I'm happy to reflect any changes to spec.

[sebmck]

any declaration node is not just hoistable

It wont effect whether a local binding is hoisted (because there is none), but it will effect circular dependencies as the export should be hoisted.

[RReverser]

@sebmck Yes - that's what I mean. Basically export default itself becomes subsequently hoisted if it's declaration node is hoistable.

[caridy]

I didn't think the hoisting mattered which is why I was in favour of reusing a FunctionExpression but it's important for circular dependencies.

This is the key here. It does matter when circular dependencies are suppose to work due to the hoistable nature of those declarations.

I recall the discussion vividly, and @jeffmo and I spoke to Allen about this particular issue, and whether or not we will have similar hoistable declarations in the future, and the answer was yes! I will be ok with custom nodes (e.g.: HoistableDeclaration[Default] and ClassDeclaration[Default]), as stated in the spec: http://www.ecma-international.org/ecma-262/6.0/index.html#sec-exports

[caridy]

Btw, in the implementation in espree and esprima, I was using identifierIsOptional flag when calling the factory method for class and function declarations to support this use case where id could be null.

[gibson042]

How would changing the definition of FunctionDeclaration#id to include null break backwards compatibility any more than e.g. computed Property keys, which are also not valid ES5? In other words, given that both spec changes are in effect an expansion of preexisting AST node properties, why would one necessitate a new type but the other not? Especially when existing tools are already outputting id: null, seemingly without problems?

Also, for my own edification and future reference, could someone provide a concrete example of where FunctionDeclaration vs. FunctionExpression in the AST for anonymous default exports makes a difference?

[sebmck]

@gibson042

How would changing the definition of FunctionDeclaration#id to include null break backwards compatibility any more than e.g. computed Property keys, which are also not valid ES5?

if (node.type === "FunctionDeclaration") {
  console.log("I'm a function with the name", node.id.name);
}

now throws a TypeError. This breaks a lot of other assumptions existing code makes.

Also, for my own edification and future reference, could someone provide a concrete example of where FunctionDeclaration vs. FunctionExpression in the AST for anonymous default exports makes a difference?

a.js

import "./b";

export default function () {
  console.log("i'm a little teapot");
}

b.js

import a from "./a";

a();

The export default should be hoisted. If it's an expression it shouldn't.

[gibson042]

if (node.type === "FunctionDeclaration") {
  console.log("I'm a function with the name", node.id.name);
}

I don't think that quite cuts it, because a) AFAIK, there have been no reports to Esprima/Espree for already returning id: null; and b) the same style of incompatibility (albeit non-fatal) would manifest from the already-extended Property#key:

if (node.type === "Property") {
  var key = node.key.name || node.key.value; // undefined for most computed keys
  console.log("I'm an object property with the name", key);
}

The export default should be hoisted. If it's an expression it shouldn't.

So attempting to execute the expression version (a.js: export default (function(){…})) should generate a runtime exception in b.js, right? And therefore the syntactic difference is meaningful.

[sebmck]

I don't think that quite cuts it

Sure it does, that was just a naive example. Making id nullable for a export default makes 5 Babel tests fail. It's not an uncommon pattern by any means.

And therefore the syntactic difference is meaningful.

Yep. The declaration hoists, the expression doesn't.

[nzakas]

We dealt with this in ESLint when we enabled modules support - several rules broke. But in all cases it was an easy fix because we had the exports context to rely on. So, I don't see this breakage as a hurdle if FunctionDeclaration is the correct choice.

[michaelficarra]

Agreed with @gibson042 that this is not the only place where consuming ASTs of the updated format will require non-trivial changes. Computed property names is a perfect example.

[RReverser]

But computed properties were described much earlier. Might be harder to make changes at this point when ES6 is finished.

But as I said above - if everyone else thinks it's a good idea, I don't mind and can change Acorn's behavior as soon as spec is updated.

[michaelficarra]

I don't see how there is even a discussion still happening here. Using a FunctionExpression to represent both an expression and a hoistable declaration in export default position just does not work. Like the problem with representing directives using string literals, you cannot allow a single tree to represent more than one program. This change or one that is equally powerful needs to be made.

[RReverser]

or one that is equally powerful needs to be made

That's exactly an answer to

I don't see how there is even a discussion still happening here

:)

[michaelficarra]

Alright, as long as we're agreed that the implementation in acorn is an insufficient representation and the spec as of now doesn't permit one that is sufficient.

[dead-claudia]

If I understand the spec correctly (the ES2015 spec, that is), would it be better to make the same condition that's in the ES spec that it can only be null within default export statements? That could be sufficient, and easy to evaluate.

[RReverser]

@IMPinball Unfortunately, that's not possible within out DSL.
Damn, let's just do it and make it nullable, this is open for too long :)

[sebmck]

@RReverser Sure it's possible.

interface NullableFunctionDeclaration <: FunctionDeclaration {
    id: Identifier | null;
}

interface NullableClassDeclaration <: ClassDeclaration {
    id: Identifier | null;
}

interface ExportDefaultDeclaration <: ModuleDeclaration {
    type: "ExportDefaultDeclaration";
    declaration: NullableFunctionDeclaration | NullableClassDeclaration | VariableDeclaration | Expression;
}

[mikesherov]

I prefer sebmck's version. Creating an intermediate interface for < 3 cases seems over-abstracted. LGTM on @sebmck's proposal.

[caridy]

+1 on @sebmck proposal as well.

[nzakas]

Then I'd suggest changing the names. Nullable is inaccurate because only the ID is nullable, not the function. I think @RReverser 's names are more accurate.

[RReverser]

@sebmck Do you want to submit PR with updated names?

[mikesherov]

@RReverser do you know where this ended up?

[hzoo]

#98 (comment)

I believe we went with a variation of sebmck proposal + some changed names. (Basically that export default function() {} would be a FunctionDeclaration with id null?)

so

interface OptFunctionDeclaration <: FunctionDeclaration {
    id: Identifier | null;
}

interface OptClassDeclaration <: ClassDeclaration {
    id: Identifier | null;
}

interface ExportDefaultDeclaration <: ModuleDeclaration {
    type: "ExportDefaultDeclaration";
    declaration: OptFunctionDeclaration | OptClassDeclaration | VariableDeclaration | Expression;
}

[mikesherov]

@RReverser @ariya what does Esprima and Acorn do here?

[hzoo]

Esprima, Espree, Babylon, Flow all do FunctionDeclaration with id null and Acorn does FunctionExpression. Actually just hit this when looking at testing webpack with babylon

[mikesherov]

Ok, let's PR this as originally described by sebmck and file an acorn bug @hzoo

[hzoo]

Acorn has done this has part of 5.0 a while ago acornjs/acorn#512

[mikesherov]

Ok @hzoo is there a PR for this to land in ESTree?

[adrianheine]

I opened a pull request for this: #174