sindresorhus / eslint-plugin-unicorn

More than 100 powerful ESLint rules
MIT License
3.98k stars 360 forks source link

Rule proposal: `prefer-optional-chaining` #429

Open jorgegonzalez opened 4 years ago

jorgegonzalez commented 4 years ago

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining

// first and second may be null or undefined:
let nestedProp = obj.first && obj.first.second;

// The line above should be transformed to this:
let nestedProp = obj.first?.second;
yakov116 commented 4 years ago

@jorgegonzalez Experimental. Expect behavior to change in the future. maybe in a year or 2

jorgegonzalez commented 4 years ago

It's in Stage 3 and already landed in Babel and TypeScript 🤷‍♂

fisker commented 4 years ago

why close? I think its great, and already using

sindresorhus commented 4 years ago

Big 👍, but we have to wait for ESLint to add support for it in their parser first.

jorgegonzalez commented 4 years ago

https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/prefer-optional-chain.md

jorgegonzalez commented 4 years ago

https://regex101.com/r/DiaDQr/3/

fisker commented 4 years ago

Optional chaining is now stage 4!

teppeis commented 4 years ago

Released in https://github.com/sindresorhus/eslint-plugin-unicorn/releases/tag/v20.0.0

sindresorhus commented 4 years ago

@teppeis No

teppeis commented 4 years ago

Oh, sorry...

horacio-penya commented 4 years ago

https://github.com/horape3/eslint-plugin-prefer-optional-chaining

EvgenyOrekhov commented 3 years ago

Support for optional chaining was added in ESLint 7.5.0.

Also, @typescript-eslint/prefer-optional-chain can be used in JavaScript projects. @typescript-eslint/prefer-optional-chain is not autofixable though.

Interesting fact: quite a few of TypeScript rules can be used in JavaScript projects, see https://github.com/EvgenyOrekhov/eslint-config-hardcore#hardcorets-for-js.

fregante commented 3 years ago

Why not close this since @typescript-eslint/prefer-optional-chain exists?

EvgenyOrekhov commented 3 years ago

@fregante

  1. @typescript-eslint rule doesn't provide an autofix.
  2. @typescript-eslint is usually used for linting TypeScript, whereas eslint-plugin-unicorn targets mostly JavaScript users.

As a side note, I recently discovered this project: https://github.com/coderaiser/putout. It has an ESLint plugin, and it provides dozens of autofixes (some of them are quite advanced), including plugin-apply-optional-chaining.

fregante commented 3 years ago
  • @typescript-eslint rule doesn't provide an autofix.

If safe, it could be added there, why write a whole rule from scratch?

@typescript-eslint is usually used for linting TypeScript

But as you mentioned it could be used on JS files as well.


Edit: I tried putout on Refined GitHub and mostly it just broke code and dropped whole lines of very clear code without any reason. I wouldn't trust it. Astounding 90% unwanted/breaking change rate.

sindresorhus commented 3 years ago

Seems like ESLint will add this rule at some point, so there's no point in adding it here. https://github.com/eslint/eslint/issues/13430

bradzacher commented 3 years ago

There is a reason that the @typescript-eslint rule no longer has an autofix. It is not possible to have a rule that autofixes to optional chaining safely[1].

You can only detect these cases with a type-aware lint rule.

For example:

function foo(arg) {
  return arg && arg.foo;
}

Should this be converted into an optional chain? Seems like it... until you add more information about the expected types:

function foo(arg: false | { foo: string }) {
  return arg && arg.foo;
}

foo(false);
foo({ foo: 'a' });

Now it is no longer correct to make this an optional chain. In a pure JS world - this is technically a "safe" change (nothing will throw at runtime), but it changes the return value of the function from string | false to string | undefined. In a TS codebase - this change would also break the build, because .foo does not exist on the type false.

Is this code a weird pattern? For sure. Is it valid code that 100% exists in the real world? Definitely.

There are numerous other examples that you could dig out of the issues filed on https://github.com/typescript-eslint/typescript-eslint.


[1] To clarify for people - an ESLint autofixer must be safe - meaning it mustn't break the build or change runtime semantics of code.

This is why the @typescript-eslint rule uses a suggestion fixer. Unlike autofixers, suggestion fixers can produce "broken" code because there's no way to auto-apply suggestion fixes -- they can only be manually applied by an engineer.

The @typescript-eslint rule originally had an autofixer - but many users complained about broken code because of it.

fregante commented 3 years ago

Yeah in this case the correct auto fix would be arg?.foo ?? false, which defeats the point of using the operator and confuses people about what the solution should be.

Samuel-Therrien-Beslogic commented 5 months ago

Copied from https://github.com/sindresorhus/eslint-plugin-unicorn/issues/2230 Some more optional chaining not mentioned in OP:

Fail

foo === undefined ? foo : foo.bar // Autofix to `foo?.bar`
foo == undefined ? undefined : foo.bar // Autofix to `foo?.bar`
foo == null ? undefined : foo.bar // Autofix to `foo?.bar`

Pass

foo == null ? foo : bar
foo == undefined ? foo : bar
// Technically these can change the return type from `null` to `undefined`. So I'm not sure if they should pass or fail with suggestion
foo == undefined ? foo : foo.bar // Suggests `foo?.bar`
foo == null ? foo : foo.bar // Suggests `foo?.bar`

Additional Info

https://github.com/sindresorhus/eslint-plugin-unicorn/blob/v46.0.1/docs/rules/no-negated-condition.md already takes cares of the != cases so I didn't list them here.

ts(5076) prevents mixing &&/|| with ?? without parenthesis. So some autofix might require adding parenthesis.

bradzacher commented 5 months ago

https://typescript-eslint.io/rules/prefer-optional-chain/

Note that since this issue in v6 of our plugin we did a massive refactoring of the rule. It now uses type information because users found that there were simply too many false positives.

By using type information it means the rule can also offer safe autofixes.

My 2c: this issue should be closed because there's no way that this plugin can provide a syntax-only rule that offers reports without false positives or an autofixer that won't change runtime behaviour.