Skip to content

Conversation

@Zzzen
Copy link
Contributor

@Zzzen Zzzen commented Apr 30, 2021

Fixes #1554

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Apr 30, 2021
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Implementation looks good. One question about a test and a whole bunch of ideas for more tests.

We're close enough to the 4.3 release that I want to hold this one for 4.4.


var str: typeof this.this = '';
}
}
Copy link
Member

Choose a reason for hiding this comment

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

please add tests of

  1. implicitly any usage, like function f() { let x: typeof this.no = 1 }. (x should have type any)
  2. implicit any errors for the same code, but when noImplicitThis: true
  3. The same usage in function f (this: { no: number })
  4. usage inside arrow functions, to make sure that the emit is correct. It would be good to have an arrow function inside a container that binds this, like a class, and in one that doesn't, like a module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooops, this type can not be used in plain functions. 🤷

    function Test1() {
        let x: typeof this.no = 1
                      ~~~~
!!! error TS2526: A 'this' type is available only in a non-static member of a class or interface.
    }
    

Copy link
Member

Choose a reason for hiding this comment

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

That error is wrong, then, because this.no is an expression, not a type. The type would be like let x: this['no'].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not notice the differences between this type and this expression. Changing getThisType to checkThisExpression fixed it.

@Zzzen Zzzen mentioned this pull request May 14, 2021

function checkQualifiedName(node: QualifiedName, checkMode: CheckMode | undefined) {
return checkPropertyAccessExpressionOrQualifiedName(node, node.left, checkNonNullExpression(node.left), node.right, checkMode);
const leftType = isTypeQueryNode(node.parent) && isThisIdentifier(node.left) ? checkNonNullType(checkThisExpression(node.left), node.left) : checkNonNullExpression(node.left);
Copy link
Member

Choose a reason for hiding this comment

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

does this work when the QualifiedName is nested, like this.x.y? I think you need a test for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't work for this.x.y. I've replaced isTypeQueryNode with isPartOfTypeQuery

export function isThisInTypeQuery(node: Node): boolean {
return isThisIdentifier(node) &&
(node.parent.kind === SyntaxKind.TypeQuery ||
(node.parent.kind === SyntaxKind.QualifiedName && (node.parent as QualifiedName).left === node));
Copy link
Member

Choose a reason for hiding this comment

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

the QualifiedName case doesn't check that its ultimate parent is a TypeQuery, so I think you could maybe fool this with this.x in a weird location, or o.this.x The latter should be easy to add a test for; I don't know what test you need for the former.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

For Backlog Bug PRs that fix a backlog bug

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

typeof this.xxx gives "identifier expected" error.

3 participants