Skip to content
This repository was archived by the owner on Jan 19, 2019. It is now read-only.

Breaking: throw syntax error on incomplete variable declarations (fixes #531) #547

Closed
wants to merge 1 commit into from

Conversation

mysticatea
Copy link
Member

This PR makes the parser throwing on incomplete variable declarations.

It's a syntax error on both JavaScript and TypeScript. But TypeScript parser looks so tolerant and triggers that core rules are confusing.

@@ -63,6 +63,15 @@
"collectCoverage": true,
"coverageReporters": [
"text-summary"
],
"collectCoverageFrom": [
Copy link
Member Author

@mysticatea mysticatea Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this configuration of jest because jest threw a syntax error on new tests/fixtures/basics/solo-const.src.js file while collecting coverage.

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something we can control in the TS parser (e.g., with a parser option)? It might be better/easier if we just tell TS not to tolerate that error type.

@mysticatea
Copy link
Member Author

I guess tsc verifies it on type checking phase so the parsing phase is tolerant.

@platinumazure
Copy link
Member

Got it.

I guess my other thought is, should this be handled in typescript-estree? If that project is supposed to create ESTree output and variable declarations with no declarators are invalid, maybe that should throw the error rather than typescript-eslint-parser?

What do you think?

@mysticatea
Copy link
Member Author

I'm thinking about it, too.

I'm not sure if this is a spec violation or not because: ESTree defines types (E.g., VariableDeclaration#declarations is Array<VariableDeclarator>) but ESTree doesn't define restrictions (E.g., VariableDeclaration#declarations.length should not be 0).

Anyway, I should open an issue on typescript-estree repo.

@JamesHenry
Copy link
Member

Yep we can get that in as an option in the parser 👍

@JamesHenry
Copy link
Member

See my comment here after Andy from the TypeScript team responded: #531 (comment)

It should hopefully be possible for us to leverage TypeScript here, as long as we can clarify the behaviour is correct

@JamesHenry
Copy link
Member

Response from Andy:

You could open an issue, though this is hard to change because grammar and semantic checking are coupled currently.

Maybe a good next step would be if I add an option to the parser (which will be false by default so as to be non-breaking) which checks for results from both:

program.getSyntacticDiagnostics(file);
program.getSemanticDiagnostics(file);

E.g. the option could be

"errorOnTypeScriptSyntaticAndSemanticIssues": false

...and we could see if that is something this ESLint parser would to enable?

@mysticatea
Copy link
Member Author

I'm sorry for the delay. I have overlooked this.

errorOnTypeScriptSyntaticAndSemanticIssues option sounds good to me.

@armano2
Copy link
Contributor

armano2 commented Dec 17, 2018

errorOnTypeScriptSyntaticAndSemanticIssues seems nice
for now i implemented this as rule in bradzacher/eslint-plugin-typescript#245

@j-f1
Copy link
Contributor

j-f1 commented Dec 17, 2018

throwSemanticErrors?

@JamesHenry
Copy link
Member

FYI I added the errorOnTypeScriptSyntaticAndSemanticIssues option to typescript-estree in version 8.1.0, so you can opt in to that and it will error in the exact test case you have in this PR

@JamesHenry
Copy link
Member

Once we merge the latest updates of typescript-estree (#596), we can explore surfacing an option for the diagnostics reporting

@JamesHenry
Copy link
Member

Closing this for now, we'll add in full support for errorOnTypeScriptSyntaticAndSemanticIssues

@JamesHenry JamesHenry closed this Jan 13, 2019
@JamesHenry JamesHenry deleted the issue531 branch January 13, 2019 17:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants