Content-Length: 343093 | pFad | http://github.com/eslint/eslint/pull/19423

C1 fix: reporting variable used in catch block in `no-useless-assignment` by Tanujkanti4441 · Pull Request #19423 · eslint/eslint · GitHub
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

fix: reporting variable used in catch block in no-useless-assignment #19423

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

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 2744b35
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/67b586aa5701ff0008ca9752
😎 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
Comment on lines 563 to 568
while (currentNode) {
if (currentNode.type === "CatchClause") {
return true;
}
currentNode = currentNode.parent;
}
Copy link
Member

Choose a reason for hiding this comment

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

This search up to the root increases the timing for this rule by around 20% when linting this project. Can we maybe store CatchClause nodes in scopeStack, and at some later point check whether a reference is in the range of some of them?

Copy link
Contributor Author

@Tanujkanti4441 Tanujkanti4441 Feb 18, 2025

Choose a reason for hiding this comment

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

I tried this:

CatchClause(node) {
     scopeStack.catchClauses.add(node);
},

But seems it's not storing the catchClause nodes that are unreachable is it the intended behavior?

Copy link
Member

Choose a reason for hiding this comment

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

Can you push the changes with a test case that doesn't work due to this so I can take a look?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

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: Second Review Needed
Development

Successfully merging this pull request may close these issues.

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








ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: http://github.com/eslint/eslint/pull/19423

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy