no-useless-constructor: Cannot read property 'body' of null

[SebastiaanYN]

This seems to be the same issue as #15.

---

What code were you trying to parse?

// src/foo.ts
class Foo {
    constructor();
    constructor(obj: any);
    constructor(obj?: any) {
        console.log(obj);
  }
}

This also happens with both examples from the other issue,

declare class Foo {
  constructor();
}

and

class Foo {
  constructor();
}

When the ESLint no-useless-constructor rule is disabled, the issue doesn't show.

What did you expect to happen?
ESLint sees the code as correct.

What actually happened?
ESLint gives an error, because the body of some constructors can't be found.

TypeError: Cannot read property 'body' of null
Occurred while linting /home/sebastiaanyn/Projects/Syntek/src/foo.ts:2
    at checkForConstructor (/home/sebastiaanyn/Projects/Syntek/node_modules/eslint/lib/rules/no-useless-constructor.js:169:42)
    at listeners.(anonymous function).forEach.listener (/home/sebastiaanyn/Projects/Syntek/node_modules/eslint/lib/util/safe-emitter.js:45:58)
    at Array.forEach (<anonymous>)
    at Object.emit (/home/sebastiaanyn/Projects/Syntek/node_modules/eslint/lib/util/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (/home/sebastiaanyn/Projects/Syntek/node_modules/eslint/lib/util/node-event-generator.js:251:26)
    at NodeEventGenerator.applySelectors (/home/sebastiaanyn/Projects/Syntek/node_modules/eslint/lib/util/node-event-generator.js:280:22)
    at NodeEventGenerator.enterNode (/home/sebastiaanyn/Projects/Syntek/node_modules/eslint/lib/util/node-event-generator.js:294:14)
    at CodePathAnalyzer.enterNode (/home/sebastiaanyn/Projects/Syntek/node_modules/eslint/lib/code-path-analysis/code-path-analyzer.js:632:23)
    at nodeQueue.forEach.traversalInfo (/home/sebastiaanyn/Projects/Syntek/node_modules/eslint/lib/linter.js:752:32)
    at Array.forEach (<anonymous>)

Versions

package	version
@typescript-eslint/eslint-plugin	1.6.0
@typescript-eslint/parser	1.6.0
TypeScript	3.4.2
ESLint	5.16.0
node	11.8.0
npm	6.9.0

[bradzacher]

Have you tried using our rule instead? @typescript-eslint/no-useless-constructor?

https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-useless-constructor.md

[SebastiaanYN]

No error occurs with the @typescript-eslint/no-useless-constructor rule.

It would however be nice to be able to use the no-useless-constructor rule. I use the airbnb eslint rules and it would be nicer to just keep the default rules instead of having to change them to

'no-useless-constructor': 'off',
'@typescript-eslint/no-useless-constructor': 'error',

[bradzacher]

It would be nice, yes, but the base eslint rule has no idea about typescript AST nodes (a function without a body is a typescript only feature).
ESLint has no way to extend base rules, so we have to create our own version of the rule to make it work.

Config with airbnb is a KP, which again we can't do anything about directly, because the base rule and our extension are two separate rules and thus cannot share config.

See #301 and its linked issues.

[ljharb]

@bradzacher could you reopen this? According to eslint/eslint#11464 (comment), the ESTree spec requires that body not be null, which means that the if parser is setting it to null, it's wrong/conflicting (since eslint, not the parser, is in charge of the spec for nodes).

Could this be changed to an object of some kind, to avoid the crash?

[platinumazure]

To be clear, I wasn't trying to say that the typescript-eslint parser should produce a non-null body. I was trying to say that the core no-useless-constructor rule assumes valid ESTree and assumes a non-null body. There is no reason why typescript-eslint/parser should conform to ESTree (after all, it is implementing a superset of JS so it cannot satisfy ESTree by definition). It's up to this project how to handle that (whether that is to create a non-null body in the parser, or to have a non-ESLint-core version of the no-useless-constructor rule). I did not intend to prescribe any particular approach on this project and I apologize if my comment on the ESLint side was open to misinterpretation on that score.

@ljharb It looks like the current direction here is to use the forked version of the rule, which does exist here.

[ljharb]

While that position is certainly logical, it seems like it would be much healthier for the eslint community overall if core rules could be programmed more defensively, so that alternate rules could more often augment, instead of replace, core rules.

[bradzacher]

sorry, I started rambling a bit here, it's all relevant, but its a bit of a stream of thoughts...

---

We have a typescript specific node (TSEmptyBodyFunctionExpression) used in the value of the MethodDefinition which signals that the function has no body.
The fact that this even has a body property at all is a holdover from when we didn't have this node (and instead just put a body: null into a standard FunctionExpression node).

Code + AST

class Foo {
  constructor();
  constructor(a?: any) {}
}

{
  "type": "ClassBody",
  "body": [
    {
      "type": "MethodDefinition",
      "key": {
        "type": "Identifier",
        "name": "constructor"
      },
      "value": {
        "type": "TSEmptyBodyFunctionExpression",
        "id": null,
        "params": [],
        "generator": false,
        "expression": false,
        "async": false,
        "body": null
      },
      "computed": false,
      "static": false,
      "kind": "constructor"
    },
    {
      "type": "MethodDefinition",
      "key": {
        "type": "Identifier",
        "name": "constructor"
      },
      "value": {
        "type": "FunctionExpression",
        "id": null,
        "params": [
          {
            "type": "Identifier",
            "name": "a",
            "typeAnnotation": {
              "type": "TSTypeAnnotation",
              "typeAnnotation": {
                "type": "TSAnyKeyword"
              }
            },
            "optional": true
          }
        ],
        "generator": false,
        "expression": false,
        "async": false,
        "body": {
          "type": "BlockStatement",
          "body": []
        }
      },
      "computed": false,
      "static": false,
      "kind": "constructor"
    }
  ]
}

repl

The eslint rule assumes a valid vanilla-js estree structure, so it assumes that the constructor will have a FunctionExpression node with a non-null body.
Reviewing the base rule source shows that even providing an empty object will not be enough to stop the rule from erroring - because it assumes that the body of the FunctionExpression is a BlockStatement, which has a body property of its own.

Ultimately, there are several potential courses of action:

• Create a rule within this plugin which handles the TSEmptyBodyFunctionExpression case in our custom ESTree variant
  • i.e. our current state. Bad UX because users have to know to configure our rule over the base rule.
• Replace body: null in TSEmptyBodyFunctionExpression with body: { body: [] }.
  • This is wrong for obvious reasons.
• Update the base rule so it doesn't make so many assumptions about the structure of the AST.
  • This is an option, though not a good one because the estree spec (around which the rule was written) specifically states that:
    • FunctionExpression extends Function, which has a non-null body property of type FunctionBody.
    • FunctionBody has a non-null body property of type (Directive | Statement)[].
• Replace the base MethodDefinition with a custom node (say, TSEmptyBodyMethodDefinition).
  • This is an option, but not a great one as it means that rule authors will have select both MethodDefinition and TSEmptyBodyConstructorDefinition when writing rules (also would be a significant breaking change).

The best UX for end users would be one of:

• make the base rule more defensive so it doesn't break.
  • However that makes every single rule in eslint more complex because it assumes every node has a set of properties with certain types.
• make the TS-ESTree AST complaint with the base ESTree AST in this case.
  • Should we then make sure that every single AST extension is backwards compatible? That would create a lot of overhead and complexity in this project and its consumers.

What is the best solution? I'm not sure....

[mysticatea]

We have a typescript specific node (TSEmptyBodyFunctionExpression) used in the value of the MethodDefinition which signals that the function has no body.

I think that ESTree-compatible TypeScript AST should use special nodes for type definition because JavaScript doesn't have type definitions.

The function nodes which don't have that body are the nodes of type definitions because those are completely removed in the compile phase. So I think TSMethodTypeDeclaration is better than TSEmptyBodyMethodDefinition or the MethodDefinition which has a TSEmptyBodyFunctionExpression. Similarly TSFunctionTypeDeclaration is more self-describing than TSEmptyBodyFunctionDeclaration. I think the name of TSEmptyBody* causes confusion of semantic boundary.

By the way, I found that ESTree describes that is extensible.

Extensible: New nodes should be specced to easily allow future spec additions. This means expanding the coverage of node types.

https://github.com/estree/estree/blob/master/README.md#philosophy

This means ESTree nodes can have newly unknown nodes. As far as I know, they had opposed making the existing member nullable (there was a long discussion to make FunctionDeclaration#id nullable), but they have often introduced newly node types as a part of the existing nodes.

Therefore, ESLint core rules should be more defensive for unknown node types, in my 2 cents.