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

[prefer-optional-chain] Suggest (foo || {}).bar as a valid match for the rule #4395

Closed
3 tasks done
omril1 opened this issue Jan 6, 2022 · 4 comments · Fixed by #4430
Closed
3 tasks done

[prefer-optional-chain] Suggest (foo || {}).bar as a valid match for the rule #4395

omril1 opened this issue Jan 6, 2022 · 4 comments · Fixed by #4430
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@omril1
Copy link
Contributor

omril1 commented Jan 6, 2022

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have read the FAQ and my problem is not listed.

Repro

{
  "rules": {
    "@typescript-eslint/prefer-optional-chain": "error",
  }
}
const foo = (bar || {}).baz;
// or
const foo = (bar|| {})[baz];
// or
const foo = (((a || {}).b || {}).c || {}).d;

Expected Result
The rule should suggest to use optional chaining

I'm working on a codebase that uses this pattern a lot (about ~60% of the cases I refactor to optional chaining arise from this pattern, the others come from &&s) and I'm only getting prompted for the && cases.

I expect to get the same warnings/errors/suggestions the && case have, not including function calls

Actual Result
No error or warning

Additional Info
Willing to add a PR if needed
Tested this already against existing code in this repo

Versions

package version
@typescript-eslint/eslint-plugin 5.9.0
@typescript-eslint/parser 5.9.0
TypeScript 4.5.5
ESLint 8.1.0
node 14.17.5
@omril1 omril1 added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Jan 6, 2022
@bradzacher bradzacher added accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule and removed triage Waiting for maintainers to take a look labels Jan 6, 2022
@glen-84
Copy link
Contributor

glen-84 commented Mar 27, 2022

I apologize for commenting on a closed issue, but I didn't want to open a new one if I may be missing something obvious.

Optional chaining only handles null and undefined, whereas logical OR handles any falsy value, which means that the suggested fix can result in invalid code.

Example

@bradzacher
Copy link
Member

bradzacher commented Mar 27, 2022

This is a known issue with the rule in general, and is not limited to just this one case.

For example:

declare const x: {foo: false | {bar: string}};

x.foo && x.foo.bar

The rule will error and suggest fix this, but it is wrong.

Suggestion fixers are allowed to be wrong though. That's the point of having them as opposed to an automatic fixer - which must always be correct.

This is just a known weakness in the rule, unfortunately, as it doesn't use type information.

@omril1
Copy link
Contributor Author

omril1 commented Mar 27, 2022

@glen-84 This weakness in the rule also exists in the && suggestion, and it's documented as a note.

**Note:** there are a few edge cases where this rule will false positive. Use your best judgement when evaluating reported errors.

@glen-84
Copy link
Contributor

glen-84 commented Mar 27, 2022

Ah, sorry for missing that, and thanks for the explanation. 👍

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
3 participants