Skip to content
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

When can body be undefined or null in vanilla espree? #734

Closed
G-Rath opened this issue Jul 20, 2019 · 6 comments · Fixed by #1289
Closed

When can body be undefined or null in vanilla espree? #734

G-Rath opened this issue Jul 20, 2019 · 6 comments · Fixed by #1289
Labels
enhancement New feature or request package: parser Issues related to @typescript-eslint/parser package: typescript-estree Issues related to @typescript-eslint/typescript-estree

Comments

@G-Rath
Copy link
Contributor

G-Rath commented Jul 20, 2019

I'm working on converting eslint-plugin-jest to typescript, and have come across a type definition that I can't reproduce at runtime.

While converting no-test-callbacks, I found out that it's apparently possible for the body of a FunctionDeclaration to be null.

This has required me to add a null check in order to please typescript.

This in turn upsets our coverage b/c that branch is never tested, leading me here as I've not been able to write any code that reproduces the defined type.

Hence: how can I have a FunctionDeclaration w/ a body of null at runtime?

I suspect this heavily relates to #420, but theres too many moving parts for me to properly track :)

@G-Rath G-Rath added package: typescript-estree Issues related to @typescript-eslint/typescript-estree triage Waiting for team members to take a look labels Jul 20, 2019
@bradzacher bradzacher added question Questions! (i.e. not a bug / enhancment / documentation) and removed triage Waiting for team members to take a look labels Jul 20, 2019
@bradzacher
Copy link
Member

bradzacher commented Jul 20, 2019

in vanilla espree?
I have no idea. I don't think it's possible there.

In typescript there are a number of places, such as:

// declarations
declare function foo(): void;

// overloads
function bar(): void;
function bar(arg?:string) {}

// abstract
abstract class Foo {
  abstract meth(): string;

  // class method overloads
  meth2(): void
  meth2(arg?: string) {}
}

@G-Rath
Copy link
Contributor Author

G-Rath commented Jul 20, 2019

That's what my investigation reveals as well, except that at those points the node type stops being FunctionDeclaration, instead becoming a TS node type (TSDeclareFunction & TSAbstractMethodDefinition), or just don't have a body at all (in the case of MethodDefinition)

It's all sounding to me like the FunctionDeclaration & FunctionExpression interfaces should redefine body as being just BlockStatement.

For reference:

// declarations
declare function foo(): void; // TSDeclareFunction

// overloads
function bar(): void; // TSDeclareFunction
function bar(arg?:string) {} // FunctionDeclaration (w/ body of type BlockStatement)

// abstract
abstract class Foo {
  abstract meth(): string; // TSAbstractMethodDefinition

  // class method overloads
  meth2(): void // MethodDefinition (w/ value of TSEmptyBodyFunctionExpression)
  meth2(arg?: string) {} // MethodDefinition (w/ value of FunctionExpression)
}

@bradzacher
Copy link
Member

bradzacher commented Jul 21, 2019

I'm not actually sure tbh.
I can't remember the cases.

The type of body is optional in the core typescript types:
https://github.com/microsoft/TypeScript/blob/97b5671f998bdc46db6e9b5cdc2f61a66efcbe7d/src/compiler/types.ts#L1051-L1084

I didn't realise it, but in #156, it added handling to convert function declarations with no body into TSFunctionDeclaration (https://github.com/typescript-eslint/typescript-eslint/pull/156/files#diff-68b2b0b0d2d0ff1846b1114472008332R674)

So it looks like there's no case now that a FunctionDeclaration can have no body?

FunctionExpression has no such handling, so the body will have to remain nullable.
Technically via the parser, we transform empty bodied function expressions into a new type TSEmptyBodyFunctionExpression, however the types come from typescript-estree, which does not have this conversion.

case 'FunctionExpression':
if (!node.body) {
node.type = `TSEmptyBody${node.type}` as any;
}
break;

Maybe we should move that conversion into typescript-estree so we can simplify the types?

cc @JamesHenry / @uniqueiniquity

@bradzacher bradzacher added enhancement New feature or request package: parser Issues related to @typescript-eslint/parser and removed question Questions! (i.e. not a bug / enhancment / documentation) labels Jul 21, 2019
@G-Rath
Copy link
Contributor Author

G-Rath commented Sep 29, 2019

Any progress on this?

I'm happy to help if there's anything I can do 🙂

@bradzacher
Copy link
Member

Happy to accept a PR, I hadn't looked into it any further.

@armano2
Copy link
Member

armano2 commented Dec 14, 2019

note. why it stayed in this way

Initially there was a lot of additional conversions from AST produced by ts-estree in parser, most of them was cleared out, and looks like i forgot about this one.

this is part of legacy code from: https://github.com/eslint/typescript-eslint-parser/blob/master/parser.js#L46-L58

and last change to this was done in PR https://github.com/eslint/typescript-eslint-parser/pull/596/files#diff-09461573a85e2d94f056dd6814769042

those changes was done before mono-repo, and at that time there was a lot of misalignment's between projects, and pushing any breaking changes was way way harder than now.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request package: parser Issues related to @typescript-eslint/parser package: typescript-estree Issues related to @typescript-eslint/typescript-estree
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants