Parsing multiple call signatures throws exception #92
Comments
corbinu
changed the title
corbinu
changed the title
|
Setup a smoke test repo https://github.com/corbinu/eslint-typescript-smoketest |
|
Awesome, @corbinu! Thanks a lot for doing that. I've never actually set up anything like that before, is there a way we could get those tests to run ahead of PRs being approved? |
|
I know we have a "regression build" for ESLint which checks some common ESLint plugins and shared configs for regressions-- @nzakas would know how that's set up. (Not sure if we just used our Jenkins instance or what.) This article by Ariya Hidayat (creator of esprima and other cool projects) talks about how he used CircleCI for similar purposes. |
|
Thanks, @platinumazure! Let's see what @nzakas thinks. @corbinu, regarding the actual issue, this seems to be similar to other issues we have had where core rules rely on a function/method declaration to have a body. @nzakas We could go a similar route to previous solutions here: Add a check for no body (currently we are just attaching the result of |
|
@corbinu Please could link to the actual source of this code? |
|
@JamesHenry I don't understand that is the source just clone, Relevant test for VSCode is here: https://gitlab.com/corbinu/eslint-typescript-smoketest/builds/4153769 It is a bit deceptive from the build as projects some of the tests are just failing on lint rules, but as things shake out here I should be able to get them all green. |
|
Sorry I should have been clearer - When I said source, I meant the source code that was being linted, and that caused the error. (i.e. the What code were you trying to parse? section above.) Looking at the build output: It doesn't help determine what TypeScript code it was trying to parse when the export class Test {
constructor() {
}
public test(param1: Number): Test;
public test(param1: Test): Test;
public test(param1: any): Test {
return new Test();
}
} |
|
@JamesHenry oh sorry that was trying to lint what it is "src/vscode" after the copy. I then linted the files one at a time and found the issue was present in: https://github.com/Microsoft/vscode/blob/master/src/vs/base/browser/builder.ts At line 131 the builder class. I then simply reduced that code down till I got a simple example that caused the error. Thats where the test class comes from. However many of the projects I added in the smoke tests throw the same error when they have multiple call signatures. |
|
That's great, thanks! |
|
@JamesHenry So I tracked it down to visit function in escope when visiting a function assumes it has a body. I can do a PR over there to that simply checks if the node as a body https://github.com/estools/escope/blob/master/src/referencer.js#L226 I made that change on my machine and VSCode started parsing fine! Otherwise is there some better way for me to try to fix this in this repo? Thanks |
|
No, we won't be changing escope for this. We need to accommodate it in the AST in a similar way to other cases. Let's see what @nzakas thoughts are on my question to him above:
|
|
Just so I understand, we're saying this is the problem? /**
* Creates a new Builder that performs all operations on the current element of the builder and
* the builder or element being passed in.
*/
public and(element: HTMLElement): MultiBuilder;
public and(builder: Builder): MultiBuilder;
public and(obj: any): MultiBuilder {
// Convert HTMLElement to Builder as necessary
if (!(obj instanceof Builder) && !(obj instanceof MultiBuilder)) {
obj = new Builder((<HTMLElement>obj), this.offdom);
}
// Wrap Builders into MultiBuilder
let builders:Builder[] = [this];
if (obj instanceof MultiBuilder) {
for (let i = 0; i < (<MultiBuilder>obj).length; i++) {
builders.push((<MultiBuilder>obj).item(i));
}
} else {
builders.push(obj);
}
return new MultiBuilder(builders);
}And because I'm still not very familiar with TypeScript, I'm guessing that the extra "definitions" are really just overloading the method signature? I think overall we need to ask: Do we want ESLint rules to check these extra method definitions or not? (Making them So, I think we probably want them to be @corbinu can you open a separate issue regarding the smoke test? I'd like to talk about that some more but don't want to have that info multiplexed with the bug. |
|
@nzakas, yes you are exactly right. I have been leaning towards getting through these parser runtime errors as quickly as possible, and worrying about the lack of linting on "special nodes" in future PRs. Purely because I feel like our users are more likely to give up and dismiss the project due to runtime errors, but will be much more forgiving of lack of coverage of TS constructs. In other words, However, I totally agree that we will want linting on nodes like: public and(element: HTMLElement): MultiBuilder;...at some stage (e.g. your type annotation rule in the plugin makes perfect sense here), and so I am on board with taking the extra time to tackle the long term issue now in escope now. |
|
@JamesHenry yeah, I understand the concern. My larger concern is that changing AST structure is a nontrivial change, so I'd like to avoid that whenever possible. Temporary changes mean future pain, so I'd rather take the pain now and have a smoother path forward. @corbinu @JamesHenry do one of you want to open an issue/PR with escope? |
|
@nzakas Sure but what am I saying. I guess I am a little confused by what we are going to change in escope. Are we doing my simple check to make sure that the null body cause a failure or are we needed escope to parse this in some other way? If it is my small fix I am happy to also open PR |
|
@corbinu exactly what you posted. |
|
I'll submit a PR to escope shortly, hopefully it should also fix #80 |
|
@nzakas not to be rude but none of the estools repos seem to have a pulse. Do you know if anybody at estools is still responsible for reviewing the issues and PRs? If not given eslints dependence on escope perhaps they would be ok with you guys taking over? Just a though don't want to push a bunch of new work on you guys. Just sounds from the roadmap like eslint 4 is going to need a few additional escope changes. |
|
I'll ping the guys behind it. I do have access to the repo and publishing permissions, I just don't feel that I can push through whatever I want because of it (spirit of collaboration). We have talked about forking escope into something that we own and control, but that is a bunch of extra work and the team is already being crushed by the deluge of PRs and issues, so I don't want to do that until I'm at least healthy enough to mostly manage it myself. |
|
@nzakas no worries thanks for all your great work it is much appricated! Sounds good and makes sense |
|
@nzakas @JamesHenry Ok diffrent plan sounds like a lot of this plugin would benefit from escope changes. I can easily fork and update escope for eslint (update to node 4 and eslint the code and update deps etc.) However I am wondering would the better option be if I create a PR for ESlint that allows a parser like this one to override escope for their own analyzer. That way this parser could simply shim in its own version that is ok with the strange syntax? If that is a crazy idea please let me know! |
I think that's a step beyond where we want to go to support this. We don't want to bundle the parser with a scope analyzer. Give me some time to think about this a bit. |
|
@nzakas Ok no worries take your time know it would be a big change thought I would suggest it as assumed allowing different analyzers would probably need to be a major version feature so thought I would suggest it before v4 was set in stone. |
❤️ I cannot agree more. IMO abstract method (from issue #80) shoud be given a separate AST construct, or whatever treatment method declarations receive on interface declarations. They are not real methods, only declarations. For instance I wouldn't verify number of parameters. I would like to check them on method implementations, if issues are reported I would fix there. Additional function signatures (this issue) should work better as TypeAnnotations attached to a function/method definition as @corbinu mentioned at the escope repo. |
JamesHenry
changed the title
JamesHenry
changed the title
|
I have tried to fix this issue by creating a body for the FunctionExpression node. If we were to create a type annotation instead would this eliminate checks on the function style (space-before-func-param, no-unused-def and explicit-member-accessibility) |
|
Just FYI, we have begun work on the escope fork. It will take a little while to get everything in order, but you can expect to see a resolution to this issue soon. |
|
This also causes the rule lines-around-directive to fail. Even after patching escope |
|
I have a fix for this that changes the FunctionExpression and MethodDeclation type to TSEmptyBodyFunctionExpression and TSEmptyBodyMethodDeclaration. Even with the fix to eslint-scope there are still rules that depend on the fact that these node types have bodies. For example lines-around-directive and array-callback-return. I don't believe we should expect that rule authors are going to consider an invalid estree. In my opinion if we use the estree node types it must follow the spec. |
There are rules in eslint that expect FunctionExpression nodes to contain a body. We should not produce an invalid estree node if we use the estree node types.
There are rules in eslint that expect FunctionExpression nodes to contain a body. We should not produce an invalid estree node if we use the estree node types.
|
I have the same error messages, but I can't find the problem in my code. |
|
@monolithed This is fixed in eslint-scope, however eslint master still is using escope. Try using this fork if you want to test it out. I am not sure when this will be stable though |
There are rules in eslint that expect FunctionExpression nodes to contain a body. We should not produce an invalid estree node if we use the estree node types.
There are rules in eslint that expect FunctionExpression nodes to contain a body. We should not produce an invalid estree node if we use the estree node types.
There are rules in eslint that expect FunctionExpression nodes to contain a body. We should not produce an invalid estree node if we use the estree node types.
|
Closing as 4.0.0 has been released whoop!! |
|
@corbinu Looking at the latest |
For fun I decided to test VSCode using this parser (seemed like a good large project to test) and my first error was this:
If you guys are open to it I would be happy to actually fork VSCode and setup Travis to smoke test the parser against it. That way I can easily open new issues as fixes are released.
What version of TypeScript are you using?
1.8.10
What version of
typescript-eslint-parserare you using?0.3.0
What code were you trying to parse?
What did you expect to happen?
Should parse out the multiple function definitions
What happened?
The text was updated successfully, but these errors were encountered: