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.

Sign up for GitHub

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

Jump to bottom

fix: fix crash when message.fix is nullish #19168

Merged
merged 3 commits into from
Dec 9, 2024
Merged

fix: fix crash when message.fix is nullish #19168

merged 3 commits into from
Dec 9, 2024

Conversation

ntnyq
Copy link
Contributor

@ntnyq ntnyq commented Nov 25, 2024 •
edited
Loading

Prerequisites checklist

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.

@ntnyq ntnyq requested a review from a team as a code owner November 25, 2024 15:22
@eslint-github-bot eslint-github-bot bot added the bug ESLint is working incorrectly label Nov 25, 2024
@github-actions github-actions bot added the core Relates to ESLint's core APIs and features label Nov 25, 2024
@netlify Netlify
Copy link

netlify bot commented Nov 25, 2024 •
edited
Loading

✅ 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...

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
Copy link
Member

nzakas commented Nov 25, 2024

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?

@mdjermanovic mdjermanovic changed the title fix: fix crash when message.fix is nil fix: fix crash when message.fix is nullish Nov 25, 2024
@mdjermanovic mdjermanovic added the needs info Not enough information has been provided to triage this issue label Dec 1, 2024
@nzakas
Copy link
Member

nzakas commented Dec 4, 2024

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

@ntnyq
Copy link
Contributor Author

ntnyq commented Dec 5, 2024

@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
Copy link
Contributor Author

ntnyq commented Dec 5, 2024

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
Copy link
Member

nzakas commented Dec 5, 2024

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
nzakas reviewed Dec 5, 2024
View reviewed changes
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.

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

tests/lib/linter/source-code-fixer.js Outdated Show resolved Hide resolved
tests/lib/linter/source-code-fixer.js Outdated Show resolved Hide resolved
tests/lib/linter/source-code-fixer.js Outdated Show resolved Hide resolved
@nzakas 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
@ntnyq
Copy link
Contributor Author

ntnyq commented Dec 5, 2024

Got it and Resolved.

Thanks for your advice.

@ntnyq
Copy link
Contributor Author

ntnyq commented Dec 5, 2024

Don't know why Browser Test failed in CI.

I tried run it in local and it went well.

@nzakas
Copy link
Member

nzakas commented Dec 5, 2024

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

nzakas
nzakas previously approved these changes Dec 5, 2024
View reviewed changes
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. Would like another review before merging.

@mdjermanovic
Copy link
Member

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
Copy link
Member

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
mdjermanovic approved these changes Dec 9, 2024
View reviewed changes
Copy link
Member

@mdjermanovic mdjermanovic 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!

@mdjermanovic mdjermanovic merged commit 67d683d into eslint:main Dec 9, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdjermanovic mdjermanovic mdjermanovic approved these changes

@nzakas nzakas nzakas left review comments

Assignees
No one assigned
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly contributor pool core Relates to ESLint's core APIs and features
Projects
Triage
Status: Complete
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ntnyq @nzakas @mdjermanovic