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, eslint-plugin): stop adding ParenthesizedExpressions to node maps #226

Merged

Conversation

uniqueiniquity
Copy link
Contributor

@uniqueiniquity uniqueiniquity commented Feb 7, 2019

Builds off #212
The core issue for the crashes is that the TS AST has individual nodes for parenthesized expressions, but the ESTree AST does not.
Assuming a tree of the form (E), where E is some expression, we would map the TS version of E to the ESTree version of E, but the ESTree version of E would map to the TS version of (E), thereby causing the missing type property in the repro.
This PR makes it so we don't add TS nodes of type ParenthesizedExpression to the ES->TS map.

I also moved some things around in the particular rule you were investigating; in particular, there's no good reason for us to use the TS node to get the text to check against the provided options, so I switched it to using the ESTree node.

fixes: #187

@uniqueiniquity uniqueiniquity requested review from JamesHenry and armano2 and removed request for JamesHenry February 7, 2019 18:15
@uniqueiniquity uniqueiniquity changed the title Handle parens convert fix(typescript-estree, eslint-plugin): stop adding ParenthesizedExpressions to node maps Feb 7, 2019
@codecov
Copy link

codecov bot commented Feb 7, 2019

Codecov Report

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

@@            Coverage Diff             @@
##           master     #226      +/-   ##
==========================================
+ Coverage   96.78%   96.79%   +<.01%     
==========================================
  Files          56       56              
  Lines        2522     2524       +2     
  Branches      372      373       +1     
==========================================
+ Hits         2441     2443       +2     
  Misses         42       42              
  Partials       39       39
Impacted Files Coverage Δ
packages/typescript-estree/src/convert.ts 97.9% <100%> (ø) ⬆️
...-plugin/lib/rules/no-unnecessary-type-assertion.js 100% <100%> (ø) ⬆️

Copy link
Member

@armano2 armano2 left a comment

Choose a reason for hiding this comment

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

this is way better solution than mine, good catch,

besides this small note LGTM

packages/typescript-estree/src/convert.ts Outdated Show resolved Hide resolved
armano2
armano2 previously approved these changes Feb 7, 2019
@codecov
Copy link

codecov bot commented Feb 7, 2019

Codecov Report

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

@@            Coverage Diff             @@
##           master     #226      +/-   ##
==========================================
+ Coverage   96.78%   96.79%   +<.01%     
==========================================
  Files          56       56              
  Lines        2522     2525       +3     
  Branches      372      374       +2     
==========================================
+ Hits         2441     2444       +3     
  Misses         42       42              
  Partials       39       39
Impacted Files Coverage Δ
...-plugin/lib/rules/no-unnecessary-type-assertion.js 100% <100%> (ø) ⬆️
packages/typescript-estree/src/convert.ts 97.9% <100%> (ø) ⬆️

@uniqueiniquity
Copy link
Contributor Author

Failing from line ending issues in the fixture

armano2
armano2 previously approved these changes Feb 8, 2019
@uniqueiniquity uniqueiniquity merged commit 317405a into typescript-eslint:master Feb 8, 2019
@uniqueiniquity uniqueiniquity deleted the handleParensConvert branch February 8, 2019 16:38
kaicataldo pushed a commit to kaicataldo/typescript-eslint that referenced this pull request Aug 27, 2019
…int#226)

### Disallow `/// <reference path="" />` comments

but it's triggered on:
- `/// <reference types="foo" />`
- `/// <reference lib="es2017.string" />`
- `/// <reference no-default-lib="true"/>`

types/lib/no-default-lib can't be replaced by es6 imports
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2020
Luckeu pushed a commit to Luckeu/typescript-eslint that referenced this pull request Jun 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positives for no-unnecessary-type-assertion
3 participants