Skip to content

prefer-optional-chain autofix is not safe #1820

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

Closed
jasonHzq opened this issue Mar 30, 2020 · 4 comments · Fixed by #1965
Closed

prefer-optional-chain autofix is not safe #1820

jasonHzq opened this issue Mar 30, 2020 · 4 comments · Fixed by #1965
Labels
bug Something isn't working good first issue Good for newcomers has pr there is a PR raised to close this package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@jasonHzq
Copy link

jasonHzq commented Mar 30, 2020

The autofix of rule "prefer-optional-chain" in eslint-plugin is not safe in this case:

source code:

// foo is null here
const value = foo && foo.value;

// value is null in source code
if (value === null) {
  // code here
}

autofixed code:

// foo is null here
const value = foo?.value;

// value will be undefined in autofix code
if (value === null) {
  // code here
}
@jasonHzq jasonHzq added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Mar 30, 2020
@bradzacher bradzacher added bug Something isn't working good first issue Good for newcomers and removed triage Waiting for team members to take a look labels Mar 30, 2020
@bradzacher
Copy link
Member

Happy to accept a PR to add an option to convert the fixer to a suggestion fixer.


My general suggestion is to avoid === null in favour of == null, because == null catches both null and undefined.
It's very rare that you want to handle the two cases differently, so this is the better check to use.

This is also very easy to enforce with the base eqeqeq rule:

"eqeqeq": ["error", "always", { "null": "never" }],

@jasonHzq
Copy link
Author

I think autofix should be completely logical equality. Shall autofix only open in IfStatement. Or if the optional chains is in 'Assign Expression'(BinarayExpression with EqualsToken), the autofix should be closed.

@jasonHzq
Copy link
Author

jasonHzq commented Mar 30, 2020

Set both the the base eqeqeq rule to { "null": "never" } and optional chains cannot completely logical equality too. If there is a npm package use optional chains, all the projects who use the npm should fix the "eqeqeq never null" problems. It's dangerous.

@bradzacher
Copy link
Member

It's only as dangerous as your types are.
If the module boundaries are poorly typed, then yes, it's dangerous.
But if it's strictly typed, then it's not dangerous, as typescript will fail your build.

The fixer in its current form may or may not cause type errors, depending on your codebase and the checks that are done on your code.

A suggestion fixer will let people manually apply the fix in their IDE, and will not let it be fixed automatically via --fix. This lets the developer choose when they want to correct the error automatically, and when they want to manually correct it.

@bradzacher bradzacher added the has pr there is a PR raised to close this label May 2, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working good first issue Good for newcomers has pr there is a PR raised to close this package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
2 participants