fix: fix crash when `message.fix` is nullish by ntnyq · Pull Request #19168 · eslint/eslint

ntnyq · 2024-11-25T15:22:08Z

Prerequisites checklist

I have read the contributing guidelines.

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

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

No need for providing the template. So I skipped it.

What changes did you make? (Give an overview)

This PR adds a simple check in linter.source-code-fixer incasing message.fix equals to null or undefined which might cause the linter throw.

Is there anything you'd like reviewers to focus on?

There is a reproduction repo https://github.com/pany-ang/issue-eslint-config.

Or you can just remove the check, run the tests added.

netlify · 2024-11-25T15:22:28Z

✅ Deploy Preview for docs-eslint ready!

Name	Link
🔨 Latest commit	`f21a9bb`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/6755aea307c1db0008a62aa7
😎 Deploy Preview	https://deploy-preview-19168--docs-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

nzakas · 2024-11-25T16:35:48Z

Thanks for the PR. It's not clear from the changes or from you repo how to reproduce this error. Can you please provide some reproduction steps?

nzakas · 2024-12-04T22:03:45Z

@ntnyq friendly ping. Can you answer my questions above? Thanks.

ntnyq · 2024-12-05T14:24:51Z

@ntnyq friendly ping. Can you answer my questions above? Thanks.

Sorry for reply late.

I created a new repo with fewer code https://github.com/ntnyq-dev/repro-eslint-fix.

And added the repro steps in README.

ntnyq · 2024-12-05T14:30:37Z

As I tested.

When call SourceCodeFixer.applyFixes, there is a message like this:

{
      ruleId: 'format/prettier',
      severity: 2,
      message: 'Parsing error: CssSyntaxError: Unclosed block',
      line: 6,
      column: 3,
      nodeType: null,
      endLine: 1,
      endColumn: NaN,
      fix: undefined
}

It cause the crash.

nzakas · 2024-12-05T16:22:26Z

Thanks, I can reproduce the crash. For the record, here's what I see in the console:

$ npm run lint

> repro-eslint-fix@1.0.0 lint
> eslint . --fix


Oops! Something went wrong! :(

ESLint: 9.16.0

TypeError: Cannot read properties of undefined (reading 'range')
    at attemptFix (C:\Users\nzaka\projects\eslint\tmp\repro-eslint-fix\node_modules\eslint\lib\linter\source-code-fixer.js:88:27)
    at SourceCodeFixer.applyFixes (C:\Users\nzaka\projects\eslint\tmp\repro-eslint-fix\node_modules\eslint\lib\linter\source-code-fixer.js:123:17)
    at Linter.verifyAndFix (C:\Users\nzaka\projects\eslint\tmp\repro-eslint-fix\node_modules\eslint\lib\linter\linter.js:2329:43)
    at verifyText (C:\Users\nzaka\projects\eslint\tmp\repro-eslint-fix\node_modules\eslint\lib\eslint\eslint.js:329:48)
    at C:\Users\nzaka\projects\eslint\tmp\repro-eslint-fix\node_modules\eslint\lib\eslint\eslint.js:810:40

In the future, please be sure to open an issue first when reporting a bug so you can provide all of the info we need.

nzakas

Just want to clean up the test names a bit, but otherwise looks good.

ntnyq · 2024-12-05T16:55:30Z

Got it and Resolved.

Thanks for your advice.

ntnyq · 2024-12-05T17:27:08Z

Don't know why Browser Test failed in CI.

I tried run it in local and it went well.

nzakas · 2024-12-05T21:26:23Z

Don't worry about the browser test. It's pretty flaky lately.

nzakas

LGTM. Would like another review before merging.

mdjermanovic · 2024-12-06T12:32:45Z

I think this is rather a bug in the processor.

It gets a lint message without fix property:

{
    ruleId: 'format/prettier',
    severity: 2,
    message: 'Parsing error: CssSyntaxError: Missed semicolon',
    line: 4,
    column: 12,
    nodeType: null
}

but returns message with fix: undefined:

{
    ruleId: 'format/prettier',
    severity: 2,
    message: 'Parsing error: CssSyntaxError: Missed semicolon',
    line: 17,
    column: 2,
    nodeType: null,
    endLine: 1,
    endColumn: NaN,
    fix: undefined
}

Lint messages normally don't have the fix property when they're not autofixable.

mdjermanovic · 2024-12-06T13:26:45Z

Now I see that both the docs and types say that the fix property can have undefined value, so although that can happen only when processors modify messages, seems we should support it.

mdjermanovic

LGTM, thanks!

ntnyq requested a review from a team as a code owner November 25, 2024 15:22

eslint-github-bot Bot added the bug ESLint is working incorrectly label Nov 25, 2024

github-actions Bot added the core Relates to ESLint's core APIs and features label Nov 25, 2024

mdjermanovic changed the title ~~fix: fix crash when message.fix is nil~~ fix: fix crash when message.fix is nullish Nov 25, 2024

mdjermanovic added the needs info Not enough information has been provided to triage this issue label Dec 1, 2024

nzakas reviewed Dec 5, 2024

View reviewed changes

Comment thread tests/lib/linter/source-code-fixer.js Outdated

Comment thread tests/lib/linter/source-code-fixer.js Outdated

Comment thread tests/lib/linter/source-code-fixer.js Outdated

nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion and removed needs info Not enough information has been provided to triage this issue labels Dec 5, 2024

nzakas previously approved these changes Dec 5, 2024

View reviewed changes

mdjermanovic added the contributor pool label Dec 6, 2024

mdjermanovic reviewed Dec 6, 2024

View reviewed changes

Comment thread tests/lib/linter/source-code-fixer.js

Comment thread tests/lib/linter/source-code-fixer.js

ntnyq dismissed nzakas’s stale review via 13941fd December 8, 2024 14:30

ntnyq added 3 commits December 8, 2024 22:35

fix: fix crash when message.fix is nil

a6bd05c

chore: teak test case title

0642cf6

chore: update test cases

f21a9bb

mdjermanovic approved these changes Dec 9, 2024

View reviewed changes

mdjermanovic merged commit 67d683d into eslint:main Dec 9, 2024

Conversation

ntnyq commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Prerequisites checklist

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

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

Uh oh!

netlify Bot commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for docs-eslint ready!

Uh oh!

nzakas commented Nov 25, 2024

Uh oh!

nzakas commented Dec 4, 2024

Uh oh!

ntnyq commented Dec 5, 2024

Uh oh!

ntnyq commented Dec 5, 2024

Uh oh!

nzakas commented Dec 5, 2024

Uh oh!

nzakas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ntnyq commented Dec 5, 2024

Uh oh!

ntnyq commented Dec 5, 2024

Uh oh!

nzakas commented Dec 5, 2024

Uh oh!

nzakas left a comment

Choose a reason for hiding this comment

Uh oh!

mdjermanovic commented Dec 6, 2024

Uh oh!

mdjermanovic commented Dec 6, 2024

Uh oh!

Uh oh!

Uh oh!

mdjermanovic left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ntnyq commented Nov 25, 2024 •

edited

Loading

netlify Bot commented Nov 25, 2024 •

edited

Loading