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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 19 commits into from Jun 27, 2022
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
3caf3c5
fix: support `typeof this`
Zzzen Jan 1, 2022
ae25168
fix: add ThisExpression to EntityName
Zzzen Jan 4, 2022
1a6ef43
fix: add parser tests
Zzzen Apr 25, 2022
1680e54
fix: prefer type predicate
Zzzen Apr 25, 2022
92da63c
Merge remote-tracking branch 'origin/main' into typeof-this
Zzzen Apr 25, 2022
fdbc897
fix: update snapshot
Zzzen Apr 25, 2022
c924ee3
fix: update baseline
Zzzen Apr 26, 2022
3aa666e
Merge branch 'main' into typeof-this
bradzacher May 18, 2022
204c0bb
Merge remote-tracking branch 'origin/main' into typeof-this
Zzzen May 28, 2022
3d09e4d
test: transform babel ast
Zzzen May 28, 2022
b03c226
test: fix CI error
Zzzen Jun 5, 2022
b699f0c
Merge branch 'main' into typeof-this
Zzzen Jun 5, 2022
fcbaa5d
test: refactor
Zzzen Jun 5, 2022
751fd94
Merge branch 'main' into typeof-this
Zzzen Jun 17, 2022
df847c1
fix merge error
Zzzen Jun 17, 2022
a60ceee
refactor
Zzzen Jun 17, 2022
b9f12b9
refactor
Zzzen Jun 18, 2022
d42ecff
apply armano's suggested node type change
bradzacher Jun 26, 2022
042c6f2
Merge branch 'main' into typeof-this
bradzacher Jun 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions packages/eslint-plugin/tests/eslint-rules/no-undef.test.ts
View file
Open in desktop
Expand Up @@ -248,6 +248,15 @@ class Foo {}
`
export type AppState = typeof import('./src/store/reducers').default;
`,
`
const obj = {
foo: '',
bar() {
let self: typeof this;
let foo: typeof this.foo;
},
};
`,
],
invalid: [
{
Expand Down
11 changes: 10 additions & 1 deletion packages/scope-manager/src/referencer/TypeVisitor.ts
View file
Open in desktop
Expand Up @@ -258,11 +258,20 @@ class TypeVisitor extends Visitor {
protected TSTypeQuery(node: TSESTree.TSTypeQuery): void {
if (node.exprName.type === AST_NODE_TYPES.Identifier) {
this.#referencer.currentScope().referenceValue(node.exprName);
} else if (
(node.exprName.type as unknown) === AST_NODE_TYPES.ThisExpression
) {
// technically exprName is either Identifier or QualifiedName, but eslint does not recognize `typeof this`,
// so we have translated it to ThisExpression.
return;
} else {
let expr = node.exprName.left;
while (expr.type !== AST_NODE_TYPES.Identifier) {
while (expr.type === TSESTree.AST_NODE_TYPES.TSQualifiedName) {
expr = expr.left;
}
if ((expr.type as unknown) === AST_NODE_TYPES.ThisExpression) {
return;
}
this.#referencer.currentScope().referenceValue(expr);
}
}
Expand Down
9 changes: 9 additions & 0 deletions packages/typescript-estree/src/convert.ts
View file
Open in desktop
Expand Up @@ -20,6 +20,7 @@ import {
isComputedProperty,
isESTreeClassMember,
isOptional,
isThisInTypeQuery,
TSError,
unescapeStringLiteralText,
} from './node-utils';
Expand Down Expand Up @@ -787,6 +788,14 @@ export class Converter {
}

case SyntaxKind.Identifier: {
if (isThisInTypeQuery(node)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@armano2 armano2 Jan 6, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return this.createNode<TSESTree.ThisExpression>(
node as unknown as ts.ThisExpression,
bradzacher marked this conversation as resolved.
Show resolved Hide resolved
{
type: AST_NODE_TYPES.ThisExpression,
},
);
}
return this.createNode<TSESTree.Identifier>(node, {
type: AST_NODE_TYPES.Identifier,
name: node.text,
Expand Down
24 changes: 24 additions & 0 deletions packages/typescript-estree/src/node-utils.ts
View file
Open in desktop
Expand Up @@ -663,3 +663,27 @@ export function firstDefined<T, U>(
}
return undefined;
}

export function identifierIsThisKeyword(id: ts.Identifier): boolean {
return id.originalKeywordKind === SyntaxKind.ThisKeyword;
}

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

@armano2 armano2 Jan 7, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

return (
!!node &&
node.kind === SyntaxKind.Identifier &&
identifierIsThisKeyword(node as ts.Identifier)
);
}

export function isThisInTypeQuery(node: ts.Node): boolean {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto above - use a predicate return type

if (!isThisIdentifier(node)) {
return false;
}

while (ts.isQualifiedName(node.parent) && node.parent.left === node) {
node = node.parent;
}

return node.parent.kind === SyntaxKind.TypeQuery;
}