Inconsistent AST about declare keyword

[mysticatea]

What version of TypeScript are you using?

• 3.1.6

What version of typescript-estree are you using?

• 2.1.0 (typescript-eslint-parser#master)

What code were you trying to parse?

declare function f1(a: number): number
function f2(a: number): number
declare namespace n1 {}
namespace n2 {}
declare class C1 {}
class C2 {}

declare namespace N3 {
    declare function f1(a: number): number
    function f2(a: number): number
}

What did you expect to happen?

I can get consistent AST for semantics about whether the declare keyword exists or not.
For example, consistently declare:boolean property or consistently different AST type.

What actually happened?

I got:

{
  "type": "Program",
  "body": [
    {
      "type": "TSEmptyBodyDeclareFunction",
      "id": {
        "type": "Identifier",
        "name": "f1"
      },
      "generator": false,
      "expression": false,
      "async": false,
      "params": [
        {
          "type": "Identifier",
          "name": "a",
          "typeAnnotation": {
            "type": "TSTypeAnnotation",
            "typeAnnotation": {
              "type": "TSNumberKeyword"
            }
          }
        }
      ],
      "body": null,
      "returnType": {
        "type": "TSTypeAnnotation",
        "typeAnnotation": {
          "type": "TSNumberKeyword"
        }
      }
    },
    {
      "type": "TSEmptyBodyFunctionDeclaration",
      "id": {
        "type": "Identifier",
        "name": "f2"
      },
      "generator": false,
      "expression": false,
      "async": false,
      "params": [
        {
          "type": "Identifier",
          "name": "a",
          "typeAnnotation": {
            "type": "TSTypeAnnotation",
            "typeAnnotation": {
              "type": "TSNumberKeyword"
            }
          }
        }
      ],
      "body": null,
      "returnType": {
        "type": "TSTypeAnnotation",
        "typeAnnotation": {
          "type": "TSNumberKeyword"
        }
      }
    },
    {
      "type": "TSModuleDeclaration",
      "id": {
        "type": "Identifier",
        "name": "n1"
      },
      "body": {
        "type": "TSModuleBlock",
        "body": []
      },
      "declare": true
    },
    {
      "type": "TSModuleDeclaration",
      "id": {
        "type": "Identifier",
        "name": "n2"
      },
      "body": {
        "type": "TSModuleBlock",
        "body": []
      }
    },
    {
      "type": "ClassDeclaration",
      "id": {
        "type": "Identifier",
        "name": "C1"
      },
      "body": {
        "type": "ClassBody",
        "body": []
      },
      "superClass": null
    },
    {
      "type": "ClassDeclaration",
      "id": {
        "type": "Identifier",
        "name": "C2"
      },
      "body": {
        "type": "ClassBody",
        "body": []
      },
      "superClass": null
    },
    {
      "type": "TSInterfaceDeclaration",
      "abstract": false,
      "body": {
        "type": "TSInterfaceBody",
        "body": []
      },
      "id": {
        "type": "Identifier",
        "name": "I1"
      },
      "heritage": []
    },
    {
      "type": "TSInterfaceDeclaration",
      "abstract": false,
      "body": {
        "type": "TSInterfaceBody",
        "body": []
      },
      "id": {
        "type": "Identifier",
        "name": "I2"
      },
      "heritage": []
    },
    {
      "type": "TSEnumDeclaration",
      "id": {
        "type": "Identifier",
        "name": "E1"
      },
      "members": [],
      "declare": true
    },
    {
      "type": "TSEnumDeclaration",
      "id": {
        "type": "Identifier",
        "name": "E2"
      },
      "members": []
    },
    {
      "type": "TSModuleDeclaration",
      "id": {
        "type": "Identifier",
        "name": "N3"
      },
      "body": {
        "type": "TSModuleBlock",
        "body": [
          {
            "type": "TSEmptyBodyDeclareFunction",
            "id": {
              "type": "Identifier",
              "name": "f1"
            },
            "generator": false,
            "expression": false,
            "async": false,
            "params": [
              {
                "type": "Identifier",
                "name": "a",
                "typeAnnotation": {
                  "type": "TSTypeAnnotation",
                  "typeAnnotation": {
                    "type": "TSNumberKeyword"
                  }
                }
              }
            ],
            "body": null,
            "returnType": {
              "type": "TSTypeAnnotation",
              "typeAnnotation": {
                "type": "TSNumberKeyword"
              }
            }
          },
          {
            "type": "TSEmptyBodyFunctionDeclaration",
            "id": {
              "type": "Identifier",
              "name": "f2"
            },
            "generator": false,
            "expression": false,
            "async": false,
            "params": [
              {
                "type": "Identifier",
                "name": "a",
                "typeAnnotation": {
                  "type": "TSTypeAnnotation",
                  "typeAnnotation": {
                    "type": "TSNumberKeyword"
                  }
                }
              }
            ],
            "body": null,
            "returnType": {
              "type": "TSTypeAnnotation",
              "typeAnnotation": {
                "type": "TSNumberKeyword"
              }
            }
          }
        ]
      },
      "declare": true
    }
  ],
  "sourceType": "script"
}

In short:

• On functions, the declare keyword changes AST node types: DeclareFunction or FunctionDeclaration
• On classes, the declare keyword does not change AST. I cannot distinguish it.
• On interfaces, the declare keyword does not change AST. I cannot distinguish it.
• On enums, the declare keyword changes node.declare property.
• On namespaces, the declare keyword changes node.declare property.
  • In namespaces which have the declare keyword, in semantics, I think that the declarations in the namespaces are same as having the declare keyword, but...

Anyway, I want a consistent behavior around this.

[j-f1]

See also eslint/typescript-eslint-parser#384

[mysticatea]

TSAmbientDeclaration may be nice.

interface TSAmbientDeclaration extends Statement {
    declaration:
        | VariableDeclaration
        | FunctionDeclaration
        | TSInterfaceDeclaration
        | TSEnumDeclaration
        | TSModuleDeclaration
}

All declaration in this node, contains declarations in module blocks recursively, are handled as ambient declarations.

This is similar to Export*Declaration in ESTree, TSAmbientDeclaration will be exported from this file.

(EDIT: I have less understanding about the ambient declaration of TypeScript, I may say strange things...)

[armano2]

I like this idea.

---

In babel-typescript types: TSInterfaceDeclaration, TSTypeAliasDeclaration, TSEnumDeclaration, TSModuleDeclaration, VariableDeclaration, FunctionDeclaration has additional field declare field

We have few types like TSDeclareMethod, TSDeclareFunction, VariableDeclaration -> VariableDeclarator and so one

this is a mess, we should decide how we are going to do it, we can follow babel or @mysticatea proposal but it has to be consistent

[armano2]

@JamesHenry can you take a look on this?

[JamesHenry]

I won’t have time over the next week as I’m travelling a lot. Definitely open to PRs if anyone else gets chance, otherwise I may have some time the week after.

My gut reaction is: I prefer @MysticTea’s approach but that initially alignment with Babel might be preferable. We can then consider if it’s worth updating both tools.

WDYT?

[armano2]

i can make those changes, but i don't know which one to go for... :>

[JamesHenry]

Let’s go for matching Babel unless there is a strong reason not to as a general rule. It’s why I spent the time setting up the testing for it. The Babel support was built by the TS Team themselves and they have always been open to changes where they make sense, so if we feel strongly the AST design is not optimal we can propose a change to them and carry it out in both tools later

[armano2]

@mysticatea any objections?

[mysticatea]

I follow the @JamesHenry decision because this is your product 😄 .

For what it's worth, as I said on #38 (comment), JS and TS have very different mental model about AST. I was warried that the mix of the two in typescript-estree might cause confusion.

[armano2]

@mysticatea i did most of changes for delcare, but i'm unsure what i should do with

type Foo = string

right now it's

{
  "body": [
    {
      "declarations": [
        {
          "id": {
            "loc": {
              "end": {
                "column": 16,
                "line": 1
              },
              "start": {
                "column": 13,
                "line": 1
              }
            },
            "name": "Foo",
            "range": [
              13,
              16
            ],
            "type": "Identifier"
          },
          "init": {
            "loc": {
              "end": {
                "column": 25,
                "line": 1
              },
              "start": {
                "column": 19,
                "line": 1
              }
            },
            "range": [
              19,
              25
            ],
            "type": "TSStringKeyword"
          },
          "loc": {
            "end": {
              "column": 25,
              "line": 1
            },
            "start": {
              "column": 13,
              "line": 1
            }
          },
          "range": [
            13,
            25
          ],
          "type": "VariableDeclarator"
        }
      ],
      "kind": "type",
      "loc": {
        "end": {
          "column": 25,
          "line": 1
        },
        "start": {
          "column": 0,
          "line": 1
        }
      },
      "range": [
        0,
        25
      ],
      "type": "VariableDeclaration"
    }
  ],
  "sourceType": "script",
  "type": "Program"
}

but babel defines is at new node TSTypeAliasDeclaration

[armano2]

There is also this DeclareFunction - function without body
but babel defines is as TSDeclareFunction

[mysticatea]

I remember eslint/typescript-eslint-parser#414

[armano2]

ok, i will take a look on this to than :>

[armano2]

@JamesHenry this issue got solved in #42