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

[@typescript-eslint/no-unnecessary-type-assertion] Fixing Vue single file components applies changes to incorrect character location #4723

Closed
3 tasks done
jaa134 opened this issue Mar 24, 2022 · 6 comments · Fixed by #4790
Assignees
Labels
bug Something isn't working has pr there is a PR raised to close this package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin vue issues relating to vue support

Comments

@jaa134
Copy link

jaa134 commented Mar 24, 2022

Applying the automatic fix for non-null assertions removes a character from the wrong place. It seems related to #2591 and #2680 where vue-eslint-parser alters the locations information for every node it produces within a script tag, but it doesn't know about the typescript nodes, so their range information is wrong. The fix is being applied as if there was no script tag. You can see that by running the lint command without the --fix flag, the eslint command output reports the error at an incorrect line or character index.

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have read the FAQ and my problem is not listed.

Repro

Steps:

  1. Create a simple Vue 3 single file component like below. This component unnecessarily does type assertion on the string literal.
<script setup lang="ts">
  const test = 'foobar'.length!
</script>
<template>
  <div></div>
<template>
  1. Apply linting with --fix flag and with the rule enabled. See package.json/.eslintrc examples below. I am using latest versions of vue-eslint-plugin, this TS parser, this TS plugin, and Eslint.
  2. When linting completes, the letter "t" is removed from const and not the unnecessary non-null assertion.
module.exports = {
    env: {
        browser: true,
        es6: true,
        'vue/setup-compiler-macros': true
    },
    plugins: ['@typescript-eslint'],
    parser: 'vue-eslint-parser',
    parserOptions: {
        parser: '@typescript-eslint/parser',
        project: '**/tsconfig.json',
        sourceType: 'module',
        extraFileExtensions: ['.vue']
    },
    extends: [
        'eslint:recommended',
        'plugin:@typescript-eslint/recommended',
        'plugin:@typescript-eslint/recommended-requiring-type-checking',
        'plugin:vue/essential',
        'prettier'
    ],
    rules: {
        '@typescript-eslint/ban-types': 'off',
        '@typescript-eslint/no-namespace': 'off',
        '@typescript-eslint/adjacent-overload-signatures': 'off',
        '@typescript-eslint/prefer-regexp-exec': 'off',
        '@typescript-eslint/no-unsafe-call': 'off',
        '@typescript-eslint/no-unsafe-assignment': 'off',
        '@typescript-eslint/no-unsafe-member-access': 'off',
        '@typescript-eslint/no-unsafe-return': 'off',
        '@typescript-eslint/restrict-template-expressions': 'off'
    }
};
{
    "name": "foobar",
    "version": "0.0.1",
    "scripts": {
        "lint:js": "eslint src --ext .js,.ts,.vue --quiet",
        "lint:js:fix": "eslint src --ext .js,.ts,.vue --fix --quiet"
    },
    "dependencies": {
        "vue": "^3.2.25"
    },
    "devDependencies": {
        "@typescript-eslint/eslint-plugin": "^5.16.0",
        "@typescript-eslint/parser": "^5.16.0",
        "eslint": "^8.11.0",
        "eslint-config-prettier": "^8.3.0",
        "eslint-plugin-vue": "^8.5.0",
        "typescript": "^4.4.3",
        "vue-eslint-parser": "^8.3.0"
    }
}
{
  "compilerOptions": {
    "baseUrl": ".",
    "esModuleInterop": true,
    "jsx": "preserve",
    "lib": ["esnext", "dom"],
    "module": "esnext",
    "moduleResolution": "node",
    "noImplicitAny": false,
    "paths": {
      "@/*": ["src/*"]
    },
    "resolveJsonModule": true,
    "sourceMap": true,
    "strict": true,
    "target": "esnext",
    "types": ["vite/client", "@types/jest"]
  },
  "include": ["src/**/*.ts", "src/**/*.vue"]
}

Expected Result

See that the non-null assertion was removed.

<script setup lang="ts">

  const test = 'foobar'.length

</script>

<template>
  <div >
<template>

Actual Result

See that the letter 't' was removed from const.

<script setup lang="ts">

  cons test = 'foobar'.length!

</script>

<template>
  <div >
<template>

Additional Info

It looks like a similar issue was identified in #2591. And a fix was applied in #2680.

Versions

package version
@typescript-eslint/eslint-plugin 5.16.0
@typescript-eslint/parser 5.16.0
TypeScript 4.4.3
ESLint 8.11.0
node 17.3.1
@jaa134 jaa134 added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Mar 24, 2022
@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Mar 26, 2022

Thanks for posting the issue @jaa134! None of us on the maintenance team here are particularly common Vue users, so just seeing the source file, package.json, and ESLint config aren't enough for us to investigate. Could you please post a standalone repository or something like it that includes instructions on how to set it up?

@JoshuaKGoldberg JoshuaKGoldberg added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for maintainers to take a look labels Mar 26, 2022
@jaa134
Copy link
Author

jaa134 commented Mar 28, 2022

@JoshuaKGoldberg Sure thing, I can set up a repo for you all. I do want to clarify that building the project isn't necessary. You should just need the ESLint config, a .vue single file component with the above code, and the dependencies installed from package.json.

@bradzacher bradzacher added the vue issues relating to vue support label Mar 29, 2022
@jaa134
Copy link
Author

jaa134 commented Mar 29, 2022

@bradzacher @JoshuaKGoldberg Here is the repo! Sorry for the delay. Let me know if there is anything else I can help out with. https://github.com/jaa134/ESLint-Vue-Test

@JoshuaKGoldberg JoshuaKGoldberg added triage Waiting for maintainers to take a look and removed awaiting response Issues waiting for a reply from the OP or another party labels Mar 29, 2022
@armano2
Copy link
Member

armano2 commented Apr 6, 2022

@JoshuaKGoldberg @jaa134 actual reason for this issue is related to what location data are we using when we are dealing with fixers, in case of this rule data used comes from typescript instance and we should always use node range,

source of originalNode is a typescript :)

originalNode.expression.end,
originalNode.end,

this issue is going to "appear" in every place when we try to use location data from typescript, and its not related to vue, same issue can be reproduced with any other parser eg, mdx

@armano2 armano2 added bug Something isn't working has pr there is a PR raised to close this and removed triage Waiting for maintainers to take a look labels Apr 6, 2022
@armano2 armano2 self-assigned this Apr 6, 2022
@jaa134
Copy link
Author

jaa134 commented Apr 6, 2022

@armano2 Thank you very much for the quick turn-around time! Do you know of any other rules that might be affected by the same problem? Obviously, I would prefer to turn these off. Searching the code base like grep -rnw packages/eslint-plugin/src/rules -e 'removeRange' -A 3 does not seem to provide any answers here (to me at least 😄 ).

@jaa134
Copy link
Author

jaa134 commented Apr 8, 2022

@JoshuaKGoldberg @armano2 Thank you to everyone involved! Impressed by the quick turnaround time on this. Y'all rock 🔥

crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this issue Apr 13, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.18.0` -> `5.19.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/5.18.0/5.19.0) |
| [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.18.0` -> `5.19.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/5.18.0/5.19.0) |

---

### Release Notes

<details>
<summary>typescript-eslint/typescript-eslint (@&#8203;typescript-eslint/eslint-plugin)</summary>

### [`v5.19.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#&#8203;5190-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5180v5190-2022-04-11)

[Compare Source](typescript-eslint/typescript-eslint@v5.18.0...v5.19.0)

##### Bug Fixes

-   **eslint-plugin:** update code to use estree range instead of ts pos/end [#&#8203;4723](typescript-eslint/typescript-eslint#4723) ([#&#8203;4790](typescript-eslint/typescript-eslint#4790)) ([a1e9fc4](typescript-eslint/typescript-eslint@a1e9fc4))

##### Features

-   **eslint-plugin:** \[unified-signatures] add `ignoreDifferentlyNamedParameters` option ([#&#8203;4659](typescript-eslint/typescript-eslint#4659)) ([fdf95e0](typescript-eslint/typescript-eslint@fdf95e0))
-   **eslint-plugin:** add support for valid number and bigint intersections in restrict-plus-operands rule ([#&#8203;4795](typescript-eslint/typescript-eslint#4795)) ([19c600a](typescript-eslint/typescript-eslint@19c600a))

</details>

<details>
<summary>typescript-eslint/typescript-eslint (@&#8203;typescript-eslint/parser)</summary>

### [`v5.19.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#&#8203;5190-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5180v5190-2022-04-11)

[Compare Source](typescript-eslint/typescript-eslint@v5.18.0...v5.19.0)

**Note:** Version bump only for package [@&#8203;typescript-eslint/parser](https://github.com/typescript-eslint/parser)

</details>

---

### Configuration

📅 **Schedule**: 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, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1299
Reviewed-by: crapStone <crapstone@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working has pr there is a PR raised to close this package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin vue issues relating to vue support
Projects
None yet
4 participants