Skip to content

Conversation

Tanujkanti4441
Copy link
Contributor

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:

Tell us about your environment (npx eslint --env-info):

  • Node version: latest
  • npm version: latest
  • Local ESLint version: latest
  • Global ESLint version: latest
  • Operating System: windows

What parser are you using (place an "X" next to just one item)?

[x] Default (Espree)
[ ] @typescript-eslint/parser
[ ] @babel/eslint-parser
[ ] vue-eslint-parser
[ ] @angular-eslint/template-parser
[ ] Other

Please show your full configuration:

Configuration
// eslint.config.js
export default [
  {
    rules: {
      "no-useless-assignment": "warn"
    },
  },
];

What did you do? Please include the actual source code causing the issue.\

async function fn() {
  let intermediaryValue;
  try {
    intermediaryValue = 42;
    unsafeFn();
    return { error: undefined };
  } catch {
    return { intermediaryValue };
  }
}

function unsafeFn() {
  throw new Error();
}

What did you expect to happen?
intermediaryValue won't get reported as it is used in catch block.

What actually happened? Please include the actual, raw output from ESLint.
it reports the intermediaryValue as unused variable.

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

fixes #19245

@Tanujkanti4441 Tanujkanti4441 requested a review from a team as a code owner February 11, 2025 07:56
@eslint-github-bot eslint-github-bot bot added the bug ESLint is working incorrectly label Feb 11, 2025
@github-actions github-actions bot added the rule Relates to ESLint's core rules label Feb 11, 2025
Copy link

netlify bot commented Feb 11, 2025

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit b6ad83f
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/67d9189e4209af0008a9092b
😎 Deploy Preview https://deploy-preview-19423--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
nzakas previously approved these changes Feb 11, 2025
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 @mdjermanovic to review before merging.

@mdjermanovic mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label Feb 13, 2025
@mdjermanovic
Copy link
Member

There are merge conflicts now.

@mdjermanovic
Copy link
Member

After thinking about this more, the problem is not only when variables are used in catch blocks. For example, this would still be a false positive:

/*eslint no-useless-assignment: "error" */

function fn() {
    let intermediaryValue;
    try {
        intermediaryValue = 42; // false positive
        unsafeFn();
        intermediaryValue = 55;
    } catch {
        // handle error
    }

    return intermediaryValue;
}

function unsafeFn() {
    throw new Error();
}

console.log(fn()); // 42

So, I think instead of ignoring variables that are used in catch blocks, the rule should ignore assignments in try blocks.

@Tanujkanti4441
Copy link
Contributor Author

So, I think instead of ignoring variables that are used in catch blocks, the rule should ignore assignments in try blocks.

Done! should we document it also?

@mdjermanovic
Copy link
Member

So, I think instead of ignoring variables that are used in catch blocks, the rule should ignore assignments in try blocks.

Done! should we document it also?

I think it's a good idea to add a Known Limitations section with this detail.

mdjermanovic
mdjermanovic previously approved these changes Mar 13, 2025
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! Leaving open for @nzakas to re-review.

snitin315
snitin315 previously approved these changes Mar 17, 2025
Copy link
Contributor

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

@Tanujkanti4441 Tanujkanti4441 dismissed stale reviews from snitin315 and mdjermanovic via b6ad83f March 18, 2025 06:54
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, and nice work on this.

@nzakas nzakas merged commit cc3bd00 into eslint:main Mar 18, 2025
31 checks passed
@github-project-automation github-project-automation bot moved this from Second Review Needed to Complete in Triage Mar 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly rule Relates to ESLint's core rules

Projects

Status: Complete

Development

Successfully merging this pull request may close these issues.

Bug: no-useless-assignment false positive in try-catch block

4 participants