-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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.
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
allow this
in typeQuery
#43898
allow this
in typeQuery
#43898
Conversation
There was a problem hiding this 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.
tests/baselines/reference/initializerReferencingConstructorParameters.errors.txt
Outdated
Show resolved
Hide resolved
|
||
var str: typeof this.this = ''; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add tests of
- implicitly any usage, like
function f() { let x: typeof this.no = 1 }
. (x should have typeany
) - implicit any errors for the same code, but when noImplicitThis: true
- The same usage in
function f (this: { no: number })
- 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.
There was a problem hiding this comment.
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.
}
There was a problem hiding this comment.
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']
.
There was a problem hiding this comment.
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.
src/compiler/checker.ts
Outdated
@@ -27253,7 +27257,8 @@ namespace ts { | |||
} | |||
|
|||
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
src/compiler/utilities.ts
Outdated
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)); |
There was a problem hiding this comment.
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.
Fixes #1554