fix(eslint-plugin): [prefer-optional-chain] fix `ThisExpression` and `PrivateIdentifier` errors #6028

omril1 · 2022-11-17T21:02:20Z

PR Checklist

Addresses an existing open issue: fixes Bug: [prefer-optional-chain] Unexpected member property type: ThisExpression #6024
That issue was marked as accepting prs
Steps in CONTRIBUTING.md were taken

Overview

This only prevents the plugin from crashing, I tried to support the syntax as well but got into a weird behavior from the fixer reverting the fix

This reverts commit 5acc1af.

typescript-eslint · 2022-11-17T21:02:22Z

Thanks for the PR, @omril1!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

nx-cloud · 2022-11-17T21:02:23Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 37716b2. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 47 targets

Sent with 💌 from NxCloud.

netlify · 2022-11-17T21:02:25Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`37716b2`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/63d8a463626f820009c332ba
😎 Deploy Preview	https://deploy-preview-6028--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

packages/eslint-plugin/src/rules/prefer-optional-chain.ts

Josh-Cena · 2022-11-17T23:48:38Z

packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts

+    // private properties
+    'this.#a && this.#b;',
+    '!this.#a || !this.#b;',
+    'a.#foo && a.#foo.bar;',
+    '!a.#foo || !a.#foo.bar;',
+    'a.#foo?.bar;',
+    '!a.#foo?.bar;',


More test cases to consider:

foo().#a a.b.#a new A().#b (await a).#b

I have no idea why this rule is so strict about what expressions it can lint, but you can in fact use private properties on all expressions, just that some may have nonsensical behaviors (e.g. (typeof a).#a). It shouldn't crash the linter anyway.

Can I fix the rule to not intentionally throw? this seems irrational to me.
It's better to not show a suggestion than to just break and do nothing

I would love to change this rule to not intentionally throw, or at least catch the error.
The rule logic now has many combinations that could be missed and better be gracefully ignored.

@Josh-Cena @JoshuaKGoldberg @bradzacher WDYT?

Yeah as Brad said in #6028 (comment), I think the rule should no longer throw. Throwing is useful for us finding missing combinations but is inconvenient for users.

Oh, so I misunderstood his comment

packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts

Co-authored-by: Joshua Chen <sidachen2003@gmail.com>

codecov · 2022-11-18T04:45:59Z

Codecov Report

Merging #6028 (37716b2) into main (202d9fb) will decrease coverage by 0.01%.
The diff coverage is 91.66%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6028      +/-   ##
==========================================
- Coverage   91.37%   91.36%   -0.01%     
==========================================
  Files         368      368              
  Lines       12596    12605       +9     
  Branches     3709     3713       +4     
==========================================
+ Hits        11509    11516       +7     
- Misses        772      773       +1     
- Partials      315      316       +1

Flag	Coverage Δ
unittest	`91.36% <91.66%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...s/eslint-plugin/src/rules/prefer-optional-chain.ts	`97.50% <91.66%> (-1.18%)`	⬇️

JoshuaKGoldberg · 2022-11-23T22:41:12Z

Something funky is up with the Git history 🤔

bradzacher · 2022-11-24T00:19:08Z

it looks like you might have rebased weirdly or something? it's borked the PR contents!

could you please update your branch so it just contains your commits - thanks!
(feel free to force-push to fix this)

…refer-optional-chain-private-and-this-in-negated-or

omril1 · 2022-11-25T16:48:44Z

packages/eslint-plugin/src/rules/prefer-optional-chain.ts

+      if (
+        [
+          AST_NODE_TYPES.TSAsExpression,
+          AST_NODE_TYPES.AwaitExpression,
+          AST_NODE_TYPES.NewExpression,
+        ].includes(node.object.type)
+      ) {
+        return '';
+      }


This feels a bit like a hack since typescript doesn't seem to think they are assignable to MemberExpression
if (node.object.type === AST_NODE_TYPES.TSAsExpression) {} shows a typescript error, but with Array#includes it does not

It is a bit of a hack 😄 but the root cause is a bug in our typings, I think. A node's member can be an await or new. [typescript-eslint playground]

const example = {} as any; (await example).prop; (await new example).prop;

For now, you can use a // @ts-expect-error and link to ~~#6239~~ #6192

…ate-and-this-in-negated-or

JoshuaKGoldberg

Still a few comments please 😇

…ate-and-this-in-negated-or

bradzacher · 2023-01-30T01:09:48Z

fix the merge conflicts and we can get this in!
thanks for your work!

…refer-optional-chain-private-and-this-in-negated-or # Conflicts: # packages/eslint-plugin/src/rules/prefer-optional-chain.ts # packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts

omril1 · 2023-01-30T05:25:27Z

@bradzacher Thanks, fixed the conflict

…ate-and-this-in-negated-or

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.48.2` -> `5.50.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/5.48.2/5.50.0) | | [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.48.2` -> `5.50.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/5.48.2/5.50.0) | --- ### Release Notes <details> <summary>typescript-eslint/typescript-eslint (@typescript-eslint/eslint-plugin)</summary> ### [`v5.50.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#5500-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5490v5500-2023-01-31) [Compare Source](typescript-eslint/typescript-eslint@v5.49.0...v5.50.0) ##### Bug Fixes - **eslint-plugin:** \[ban-ts-comment] counts graphemes instead of `String.prototype.length` ([#5704](typescript-eslint/typescript-eslint#5704)) ([09d57ce](typescript-eslint/typescript-eslint@09d57ce)) - **eslint-plugin:** \[prefer-optional-chain] fix `ThisExpression` and `PrivateIdentifier` errors ([#6028](typescript-eslint/typescript-eslint#6028)) ([85e783c](typescript-eslint/typescript-eslint@85e783c)) - **eslint-plugin:** \[prefer-optional-chain] fixer produces wrong logic ([#5919](typescript-eslint/typescript-eslint#5919)) ([b0f6c8e](typescript-eslint/typescript-eslint@b0f6c8e)), closes [#1438](typescript-eslint/typescript-eslint#1438) ##### Features - **eslint-plugin:** add `key-spacing` rule extension for interface & type declarations ([#6211](typescript-eslint/typescript-eslint#6211)) ([67706e7](typescript-eslint/typescript-eslint@67706e7)) ### [`v5.49.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#5490-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5482v5490-2023-01-23) [Compare Source](typescript-eslint/typescript-eslint@v5.48.2...v5.49.0) ##### Features - **eslint-plugin:** \[naming-convention] add support for `#private` modifier on class members ([#6259](typescript-eslint/typescript-eslint#6259)) ([c8a6d80](typescript-eslint/typescript-eslint@c8a6d80)) #### [5.48.2](typescript-eslint/typescript-eslint@v5.48.1...v5.48.2) (2023-01-16) **Note:** Version bump only for package [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/eslint-plugin) #### [5.48.1](typescript-eslint/typescript-eslint@v5.48.0...v5.48.1) (2023-01-09) **Note:** Version bump only for package [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/eslint-plugin) </details> <details> <summary>typescript-eslint/typescript-eslint (@typescript-eslint/parser)</summary> ### [`v5.50.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#5500-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5490v5500-2023-01-31) [Compare Source](typescript-eslint/typescript-eslint@v5.49.0...v5.50.0) **Note:** Version bump only for package [@typescript-eslint/parser](https://github.com/typescript-eslint/parser) ### [`v5.49.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#5490-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5482v5490-2023-01-23) [Compare Source](typescript-eslint/typescript-eslint@v5.48.2...v5.49.0) **Note:** Version bump only for package [@typescript-eslint/parser](https://github.com/typescript-eslint/parser) #### [5.48.2](typescript-eslint/typescript-eslint@v5.48.1...v5.48.2) (2023-01-16) **Note:** Version bump only for package [@typescript-eslint/parser](https://github.com/typescript-eslint/parser) #### [5.48.1](typescript-eslint/typescript-eslint@v5.48.0...v5.48.1) (2023-01-09) **Note:** Version bump only for package [@typescript-eslint/parser](https://github.com/typescript-eslint/parser) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1757 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

omril1 added 3 commits November 17, 2022 22:53

prevent thrown error from default case

71cb502

try

5acc1af

Revert "try"

a3b6eda

This reverts commit 5acc1af.

JoshuaKGoldberg reviewed Nov 17, 2022

View reviewed changes

packages/eslint-plugin/src/rules/prefer-optional-chain.ts Outdated Show resolved Hide resolved

JoshuaKGoldberg requested a review from bradzacher November 17, 2022 23:39

JoshuaKGoldberg assigned bradzacher Nov 17, 2022

Josh-Cena reviewed Nov 17, 2022

View reviewed changes

More cases and format

a68b13a

Josh-Cena reviewed Nov 18, 2022

View reviewed changes

packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts Outdated Show resolved Hide resolved

Update packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts

7e2185e

Co-authored-by: Joshua Chen <sidachen2003@gmail.com>

bradzacher changed the title ~~fix(eslint-plugin) [prefer-optional-chain] - Fix ThisExpression and PrivateIdentifier~~ fix(eslint-plugin) [prefer-optional-chain] fix ThisExpression and PrivateIdentifier errors Nov 18, 2022

bradzacher added the bug Something isn't working label Nov 18, 2022

bradzacher changed the title ~~fix(eslint-plugin) [prefer-optional-chain] fix ThisExpression and PrivateIdentifier errors~~ fix(eslint-plugin): [prefer-optional-chain] fix ThisExpression and PrivateIdentifier errors Nov 23, 2022

bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Nov 24, 2022

omril1 force-pushed the fix/issue6024-fix-prefer-optional-chain-private-and-this-in-negated-or branch from e647994 to 7e2185e Compare November 24, 2022 04:36

omril1 added 3 commits November 24, 2022 06:39

Merge remote-tracking branch 'upstream/main' into fix/issue6024-fix-p…

1a30e94

…refer-optional-chain-private-and-this-in-negated-or

TSAsExpression,AwaitExpression,NewExpression

21cfbd2

Merge remote-tracking branch 'upstream/main' into fix/issue6024-fix-p…

0ae6c45

…refer-optional-chain-private-and-this-in-negated-or

omril1 commented Nov 25, 2022

View reviewed changes

omril1 added 2 commits December 5, 2022 16:15

Merge branch 'main' into fix/issue6024-fix-prefer-optional-chain-priv…

ecfacff

…ate-and-this-in-negated-or

Merge branch 'main' into fix/issue6024-fix-prefer-optional-chain-priv…

3ce9b0a

…ate-and-this-in-negated-or

omril1 requested review from JoshuaKGoldberg and removed request for bradzacher and JoshuaKGoldberg December 17, 2022 00:12

omril1 added 2 commits December 24, 2022 13:55

coverage

12effef

over covered case

0afc516

omril1 requested review from JoshuaKGoldberg and removed request for bradzacher December 24, 2022 12:15

JoshuaKGoldberg requested changes Dec 24, 2022

View reviewed changes

Clearly, we had some missing coverage 😄

8efa51d

omril1 requested a review from JoshuaKGoldberg December 24, 2022 17:00

this got uglier

57b86c1

omril1 requested review from bradzacher and removed request for JoshuaKGoldberg December 24, 2022 18:36

omril1 added 4 commits December 25, 2022 21:32

fix coverage

99528f0

Merge branch 'main' into fix/issue6024-fix-prefer-optional-chain-priv…

cbb5168

…ate-and-this-in-negated-or

Merge branch 'main' into fix/issue6024-fix-prefer-optional-chain-priv…

b6d265c

…ate-and-this-in-negated-or

Merge branch 'main' into fix/issue6024-fix-prefer-optional-chain-priv…

54d4664

…ate-and-this-in-negated-or

omril1 requested review from JoshuaKGoldberg and removed request for bradzacher January 13, 2023 21:37

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Jan 19, 2023

bradzacher previously approved these changes Jan 30, 2023

View reviewed changes

bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Jan 30, 2023

omril1 dismissed bradzacher’s stale review via ac8b6a2 January 30, 2023 05:06

omril1 added 2 commits January 30, 2023 10:04

Merge branch 'main' into fix/issue6024-fix-prefer-optional-chain-priv…

a2a6314

…ate-and-this-in-negated-or

Merge branch 'main' into fix/issue6024-fix-prefer-optional-chain-priv…

37716b2

…ate-and-this-in-negated-or

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Jan 31, 2023

bradzacher merged commit 85e783c into typescript-eslint:main Jan 31, 2023

omril1 deleted the fix/issue6024-fix-prefer-optional-chain-private-and-this-in-negated-or branch January 31, 2023 07:43

github-actions bot locked as resolved and limited conversation to collaborators Feb 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(eslint-plugin): [prefer-optional-chain] fix `ThisExpression` and `PrivateIdentifier` errors #6028

fix(eslint-plugin): [prefer-optional-chain] fix `ThisExpression` and `PrivateIdentifier` errors #6028

omril1 commented Nov 17, 2022

typescript-eslint bot commented Nov 17, 2022

nx-cloud bot commented Nov 17, 2022 •

edited

netlify bot commented Nov 17, 2022 •

edited

Josh-Cena Nov 17, 2022 •

edited

omril1 Nov 18, 2022

omril1 Dec 17, 2022

JoshuaKGoldberg Dec 17, 2022

omril1 Dec 18, 2022

codecov bot commented Nov 18, 2022 •

edited

JoshuaKGoldberg commented Nov 23, 2022

bradzacher commented Nov 24, 2022

omril1 Nov 25, 2022

JoshuaKGoldberg Dec 17, 2022 •

edited

JoshuaKGoldberg left a comment

bradzacher commented Jan 30, 2023

omril1 commented Jan 30, 2023

fix(eslint-plugin): [prefer-optional-chain] fix ThisExpression and PrivateIdentifier errors #6028

fix(eslint-plugin): [prefer-optional-chain] fix ThisExpression and PrivateIdentifier errors #6028

Conversation

omril1 commented Nov 17, 2022

PR Checklist

Overview

typescript-eslint bot commented Nov 17, 2022

nx-cloud bot commented Nov 17, 2022 • edited

☁️ Nx Cloud Report

netlify bot commented Nov 17, 2022 • edited

✅ Deploy Preview for typescript-eslint ready!

Josh-Cena Nov 17, 2022 • edited

Choose a reason for hiding this comment

omril1 Nov 18, 2022

Choose a reason for hiding this comment

omril1 Dec 17, 2022

Choose a reason for hiding this comment

JoshuaKGoldberg Dec 17, 2022

Choose a reason for hiding this comment

omril1 Dec 18, 2022

Choose a reason for hiding this comment

codecov bot commented Nov 18, 2022 • edited

Codecov Report

JoshuaKGoldberg commented Nov 23, 2022

bradzacher commented Nov 24, 2022

omril1 Nov 25, 2022

Choose a reason for hiding this comment

JoshuaKGoldberg Dec 17, 2022 • edited

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

bradzacher commented Jan 30, 2023

omril1 commented Jan 30, 2023

fix(eslint-plugin): [prefer-optional-chain] fix `ThisExpression` and `PrivateIdentifier` errors #6028

fix(eslint-plugin): [prefer-optional-chain] fix `ThisExpression` and `PrivateIdentifier` errors #6028

nx-cloud bot commented Nov 17, 2022 •

edited

netlify bot commented Nov 17, 2022 •

edited

Josh-Cena Nov 17, 2022 •

edited

codecov bot commented Nov 18, 2022 •

edited

JoshuaKGoldberg Dec 17, 2022 •

edited