feat: treat `this` in `typeof this` as a `ThisExpression` #4382

Zzzen · 2022-01-01T05:48:08Z

PR Checklist

Addresses an existing issue: fixes [no-undef] this in typeof this is reported as undefined incorrectly #4364
That issue was marked as accepting prs
Steps in CONTRIBUTING.md were taken

Overview

nx-cloud · 2022-01-01T05:48:10Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 042c6f2. 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.

typescript-eslint · 2022-01-01T05:48:12Z

Thanks for the PR, @Zzzen!

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. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

netlify · 2022-01-01T05:48:15Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`042c6f2`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/62b8c0c73eac3f00097efc73
😎 Deploy Preview	https://deploy-preview-4382--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.

codecov · 2022-01-01T06:03:24Z

Codecov Report

Merging #4382 (042c6f2) into main (2600f7f) will increase coverage by 1.55%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #4382      +/-   ##
==========================================
+ Coverage   91.33%   92.88%   +1.55%     
==========================================
  Files         132      323     +191     
  Lines        1488    10241    +8753     
  Branches      224     2992    +2768     
==========================================
+ Hits         1359     9512    +8153     
- Misses         65      419     +354     
- Partials       64      310     +246

Flag	Coverage Δ
unittest	`92.88% <100.00%> (+1.55%)`	⬆️

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

Impacted Files	Coverage Δ
...kages/scope-manager/src/referencer/ClassVisitor.ts	`94.01% <100.00%> (+0.05%)`	⬆️
...ckages/scope-manager/src/referencer/TypeVisitor.ts	`97.93% <100.00%> (+0.04%)`	⬆️
...kages/eslint-plugin/src/rules/no-dynamic-delete.ts	`100.00% <0.00%> (ø)`
...ackages/eslint-plugin/src/rules/no-for-in-array.ts	`100.00% <0.00%> (ø)`
packages/eslint-plugin/src/rules/no-shadow.ts	`78.48% <0.00%> (ø)`
...ackages/eslint-plugin/src/rules/no-extra-parens.ts	`90.41% <0.00%> (ø)`
...es/eslint-plugin/src/rules/no-floating-promises.ts	`100.00% <0.00%> (ø)`
packages/eslint-plugin/src/configs/base.ts	`100.00% <0.00%> (ø)`
...s/eslint-plugin/src/rules/no-unused-expressions.ts	`89.47% <0.00%> (ø)`
...ages/eslint-plugin/src/rules/no-require-imports.ts	`100.00% <0.00%> (ø)`
... and 183 more

Zzzen · 2022-01-02T15:45:57Z

packages/typescript-estree/src/convert.ts

@@ -787,6 +788,14 @@ export class Converter {
      }

      case SyntaxKind.Identifier: {
+        if (isThisInTypeQuery(node)) {


Changing the AST feels hacky and is gonna break some plugins. The no-undef rule uses globalScope.through to check undefined variables. Creating a variable called this for functions should prevent eslint from reporting this.

https://github.com/eslint/eslint/blob/main/lib/rules/no-undef.js#L59L75

this looks like an issue in typescript, we should either get SyntaxKind.ThisType or SyntaxKind.ThisKeyword

playground 4.3.5
playground 4.5.4

class Test { fn () { const a: typeof this = this a.fn() const b: this = this b.fn() } }

const b: this = this - is correctly reported as ThisType

older versions of typescript (<4.4) reported typeof this as error Cannot find name 'this'. (2304)

we should add test cases for parser and visitor

bradzacher · 2022-01-03T19:03:39Z

Changing the AST feels hacky

It's not hacky because it's the correct AST.
this is not a random Identifier - it's a ThisExpression.
The change could break some plugins, but the number of rules that specifically inspect TSQualifiedName are going to be few (it's not a common thing to lint against). So breakages should be really sparse.
Those that do break will be easily fixed as the updated types will highlight the exact breakage.

Defining a global variable for this is a hacky workaround which could cause other problems.
For example - I'd guess that in script mode you'll now find that function foo(this: string) {} will cause a no-shadow error because you're shadowing the global variable this.

Zzzen · 2022-01-04T16:10:47Z

Did not notice we have our own definition for all AST nodes...

packages/typescript-estree/src/convert.ts

armano2 · 2022-01-07T14:28:44Z

packages/typescript-estree/src/node-utils.ts

+  return id.originalKeywordKind === SyntaxKind.ThisKeyword;
+}
+
+export function isThisIdentifier(node: ts.Node | undefined): boolean {


you should use TSNode from ts-estree type and remove casts

you should use a type predicate so that TS can understand the refinement you're making within the function

Suggested change

export function isThisIdentifier(node: ts.Node | undefined): boolean {

export function isThisIdentifier(node: ts.Node | undefined): node is ts.ThisKeyword {

ts does not have nodes named ThisKeyword. The most similar thing is ThisExpression. Probably not what we want.

export interface ThisExpression extends PrimaryExpression { readonly kind: SyntaxKind.ThisKeyword; }

bradzacher

please add a parser test for this as well.

you can add a fixture somewhere in here packages/shared-fixtures/fixtures/typescript/types

then run the test with

$ cd packages/typescript-estree
$ yarn jest ast-fixtures.test

It'll dump a snapshot to packages/typescript-estree/tests/snapshots/typescript/types

bradzacher · 2022-01-24T19:25:51Z

packages/typescript-estree/src/node-utils.ts

+  );
+}
+
+export function isThisInTypeQuery(node: ts.Node): boolean {


ditto above - use a predicate return type

JoshuaKGoldberg · 2022-04-24T21:43:00Z

👋 @Zzzen, this PR would be nice to have. Do you still have time to work on it? (no worries if not, just checking!)

Zzzen · 2022-04-25T13:47:07Z

Oh, sure! I almost forgot it.

Zzzen · 2022-04-27T16:37:00Z

We have converted the AST, so it doesn't align with babel. Should we update preprocessBabylonAST too?

FAIL  tests/ast-alignment/spec.ts
  ● /home/runner/work/typescript-eslint/typescript-eslint/packages/shared-fixtures/fixtures/typescript/types/typeof-this.src.ts

    expect(received).toEqual(expected) // deep equality

    - Expected  - 2
    + Received  + 4

    @@ -46,15 +46,16 @@
                          "start": Object {
                            "column": 17,
                            "line": 1,
                          },
                        },
    +                   "name": "this",
                        "range": Array [
                          17,
                          21,
                        ],
    -                   "type": "ThisExpression",
    +                   "type": "Identifier",
                      },
                      "loc": Object {
                        "end": Object {
                          "column": 21,
                          "line": 1,
    @@ -154,15 +155,16 @@
                            "start": Object {
                              "column": 16,
                              "line": 2,
                            },
                          },
    +                     "name": "this",
                          "range": Array [
                            39,
                            43,
                          ],
    -                     "type": "ThisExpression",
    +                     "type": "Identifier",
                        },
                        "loc": Object {
                          "end": Object {
                            "column": [24](https://github.com/typescript-eslint/typescript-eslint/runs/6177792710?check_suite_focus=true#step:10:24),
                            "line": 2,

      82 |         fixture.ignoreSourceType,
      83 |       ),
    > 84 |     ).toEqual(
         |       ^
      85 |       removeLocationDataAndSourceTypeFromProgramNode(
      86 |         preprocessTypescriptAST(typeScriptESTreeResult.ast),
      87 |         fixture.ignoreSourceType,

      at Object.<anonymous> (tests/ast-alignment/spec.ts:84:7)

armano2 · 2022-05-14T06:35:39Z

We have converted the AST, so it doesn't align with babel. Should we update preprocessBabylonAST too?

@Zzzen yes

armano2 · 2022-05-28T16:02:06Z

packages/typescript-estree/tests/ast-alignment/utils.ts

@@ -295,6 +295,27 @@ export function preprocessBabylonAST(ast: File): any {
          delete node.loc.start.index;
        }
      },
+      TSTypeQuery(node: TSTypeQuery) {


can you add an comment with explanation why we need this and if there is babel ticket please link it here,
(this is necessary to ease maintenance of this file)

bradzacher

LGTM! thanks for all your work on this!

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.29.0` -> `5.30.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/5.29.0/5.30.0) | | [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.29.0` -> `5.30.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/5.29.0/5.30.0) | --- ### Release Notes <details> <summary>typescript-eslint/typescript-eslint (@typescript-eslint/eslint-plugin)</summary> ### [`v5.30.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#5300-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5290v5300-2022-06-27) [Compare Source](typescript-eslint/typescript-eslint@v5.29.0...v5.30.0) ##### Features - **eslint-plugin:** \[no-shadow] add shadowed variable location to the error message ([#5183](typescript-eslint/typescript-eslint#5183)) ([8ca08e9](typescript-eslint/typescript-eslint@8ca08e9)) - treat `this` in `typeof this` as a `ThisExpression` ([#4382](typescript-eslint/typescript-eslint#4382)) ([b04b2ce](typescript-eslint/typescript-eslint@b04b2ce)) </details> <details> <summary>typescript-eslint/typescript-eslint (@typescript-eslint/parser)</summary> ### [`v5.30.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#5300-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5290v5300-2022-06-27) [Compare Source](typescript-eslint/typescript-eslint@v5.29.0...v5.30.0) **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, click this checkbox. --- 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/1436 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>

fix: support typeof this

3caf3c5

Zzzen commented Jan 2, 2022

View reviewed changes

bradzacher added awaiting response Issues waiting for a reply from the OP or another party bug Something isn't working labels Jan 3, 2022

fix: add ThisExpression to EntityName

ae25168

Zzzen force-pushed the typeof-this branch from 5b1ba66 to ae25168 Compare January 4, 2022 16:02

armano2 reviewed Jan 7, 2022

View reviewed changes

packages/typescript-estree/src/convert.ts Outdated Show resolved Hide resolved

armano2 reviewed Jan 7, 2022

View reviewed changes

bradzacher requested changes Jan 7, 2022

View reviewed changes

bradzacher reviewed Jan 24, 2022

View reviewed changes

Zzzen added 4 commits April 25, 2022 22:33

fix: add parser tests

1a6ef43

fix: prefer type predicate

1680e54

Merge remote-tracking branch 'origin/main' into typeof-this

92da63c

fix: update snapshot

fdbc897

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Apr 25, 2022

fix: update baseline

c924ee3

armano2 modified the milestone: 6.0.0 May 15, 2022

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

Merge branch 'main' into typeof-this

3aa666e

Zzzen added 2 commits May 28, 2022 21:39

Merge remote-tracking branch 'origin/main' into typeof-this

204c0bb

test: transform babel ast

3d09e4d

armano2 reviewed May 28, 2022

View reviewed changes

Zzzen added 3 commits June 5, 2022 16:14

test: fix CI error

b03c226

Merge branch 'main' into typeof-this

b699f0c

test: refactor

fcbaa5d

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Jun 13, 2022

Zzzen force-pushed the typeof-this branch from ece3642 to fcbaa5d Compare June 17, 2022 14:21

Zzzen and others added 6 commits June 17, 2022 22:23

Merge branch 'main' into typeof-this

751fd94

fix merge error

df847c1

refactor

a60ceee

refactor

b9f12b9

apply armano's suggested node type change

d42ecff

Merge branch 'main' into typeof-this

042c6f2

bradzacher changed the title ~~fix: support typeof this~~ feat: treat this in typeof this as a ThisExpression Jun 26, 2022

bradzacher added enhancement New feature or request and removed bug Something isn't working labels Jun 26, 2022

bradzacher approved these changes Jun 26, 2022

View reviewed changes

bradzacher merged commit b04b2ce into typescript-eslint:main Jun 27, 2022

github-actions bot locked as resolved and limited conversation to collaborators Jul 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: treat `this` in `typeof this` as a `ThisExpression` #4382

feat: treat `this` in `typeof this` as a `ThisExpression` #4382

Zzzen commented Jan 1, 2022

nx-cloud bot commented Jan 1, 2022 •

edited

Loading

typescript-eslint bot commented Jan 1, 2022

netlify bot commented Jan 1, 2022 •

edited

Loading

codecov bot commented Jan 1, 2022 •

edited

Loading

Zzzen Jan 2, 2022

armano2 Jan 6, 2022 •

edited

Loading

bradzacher commented Jan 3, 2022 •

edited

Loading

Zzzen commented Jan 4, 2022

armano2 Jan 7, 2022 •

edited

Loading

bradzacher Jan 24, 2022 •

edited

Loading

Zzzen Apr 25, 2022

bradzacher left a comment

bradzacher Jan 24, 2022

JoshuaKGoldberg commented Apr 24, 2022

Zzzen commented Apr 25, 2022

Zzzen commented Apr 27, 2022

armano2 commented May 14, 2022 •

edited

Loading

armano2 May 28, 2022 •

edited

Loading

bradzacher left a comment

	export function isThisIdentifier(node: ts.Node \| undefined): boolean {
	export function isThisIdentifier(node: ts.Node \| undefined): node is ts.ThisKeyword {

feat: treat this in typeof this as a ThisExpression #4382

feat: treat this in typeof this as a ThisExpression #4382

Conversation

Zzzen commented Jan 1, 2022

PR Checklist

Overview

nx-cloud bot commented Jan 1, 2022 • edited Loading

☁️ Nx Cloud Report

typescript-eslint bot commented Jan 1, 2022

netlify bot commented Jan 1, 2022 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

codecov bot commented Jan 1, 2022 • edited Loading

Codecov Report

Zzzen Jan 2, 2022

Choose a reason for hiding this comment

armano2 Jan 6, 2022 • edited Loading

Choose a reason for hiding this comment

bradzacher commented Jan 3, 2022 • edited Loading

Zzzen commented Jan 4, 2022

armano2 Jan 7, 2022 • edited Loading

Choose a reason for hiding this comment

bradzacher Jan 24, 2022 • edited Loading

Choose a reason for hiding this comment

Zzzen Apr 25, 2022

Choose a reason for hiding this comment

bradzacher left a comment

Choose a reason for hiding this comment

bradzacher Jan 24, 2022

Choose a reason for hiding this comment

JoshuaKGoldberg commented Apr 24, 2022

Zzzen commented Apr 25, 2022

Zzzen commented Apr 27, 2022

armano2 commented May 14, 2022 • edited Loading

armano2 May 28, 2022 • edited Loading

Choose a reason for hiding this comment

bradzacher left a comment

Choose a reason for hiding this comment

feat: treat `this` in `typeof this` as a `ThisExpression` #4382

feat: treat `this` in `typeof this` as a `ThisExpression` #4382

nx-cloud bot commented Jan 1, 2022 •

edited

Loading

netlify bot commented Jan 1, 2022 •

edited

Loading

codecov bot commented Jan 1, 2022 •

edited

Loading

armano2 Jan 6, 2022 •

edited

Loading

bradzacher commented Jan 3, 2022 •

edited

Loading

armano2 Jan 7, 2022 •

edited

Loading

bradzacher Jan 24, 2022 •

edited

Loading

armano2 commented May 14, 2022 •

edited

Loading

armano2 May 28, 2022 •

edited

Loading