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

Update: throw error when fix range is invalid #14142

Merged
merged 7 commits into from Mar 11, 2021

Conversation

jtbandes
Copy link
Contributor

@jtbandes jtbandes commented Feb 26, 2021

This plugin has a bug when it's used with @typescript-eslint/parser. The bug causes it to try and make a fix to a large range of source code, but with a range of {start:undefined, end:undefined}.

ESLint doesn't handle this well, and ends up appending the fix text (which, due to the plugin bug, is the whole source text) at the beginning of the file, even though the range it should be inserted was unknown:

output += text.slice(Math.max(0, lastPos), Math.max(0, start));
output += fix.text;

These bugs interact such that ESLint runs repeatedly on longer and longer strings (as it tries up to 10 times to make a successful fix), eventually running out of memory if the original source file was large.

The plugin is definitely at fault for producing an invalid fix, but ESLint still has a responsibility not to apply an invalid fix.

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

Bug fix

Tell us about your environment

  • ESLint Version: 7.20.0
  • Node Version: 15.9.0
  • npm Version: 7.5.4

What parser (default, @babel/eslint-parser, @typescript-eslint/parser, etc.) are you using?

@typescript-eslint/parser

Please show your full configuration:

Configuration
    "@typescript-eslint/parser": "^4.15.2",
    "eslint-import-resolver-node": "^0.3.4",
    "eslint-plugin-import-order-alphabetical": "^1.0.1"
env:
  browser: true
  es6: true
  node: true

parser: "@typescript-eslint/parser"
parserOptions:
  ecmaVersion: 12
  sourceType: module
plugins:
  - import-order-alphabetical

rules:
  import-order-alphabetical/order:
    - error
    - newlines-between: always
      groups: [[builtin, external], [internal, parent, sibling, index]]

What did you do? Please include the actual source code causing the issue.
Install the packages above.
Try linting (with --fix) an example file with some out-of-order imports

import x from "def";
import z from "abc";

What did you expect to happen?
Imports are fixed

What actually happened? Please include the actual, raw output from ESLint.
ESLint runs the rule 10 times. If the input file was large enough, it can run out of JS heap memory.

What changes did you make? (Give an overview)

Simply skip applying fixes if the range is invalid.

Added a test case that passes on this branch but fails on master.

@mdjermanovic mdjermanovic added bug ESLint is working incorrectly core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion needs design Important details about this change need to be discussed accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Feb 27, 2021
@mdjermanovic
Copy link
Member

Hi @jtbandes, thanks for the PR and all the details!

I agree we should handle non-number ranges explicitly, instead of letting them pass to the source code fixing calculations where numbers are expected and non-numbers such as undefined produce unpredictable results.

Just, I think we should have these checks at an earlier point, maybe in report-translator. If we add them to source-code-fixer, they will prevent some errors, but our API could still return invalid EditInfo objects.

An additional question is should we silently drop the fix or throw an error.

Marked this as "accepted" and "needs design" since this is something that should be fixed, but we have to figure out the details.

@jtbandes
Copy link
Contributor Author

@mdjermanovic Thanks for the feedback! I considered putting this validation in report-translator, but I noticed that if I put the validation in rule-fixer then it's the only kind of validation in here. Since there is no other validation, I wasn't sure if this was an appropriate place to add it. Since this is kind of far away from the point of use, I also wasn't confident that report-translator/rule-fixer is the only way that bad values could get in to source-code-fixer. But if you think that's the right approach, I'm happy to do that instead.

As for throwing an error — I think that would be ideal, but it might break some code that's unintentionally depending on it. Maybe it would make sense to throw an error starting in the next major version of eslint since it could be a breaking change. For now, logging a warning seems best, what do you think?

@eslint-github-bot
Copy link

Hi @jtbandes!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag must be one of the following:

    The Tag is one of the following:

    • Fix - for a bug fix.
    • Update - either for a backwards-compatible enhancement or for a rule change that adds reported problems.
    • New - implements a new feature.
    • Breaking - for a backwards-incompatible enhancement or feature.
    • Docs - changes to documentation only.
    • Build - changes to build process only.
    • Upgrade - for a dependency upgrade.
    • Chore - for anything that isn't user-facing (for example, refactoring, adding tests, etc.).

    You can use the labels of the issue you are working on to determine the best tag.

  • There should be a space following the initial tag and colon, for example 'New: Message'.

Read more about contributing to ESLint here

@jtbandes jtbandes changed the title Do nothing when fix range is invalid Fix: Do nothing when fix range is invalid Mar 1, 2021
@mdjermanovic
Copy link
Member

I was thinking about lib/linter/report-translator.js, at some point after we call fix().

Returning null from lib/linter/rule-fixer.js could be problematic for rules that return an array/sequence of partial fixes, which are then merged to one. Throwing an error from rule-fixer would be fine (if we decide that an error should be thrown in this case), but still a rule might for whatever reason avoid using rule-fixer (i.e. the fixer argument in fix(fixer)) and return anything from the fix() function.

report-translator looks to me like the best place for these checks, as it has the final return value of fix() and is also the only place where we call fix() functions from rules.

@jtbandes
Copy link
Contributor Author

jtbandes commented Mar 1, 2021

That's a good point that fix functions don't have to use the fixer. I moved the errors to report-translator — chose to throw errors rather than just logging.

@jtbandes
Copy link
Contributor Author

jtbandes commented Mar 5, 2021

Hi @mdjermanovic — just curious if you've had a chance to look at this again?

@mdjermanovic mdjermanovic changed the title Fix: Do nothing when fix range is invalid Update: throw error when fix range is invalid Mar 5, 2021
@mdjermanovic
Copy link
Member

Yes, this looks good! I'd have a few suggestions about some details, but would like first to be sure that we have a consensus on throwing an error in this case.

@mdjermanovic
Copy link
Member

If a rule returns a range that isn't [number, number], then it's a bug in the rule.

Unfortunately, ESLint currently doesn't validate that, and also doesn't throw by chance.

I think we should either:

  1. Throw an error right after we get an invalid fix from a rule.
  2. Just remove an invalid fix from the result object.

eslint --fix with a rule that returns [undefined, undefined] finishes successfully, but produces invalid files. As these runs already produce invalid results, 1. might be ok for a minor release.

eslint without --fix is more questionable because it doesn't save invalid files. Throwing an error now could break someone's build, while the previous behavior with [undefined, undefined] might not have been observable. Still, ESLint shouldn't produce invalid EditInfo objects, because it's unexpected for formatters and integrations. Therefore, the behavior should be probably the same as with --fix, but then 2. might be a better choice.

@jtbandes
Copy link
Contributor Author

jtbandes commented Mar 5, 2021

If we choose 2. we should probably still output some kind of warning, otherwise debugging issues will be difficult. I agree that if we throw an error it should be right after getting an invalid fix from a rule, which is why I tried putting it in rule-fixer, but then you pointed out not all rules use rule-fixer. So it seemed like report-translator was a good compromise there.

Do you think we should intentionally do different behavior depending on the --fix setting, such as throwing an error when --fix is provided but just logging a warning otherwise? I'm not sure if it is currently easy for this code to know whether --fix is enabled or not.

@mdjermanovic
Copy link
Member

If we choose 2. we should probably still output some kind of warning, otherwise debugging issues will be difficult.

You're right, it would be confusing, especially during the development.

Do you think we should intentionally do different behavior depending on the --fix setting, such as throwing an error when --fix is provided but just logging a warning otherwise? I'm not sure if it is currently easy for this code to know whether --fix is enabled or not.

That could work, but seems very complicated to implement.

I'd vote for 1. - throwing an error regardless of the context, as it is implemented in the current iteration of this PR.

@nzakas
Copy link
Member

nzakas commented Mar 8, 2021

I’m also in favor of throwing an error. That’s definitely preferable to a random error or a random incorrect fix, so I think introducing this in a minor release is fine.

I think we only run the fix() method of a rule when running in fix mode, so that’s probably not going to throw an error when not running in fix mode, and I think that’s fine.

lib/linter/report-translator.js Outdated Show resolved Hide resolved
lib/linter/report-translator.js Outdated Show resolved Hide resolved
@jtbandes jtbandes requested a review from nzakas March 8, 2021 22:50
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@jtbandes
Copy link
Contributor Author

jtbandes commented Mar 9, 2021

Thanks for reviewing! I'm not aware of the policy for merging, but since I'm not a repo member I will need someone else to do it :)

tests/lib/linter/report-translator.js Outdated Show resolved Hide resolved
tests/lib/linter/report-translator.js Show resolved Hide resolved
lib/linter/report-translator.js Outdated Show resolved Hide resolved
@mdjermanovic mdjermanovic removed the needs design Important details about this change need to be discussed label Mar 9, 2021
This was referenced Mar 18, 2021
facebook-github-bot pushed a commit to facebook/flipper that referenced this pull request Mar 25, 2021
Summary:
Bumps [eslint](https://github.com/eslint/eslint) from 7.21.0 to 7.22.0.
<details>
<summary>Release notes</summary>
<p><em>Sourced from <a href="https://github.com/eslint/eslint/releases">eslint's releases</a>.</em></p>
<blockquote>
<h2>v7.22.0</h2>
<ul>
<li><a href="https://github.com/eslint/eslint/commit/3a432d82b3a5710aff7da20302fe0b94fedc46c2"><code>3a432d8</code></a> Docs: Improve documentation for indent rule (<a href="https://github.com/eslint/eslint/issues/14168">#14168</a>) (Serkan Özel)</li>
<li><a href="https://github.com/eslint/eslint/commit/f62ec8d30d925e70e4d0d40640857c587ac2e116"><code>f62ec8d</code></a> Update: throw error when fix range is invalid (<a href="https://github.com/eslint/eslint/issues/14142">#14142</a>) (Jacob Bandes-Storch)</li>
<li><a href="https://github.com/eslint/eslint/commit/0eecad271358f753730741fcfcb2f7cc915c1fa7"><code>0eecad2</code></a> Upgrade: Update lodash in package.json to V 4.17.21 (<a href="https://github.com/eslint/eslint/issues/14159">#14159</a>) (Basem Al-Nabulsi)</li>
<li><a href="https://github.com/eslint/eslint/commit/5ad91aa7df3d6bc185786e6eccd9e055fd951055"><code>5ad91aa</code></a> Update: report es2021 globals in no-extend-native (refs <a href="https://github.com/eslint/eslint/issues/13602">#13602</a>) (<a href="https://github.com/eslint/eslint/issues/14177">#14177</a>) (Milos Djermanovic)</li>
<li><a href="https://github.com/eslint/eslint/commit/c295581aca4e08ec4ae8e5ee5726a6f454a3ee26"><code>c295581</code></a> Chore: remove leftover JSDoc from lint-result-cache (<a href="https://github.com/eslint/eslint/issues/14176">#14176</a>) (Milos Djermanovic)</li>
<li><a href="https://github.com/eslint/eslint/commit/0d541f9d9d58966372e2055a8f69fb9483d56a4b"><code>0d541f9</code></a> Chore: Reduce lodash usage (<a href="https://github.com/eslint/eslint/issues/14178">#14178</a>) (Stephen Wade)</li>
<li><a href="https://github.com/eslint/eslint/commit/27a67d71ffa9bbd7af02ae448844e127bcf956dc"><code>27a67d7</code></a> Sponsors: Sync README with website (ESLint Jenkins)</li>
<li><a href="https://github.com/eslint/eslint/commit/459d821f4a599501ceb002f9d7a5034fc45ffbb0"><code>459d821</code></a> Chore: upgrade dependencies of browser test (<a href="https://github.com/eslint/eslint/issues/14127">#14127</a>) (Pig Fang)</li>
<li><a href="https://github.com/eslint/eslint/commit/ebfb63a682004a008f2707dbad616e5ae1630b2c"><code>ebfb63a</code></a> Sponsors: Sync README with website (ESLint Jenkins)</li>
<li><a href="https://github.com/eslint/eslint/commit/3ba029fbffd44068be93254890fc2aec3e92c212"><code>3ba029f</code></a> Docs: Remove Extraneous Dash (<a href="https://github.com/eslint/eslint/issues/14164">#14164</a>) (Danny Hurlburt)</li>
<li><a href="https://github.com/eslint/eslint/commit/6f4540ea7ea39775906526506fd7abd7ea97610c"><code>6f4540e</code></a> Sponsors: Sync README with website (ESLint Jenkins)</li>
<li><a href="https://github.com/eslint/eslint/commit/ddf361ca2a2a01a9974f421e5f62270df282d0e8"><code>ddf361c</code></a> Docs: Fix Formatting (<a href="https://github.com/eslint/eslint/issues/14154">#14154</a>) (Danny Hurlburt)</li>
<li><a href="https://github.com/eslint/eslint/commit/c0d2ac16f8f9c75c62c78e9fe6a24a25ba0d7828"><code>c0d2ac1</code></a> Sponsors: Sync README with website (ESLint Jenkins)</li>
<li><a href="https://github.com/eslint/eslint/commit/a8df03efe3bc47665d2112c2cdd5bead337d475d"><code>a8df03e</code></a> Docs: Clarify triage process (<a href="https://github.com/eslint/eslint/issues/14117">#14117</a>) (Nicholas C. Zakas)</li>
</ul>
</blockquote>
</details>
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a href="https://github.com/eslint/eslint/blob/master/CHANGELOG.md">eslint's changelog</a>.</em></p>
<blockquote>
<p>v7.22.0 - March 12, 2021</p>
<ul>
<li><a href="https://github.com/eslint/eslint/commit/3a432d82b3a5710aff7da20302fe0b94fedc46c2"><code>3a432d8</code></a> Docs: Improve documentation for indent rule (<a href="https://github.com/eslint/eslint/issues/14168">#14168</a>) (Serkan Özel)</li>
<li><a href="https://github.com/eslint/eslint/commit/f62ec8d30d925e70e4d0d40640857c587ac2e116"><code>f62ec8d</code></a> Update: throw error when fix range is invalid (<a href="https://github.com/eslint/eslint/issues/14142">#14142</a>) (Jacob Bandes-Storch)</li>
<li><a href="https://github.com/eslint/eslint/commit/0eecad271358f753730741fcfcb2f7cc915c1fa7"><code>0eecad2</code></a> Upgrade: Update lodash in package.json to V 4.17.21 (<a href="https://github.com/eslint/eslint/issues/14159">#14159</a>) (Basem Al-Nabulsi)</li>
<li><a href="https://github.com/eslint/eslint/commit/5ad91aa7df3d6bc185786e6eccd9e055fd951055"><code>5ad91aa</code></a> Update: report es2021 globals in no-extend-native (refs <a href="https://github.com/eslint/eslint/issues/13602">#13602</a>) (<a href="https://github.com/eslint/eslint/issues/14177">#14177</a>) (Milos Djermanovic)</li>
<li><a href="https://github.com/eslint/eslint/commit/c295581aca4e08ec4ae8e5ee5726a6f454a3ee26"><code>c295581</code></a> Chore: remove leftover JSDoc from lint-result-cache (<a href="https://github.com/eslint/eslint/issues/14176">#14176</a>) (Milos Djermanovic)</li>
<li><a href="https://github.com/eslint/eslint/commit/0d541f9d9d58966372e2055a8f69fb9483d56a4b"><code>0d541f9</code></a> Chore: Reduce lodash usage (<a href="https://github.com/eslint/eslint/issues/14178">#14178</a>) (Stephen Wade)</li>
<li><a href="https://github.com/eslint/eslint/commit/27a67d71ffa9bbd7af02ae448844e127bcf956dc"><code>27a67d7</code></a> Sponsors: Sync README with website (ESLint Jenkins)</li>
<li><a href="https://github.com/eslint/eslint/commit/459d821f4a599501ceb002f9d7a5034fc45ffbb0"><code>459d821</code></a> Chore: upgrade dependencies of browser test (<a href="https://github.com/eslint/eslint/issues/14127">#14127</a>) (Pig Fang)</li>
<li><a href="https://github.com/eslint/eslint/commit/ebfb63a682004a008f2707dbad616e5ae1630b2c"><code>ebfb63a</code></a> Sponsors: Sync README with website (ESLint Jenkins)</li>
<li><a href="https://github.com/eslint/eslint/commit/3ba029fbffd44068be93254890fc2aec3e92c212"><code>3ba029f</code></a> Docs: Remove Extraneous Dash (<a href="https://github.com/eslint/eslint/issues/14164">#14164</a>) (Danny Hurlburt)</li>
<li><a href="https://github.com/eslint/eslint/commit/6f4540ea7ea39775906526506fd7abd7ea97610c"><code>6f4540e</code></a> Sponsors: Sync README with website (ESLint Jenkins)</li>
<li><a href="https://github.com/eslint/eslint/commit/ddf361ca2a2a01a9974f421e5f62270df282d0e8"><code>ddf361c</code></a> Docs: Fix Formatting (<a href="https://github.com/eslint/eslint/issues/14154">#14154</a>) (Danny Hurlburt)</li>
<li><a href="https://github.com/eslint/eslint/commit/c0d2ac16f8f9c75c62c78e9fe6a24a25ba0d7828"><code>c0d2ac1</code></a> Sponsors: Sync README with website (ESLint Jenkins)</li>
<li><a href="https://github.com/eslint/eslint/commit/a8df03efe3bc47665d2112c2cdd5bead337d475d"><code>a8df03e</code></a> Docs: Clarify triage process (<a href="https://github.com/eslint/eslint/issues/14117">#14117</a>) (Nicholas C. Zakas)</li>
</ul>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/eslint/eslint/commit/6ee803747fd996ff3bbcea2f7adcd560eae22576"><code>6ee8037</code></a> 7.22.0</li>
<li><a href="https://github.com/eslint/eslint/commit/a55e8a1a0174aee09e406d261ccb9b2bf7449602"><code>a55e8a1</code></a> Build: changelog update for 7.22.0</li>
<li><a href="https://github.com/eslint/eslint/commit/3a432d82b3a5710aff7da20302fe0b94fedc46c2"><code>3a432d8</code></a> Docs: Improve documentation for indent rule (<a href="https://github.com/eslint/eslint/issues/14168">#14168</a>)</li>
<li><a href="https://github.com/eslint/eslint/commit/f62ec8d30d925e70e4d0d40640857c587ac2e116"><code>f62ec8d</code></a> Update: throw error when fix range is invalid (<a href="https://github.com/eslint/eslint/issues/14142">#14142</a>)</li>
<li><a href="https://github.com/eslint/eslint/commit/0eecad271358f753730741fcfcb2f7cc915c1fa7"><code>0eecad2</code></a> Upgrade: Update lodash in package.json to V 4.17.21 (<a href="https://github.com/eslint/eslint/issues/14159">#14159</a>)</li>
<li><a href="https://github.com/eslint/eslint/commit/5ad91aa7df3d6bc185786e6eccd9e055fd951055"><code>5ad91aa</code></a> Update: report es2021 globals in no-extend-native (refs <a href="https://github.com/eslint/eslint/issues/13602">#13602</a>) (<a href="https://github.com/eslint/eslint/issues/14177">#14177</a>)</li>
<li><a href="https://github.com/eslint/eslint/commit/c295581aca4e08ec4ae8e5ee5726a6f454a3ee26"><code>c295581</code></a> Chore: remove leftover JSDoc from lint-result-cache (<a href="https://github.com/eslint/eslint/issues/14176">#14176</a>)</li>
<li><a href="https://github.com/eslint/eslint/commit/0d541f9d9d58966372e2055a8f69fb9483d56a4b"><code>0d541f9</code></a> Chore: Reduce lodash usage (<a href="https://github.com/eslint/eslint/issues/14178">#14178</a>)</li>
<li><a href="https://github.com/eslint/eslint/commit/27a67d71ffa9bbd7af02ae448844e127bcf956dc"><code>27a67d7</code></a> Sponsors: Sync README with website</li>
<li><a href="https://github.com/eslint/eslint/commit/459d821f4a599501ceb002f9d7a5034fc45ffbb0"><code>459d821</code></a> Chore: upgrade dependencies of browser test (<a href="https://github.com/eslint/eslint/issues/14127">#14127</a>)</li>
<li>Additional commits viewable in <a href="https://github.com/eslint/eslint/compare/v7.21.0...v7.22.0">compare view</a></li>
</ul>
</details>
<br />

[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=eslint&package-manager=npm_and_yarn&previous-version=7.21.0&new-version=7.22.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

 ---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `dependabot rebase` will rebase this PR
- `dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `dependabot merge` will merge this PR after your CI passes on it
- `dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `dependabot cancel merge` will cancel a previously requested merge and block automerging
- `dependabot reopen` will reopen this PR if it is closed
- `dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

</details>

Pull Request resolved: #2054

Reviewed By: passy

Differential Revision: D27230363

Pulled By: priteshrnandgaonkar

fbshipit-source-id: 27ff5431b86f91b543cdc7093f8eed9c5147a489
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Sep 8, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants