-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
no-useless-constructor: Cannot read property 'body' of null #420
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
Have you tried using our rule instead? |
No error occurs with the It would however be nice to be able to use the 'no-useless-constructor': 'off',
'@typescript-eslint/no-useless-constructor': 'error', |
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). 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. |
@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? |
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. |
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. |
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 ( 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"
}
]
} The eslint rule assumes a valid vanilla-js estree structure, so it assumes that the constructor will have a Ultimately, there are several potential courses of action:
The best UX for end users would be one of:
What is the best solution? I'm not sure.... |
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 By the way, I found that ESTree describes that is extensible.
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 Therefore, ESLint core rules should be more defensive for unknown node types, in my 2 cents. |
This seems to be the same issue as #15.
What code were you trying to parse?
This also happens with both examples from the other issue,
and
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.
Versions
@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
The text was updated successfully, but these errors were encountered: