fix(ast-spec): correct some incorrect ast types #6257
Conversation
|
Thanks for the PR, @bradzacher! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #6257 +/- ##
==========================================
- Coverage 91.28% 91.27% -0.02%
==========================================
Files 366 366
Lines 12445 12445
Branches 3645 3645
==========================================
- Hits 11361 11359 -2
+ Misses 773 772 -1
- Partials 311 314 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| import type { Property } from '../element/Property/spec'; | ||
| import type { SpreadElement } from '../element/SpreadElement/spec'; | ||
|
|
||
| export type ObjectLiteralElement = MethodDefinition | Property | SpreadElement; | ||
| export type ObjectLiteralElement = Property | SpreadElement; |
There was a problem hiding this comment.
Huh, I guess they're not the same in objects. Cool.
There was a problem hiding this comment.
Yeah objects use Property with method: true
There's definitely scope to work on the Property type to better discriminate the union based on the flags.
I might raise a PR to do that soon as it's a pretty nice property to have hyper-specific types for this sort of case.
I actually did it when I wrote the flow-estree types
https://github.com/facebook/hermes/blob/main/tools/hermes-parser/js/hermes-estree/src/types.js#L448-L536
(looks like I didn't do method types, but that's a natural next step)
|
For posterity - in theory these are breaking changes as they will cause errors in consumer's code. In practice however we consider this a bug fix because the types did not match reality - the existing runtime values did not match the old types - meaning any new type errors in consuming are flagging code that may be broken and may crash for some cases. |
|
@bradzacher Sorry for the late message, but as a heads-up, virtually all expressions' type defs are too narrow after removal of the |
* fix(ast-spec): correct some ast types * type error
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | patch | [`5.47.0` -> `5.47.1`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/5.47.0/5.47.1) | | [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | patch | [`5.47.0` -> `5.47.1`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/5.47.0/5.47.1) | --- ### Release Notes <details> <summary>typescript-eslint/typescript-eslint (@​typescript-eslint/eslint-plugin)</summary> ### [`v5.47.1`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#​5471-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5470v5471-2022-12-26) [Compare Source](typescript-eslint/typescript-eslint@v5.47.0...v5.47.1) ##### Bug Fixes - **ast-spec:** correct some incorrect ast types ([#​6257](typescript-eslint/typescript-eslint#6257)) ([0f3f645](typescript-eslint/typescript-eslint@0f3f645)) - **eslint-plugin:** \[member-ordering] correctly invert optionalityOrder ([#​6256](typescript-eslint/typescript-eslint#6256)) ([ccd45d4](typescript-eslint/typescript-eslint@ccd45d4)) </details> <details> <summary>typescript-eslint/typescript-eslint (@​typescript-eslint/parser)</summary> ### [`v5.47.1`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#​5471-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5470v5471-2022-12-26) [Compare Source](typescript-eslint/typescript-eslint@v5.47.0...v5.47.1) **Note:** Version bump only for package [@​typescript-eslint/parser](https://github.com/typescript-eslint/parser) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC43My4xIiwidXBkYXRlZEluVmVyIjoiMzQuNzMuMSJ9--> Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1693 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
PR Checklist
Overview
These changes will be disruptive to downstream consumers but they are correct and will help expose potentially bad code.
Particularly the sparse array type fix because it's meant to include
null, not just some other node type. They're a rarer case in code (because sparse arrays are one of the dumbest features in JS), but if encountered it will crash code that doesn't expect it.