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

fix(typescript-estree): error on unexpected jsdoc nodes #1525

Merged
merged 2 commits into from Jan 26, 2020
Merged

fix(typescript-estree): error on unexpected jsdoc nodes #1525

merged 2 commits into from Jan 26, 2020

Conversation

armano2
Copy link
Member

@armano2 armano2 commented Jan 25, 2020

fixes #1301

@typescript-eslint

This comment has been minimized.

@armano2 armano2 self-assigned this Jan 25, 2020
@armano2 armano2 added bug Something isn't working package: typescript-estree Issues related to @typescript-eslint/typescript-estree labels Jan 25, 2020
@codecov
Copy link

codecov bot commented Jan 25, 2020

Codecov Report

Merging #1525 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1525      +/-   ##
==========================================
+ Coverage   95.57%   95.57%   +<.01%     
==========================================
  Files         149      149              
  Lines        6688     6691       +3     
  Branches     1915     1917       +2     
==========================================
+ Hits         6392     6395       +3     
  Misses        112      112              
  Partials      184      184
Impacted Files Coverage Δ
packages/typescript-estree/src/convert.ts 99.43% <100%> (ø) ⬆️

@bradzacher bradzacher merged commit c8dfac3 into typescript-eslint:master Jan 26, 2020
@armano2 armano2 deleted the jsdoc-nodes branch January 26, 2020 20:43
@thorn0
Copy link
Contributor

thorn0 commented Jan 29, 2020

Sorry for being late on this PR. In Prettier, we thought printing ?NullableTypes as is was a good idea, useful for people who migrate from Flow to TS (see prettier/prettier#7020). Otherwise, formatting wouldn't work for them until they eliminate this syntax in the entire file. Can we probably somehow still allow this specific type (TSJSDocNullableType) of node in the AST?

@armano2
Copy link
Member Author

armano2 commented Jan 29, 2020

hmm, thats good point, main reason for this PR was fixing syntax var a: function(b): c;, JSDocFunctionType,

i'm in conflict now, i don't want to produce that broken ast, but i want to support it at same time.

i think we could support it, but we will have to prepare some tests (as to not crash because of it anymore)

@thorn0
Copy link
Contributor

thorn0 commented Jan 29, 2020

I'm asking specifically about TSJSDocNullableType only. It didn't cause any crashes, did it? Having the AST, TypeScript ESLint could even introduce a rule that would autofix it into ... | null | undefined.

The rest of the JSDoc types are of no interest. At least for now. We might meet them again though if one day Prettier decides to format JSDoc, but this is a completely different story.

@armano2
Copy link
Member Author

armano2 commented Jan 29, 2020

that's good idea, i will take a look, and prepare POC latter today,
thanks for your feedback

@armano2
Copy link
Member Author

armano2 commented Jan 29, 2020

from my research it seems that only few jdoc nodes can actually appear in types.
typescript uses this file to validate them: https://github.com/microsoft/TypeScript/blob/master/tests/baselines/reference/jsdocDisallowedInTypescript.js

JSDocAllType = '*'
JSDocUnknownType = '?'
JSDocNonNullableType = '!xxxx' | 'xxxx!'
JSDocNullableType = '?xxxx' | 'xxxx?'

additionally before this change syntax was crashing. (JSDocFunctionType)

const x: function(new: number, string);
const x: function(this: number, string);
var g: function(number, number): number;

@bradzacher
Copy link
Member

Based on @thorn0's suggestion of "people who migrate from Flow to TS"
I think we only need to support:

@armano2
Copy link
Member Author

armano2 commented Jan 29, 2020

both syntaxes leading and trailing are generating currently same node (position is no preserved),

i started writing rule for this, i will push this soon (and ofc i revert this commit and fixed issue)

my only question is what node type we should use for them? maybe we should just keep those auto-generated names

@bradzacher
Copy link
Member

Yeah they can fall into the "unknown node bucket" which just prepends TS.
I wouldn't revert this commit - just submit a new one that excludes those two from the range.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working package: typescript-estree Issues related to @typescript-eslint/typescript-estree
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exception when parsing JSDocFunctionType
3 participants